| Message ID | 20190918231846.22538-14-alxndr@bu.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add virtual device fuzzing support | expand |
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 <alxndr@bu.edu> > --- > 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.
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 <alxndr@bu.edu> >> --- >> 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
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 <alxndr@bu.edu<mailto:alxndr@bu.edu>> > > > --- > > > 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. 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);
On 9/19/19 2:56 PM, 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 <alxndr@bu.edu> >>> --- >>> 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. 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);
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 <alxndr@bu.edu >>>> <mailto:alxndr@bu.edu>> >>>> > > > --- >>>> > > > 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
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); }
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 <alxndr@bu.edu> --- tests/libqtest.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)