Re: [PATCH v3 13/22] libqtest: make qtest_bufwrite send "atomic"

[John Snow]

On 9/19/19 3:36 PM, Oleinik, Alexander wrote:
> On Thu, 2019-09-19 at 14:56 -0400, John Snow wrote:
>> >
>> > On 9/19/19 6:37 AM, Stefan Hajnoczi wrote:
>>> > > On Wed, Sep 18, 2019 at 11:19:40PM +0000, Oleinik, Alexander wrote:
>>>> > > > When using qtest "in-process" communication, qtest_sendf directly
>>>> > > > calls
>>>> > > > a function in the server (qtest.c). Combining the contents of the
>>>> > > > subsequent socket_sends into the qtest_sendf, makes it so the
>>>> > > > server can
>>>> > > > immediately handle the command, without building a local buffer
>>>> > > > and
>>>> > > > waiting for a newline.
>>>> > > >
>>>> > > > Signed-off-by: Alexander Oleinik <address@hidden
>>>> <mailto:address@hidden>>
>>>> > > > ---
>>>> > > >  tests/libqtest.c | 4 +---
>>>> > > >  1 file changed, 1 insertion(+), 3 deletions(-)
>>>> > > >
>>>> > > > diff --git a/tests/libqtest.c b/tests/libqtest.c
>>>> > > > index 19feea9e17..d770462869 100644
>>>> > > > --- a/tests/libqtest.c
>>>> > > > +++ b/tests/libqtest.c
>>>> > > > @@ -1086,9 +1086,7 @@ void qtest_bufwrite(QTestState *s, uint64_t
>>>> > > > addr, const void *data, size_t size)
>>>> > > >      gchar *bdata;
>>>> > > >  
>>>> > > >      bdata = g_base64_encode(data, size);
>>>> > > > -    qtest_sendf(s, "b64write 0x%" PRIx64 " 0x%zx ", addr, size);
>>>> > > > -    socket_send(s->fd, bdata, strlen(bdata));
>>>> > > > -    socket_send(s->fd, "\n", 1);
>>>> > > > +    qtest_sendf(s, "b64write 0x%" PRIx64 " 0x%zx %s\n", addr,
>>>> > > > size, bdata);
>>>> > > >      qtest_rsp(s, 0);
>>>> > > >      g_free(bdata);
>>>> > > >  }
>>>> > > > -- 
>>>> > > > 2.23.0
>>> > >
>>> > > Cc John Snow, who added the b64write command.
>>> > >
>>> > > The downside to doing this is that sprintf-formatting needs to be
>>> > > performed on the entire base64 buffer.  This slows things down
>>> > > slightly
>>> > > and a larger temporary buffer needs to be allocated, but I'm not
>>> > > sure it
>>> > > matters.
>>> > >
>> >
>> > *struggles to remember*
>> >
>> > I guess I wanted something that had some space savings while
>> > maintaining
>> > some semblance of debuggability. This is almost certainly meant for
>> > AHCI
>> > tests where it's writing various patterns to large blocks of memory.
>> >
>> > I doubt I really measured the performance of it, but it seemed like
>> > the
>> > way to go for transferring medium amounts of data at the time via the
>> > qtest protocol.
>> >
>> > Looks like I am the only user of it, still:
>> >
>> > tests/ahci-test.c:    qtest_bufwrite(ahci->parent->qts, ptr, tx,
>> > bufsize);
>> > tests/ahci-test.c:    qtest_bufwrite(ahci->parent->qts, ptr, tx,
>> > bufsize);
>> > tests/libqos/ahci.c:        qtest_bufwrite(ahci->parent->qts, ptr,
>> > buffer, bufsize);
>> >
>> > The buffers can be quite large, so you might be re-buffering a decent
>> > amount of data from the sender now.
>> >
>> > 1, Are large transfers like this guaranteed to be atomic anyway? What
>> > kind of socket is it? we're probably eclipsing frame and packet sizes
>> > here.
>> >
>> > 2, I am not sure what being "atomic" affords us in terms of allowing
>> > the server to not wait for newlines, how does this change help?
>> >
>> > --js
> 
> I'm modifying qtest to allow the server and client to co-exist within
> the same process (facilitating coverage-guided fuzzing). One of the
> modifications is making qtest_sendf directly call a function in
> qtest.c. All the other qtest commands are sent with a single
> qtest_sendf call, so the qtest.c function could immediately call
> qtest_process_command. This breaks if the command is sent with
> different qtest_send/socket_send calls, as in b64write.
> 
> It should be simple to change qtest_server_inproc_recv (the qtest.c
> receiver) to wait for an "\n" prior to qtest_process_command, so I will
> probably do that and then normal(socket) qtest will keep the
> memory-reduction benefits of the non-"atomic" approach.
> 

Before you spend the effort, just benchmark it if you can. I'd look at
the before and after of the AHCI tests with and without the pre-buffering.

> As a side note, would qtest_memwrite, also benefit from splitting up the
> send command?
> 
>     for (i = 0; i < size; i++) {
>         sprintf(&enc[i * 2], "%02x", ptr[i]);
>     }
> 
>     qtest_sendf(s, "write 0x%" PRIx64 " 0x%zx 0x%s\n", addr, size, enc);
>     qtest_rsp(s, 0);
>     g_free(enc);

Depends on the users. I think at the time I wrote bufwrite it was using
obviously more data. Some of the memwrite users might be writing pretty
small things.

Looks like users are:

tests/ahci-test
tests/ide-test
tests/libqos/e1000e.c
tests/libqos/pci-pc.c
tests/libqos/pci-spapr.c
tests/megasas-test.c
tests/tpm-util.c
tests/virtio-9p-test.c

the libqos ones are used by other tests -- you can give it a skim and
come up with a small list of tests to benchmark and see if it makes any
actual difference.

--js