Libguestfs - Jun 2019 - Re: [libnbd PATCH 2/1] states: Avoid wasted send() when REPLY interrupts request

On Wed, Jun 19, 2019 at 09:11:52PM -0500, Eric Blake
wrote:
> When we are blocked waiting for POLLOUT during a request, and happen
> to receive notice of POLLIN instead, we know that the work done in
> response to POLLIN will be non-blocking (it returns to %.READY as soon
> as it would block, which in turn jumps right back into ISSUE_COMMAND
> because we have a pending request not fully sent yet).  Since the
> jaunt through REPLY was non-blocking, it is unlikely that the POLLOUT
> situation has changed in the meantime, so if we use SET_NEXT_STATE()
> to step back into SEND_REQUEST, our recv() call will likely fail with
> EAGAIN, once again blocking us until our next POLLOUT.  Although the
> wasted syscall is not on the hot-path (after all, we can't progress
> until data arrives from the server), it's slightly cleaner if we
> instead declare that we are already blocked.
> 
> I tested with:
> $ nbdkit -U - null 16M --run 'examples/threaded-reads-and-writes
$unixsocket'
> 
> There was no real noticeable difference in timing, but I did observe
> that pre-patch, the run encountered 168825 pre-emptions and 136976
> send() EAGAIN failures (81%), while post-patch the run encountered
> 166066 pre-emptions and 552 EAGAIN failures (0.3%).
> ---
>  generator/states-issue-command.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/generator/states-issue-command.c
b/generator/states-issue-command.c
> index 821b28a..f746f80 100644
> --- a/generator/states-issue-command.c
> +++ b/generator/states-issue-command.c
> @@ -25,12 +25,17 @@
>    assert (h->cmds_to_issue != NULL);
>    cmd = h->cmds_to_issue;
> 
> -  /* Were we interrupted by reading a reply to an earlier command? */
> +  /* Were we interrupted by reading a reply to an earlier command? If
> +   * so, we can only get back here after a non-blocking jaunt through
> +   * the REPLY engine, which means we are unlikely to be unblocked for
> +   * writes yet; we want to advance back to the correct state but
> +   * without trying a send_from_wbuf that will likely return 1.
> +   */
>    if (h->wlen) {
>      if (h->in_write_payload)
> -      SET_NEXT_STATE(%SEND_WRITE_PAYLOAD);
> +      *next_state = %SEND_WRITE_PAYLOAD;
>      else
> -      SET_NEXT_STATE(%SEND_REQUEST);
> +      *next_state = %SEND_REQUEST;
It would be nice to do this without fiddling with essentially an
internal detail of the generated code.

Could we add another macro, something like "SET_NEXT_STATE_AND_BLOCK"?

On the other hand if it's not on the hot path, maybe we shouldn't
do this at all?

Rich.
>      return 0;
>    }
> 
> -- 
> 2.20.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On 6/25/19 4:06 AM, Richard W.M. Jones wrote:
> On Wed, Jun 19, 2019 at 09:11:52PM -0500, Eric Blake wrote:
>> When we are blocked waiting for POLLOUT during a request, and happen
>> to receive notice of POLLIN instead, we know that the work done in
>> response to POLLIN will be non-blocking (it returns to %.READY as soon
>> as it would block, which in turn jumps right back into ISSUE_COMMAND
>> because we have a pending request not fully sent yet).  Since the
>> jaunt through REPLY was non-blocking, it is unlikely that the POLLOUT
>> situation has changed in the meantime, so if we use SET_NEXT_STATE()
>> to step back into SEND_REQUEST, our recv() call will likely fail with
>> EAGAIN, once again blocking us until our next POLLOUT.  Although the
>> wasted syscall is not on the hot-path (after all, we can't progress
>> until data arrives from the server), it's slightly cleaner if we
>> instead declare that we are already blocked.
>>
>>    if (h->wlen) {
>>      if (h->in_write_payload)
>> -      SET_NEXT_STATE(%SEND_WRITE_PAYLOAD);
>> +      *next_state = %SEND_WRITE_PAYLOAD;
>>      else
>> -      SET_NEXT_STATE(%SEND_REQUEST);
>> +      *next_state = %SEND_REQUEST;
> 
> It would be nice to do this without fiddling with essentially an
> internal detail of the generated code.
> 
> Could we add another macro, something like
"SET_NEXT_STATE_AND_BLOCK"?
Yes, that's a nice idea, and easy enough to squash in.
> 
> On the other hand if it's not on the hot path, maybe we shouldn't
> do this at all?
It wasn't on the hot path on any test I could come up with (where we
were waiting for the server anyway); but it may still be possible to
come up with a scenario where it matters more.

Should I push with this squashed in?

diff --git i/generator/generator w/generator/generator
index 34e70da..cbf4e59 100755
--- i/generator/generator
+++ w/generator/generator
@@ -2541,6 +2541,7 @@ let generate_lib_states_c ()    pr "%s\n"
state_machine_prologue;
   pr "\n";
   pr "#define SET_NEXT_STATE(s) (*blocked = false, *next_state =
(s))\n";
+  pr "#define SET_NEXT_STATE_AND_BLOCK(s) (*next_state = (s))\n";
   pr "\n";

   (* The state machine C code fragments. *)
diff --git i/generator/states-issue-command.c
w/generator/states-issue-command.c
index 9fc8c93..35f3c79 100644
--- i/generator/states-issue-command.c
+++ w/generator/states-issue-command.c
@@ -33,9 +33,9 @@
    */
   if (h->wlen) {
     if (h->in_write_payload)
-      *next_state = %SEND_WRITE_PAYLOAD;
+      SET_NEXT_STATE_AND_BLOCK (%SEND_WRITE_PAYLOAD);
     else
-      *next_state = %SEND_REQUEST;
+      SET_NEXT_STATE_AND_BLOCK (%SEND_REQUEST);
     return 0;
   }



-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Jun 25, 2019 at 11:51:42AM -0500, Eric Blake
wrote:
> On 6/25/19 4:06 AM, Richard W.M. Jones wrote:
> > On Wed, Jun 19, 2019 at 09:11:52PM -0500, Eric Blake wrote:
> >> When we are blocked waiting for POLLOUT during a request, and
happen
> >> to receive notice of POLLIN instead, we know that the work done in
> >> response to POLLIN will be non-blocking (it returns to %.READY as
soon
> >> as it would block, which in turn jumps right back into
ISSUE_COMMAND
> >> because we have a pending request not fully sent yet).  Since the
> >> jaunt through REPLY was non-blocking, it is unlikely that the
POLLOUT
> >> situation has changed in the meantime, so if we use
SET_NEXT_STATE()
> >> to step back into SEND_REQUEST, our recv() call will likely fail
with
> >> EAGAIN, once again blocking us until our next POLLOUT.  Although
the
> >> wasted syscall is not on the hot-path (after all, we can't
progress
> >> until data arrives from the server), it's slightly cleaner if
we
> >> instead declare that we are already blocked.
> >>
> 
> >>    if (h->wlen) {
> >>      if (h->in_write_payload)
> >> -      SET_NEXT_STATE(%SEND_WRITE_PAYLOAD);
> >> +      *next_state = %SEND_WRITE_PAYLOAD;
> >>      else
> >> -      SET_NEXT_STATE(%SEND_REQUEST);
> >> +      *next_state = %SEND_REQUEST;
> > 
> > It would be nice to do this without fiddling with essentially an
> > internal detail of the generated code.
> > 
> > Could we add another macro, something like
"SET_NEXT_STATE_AND_BLOCK"?
> 
> Yes, that's a nice idea, and easy enough to squash in.
> 
> > 
> > On the other hand if it's not on the hot path, maybe we
shouldn't
> > do this at all?
> 
> It wasn't on the hot path on any test I could come up with (where we
> were waiting for the server anyway); but it may still be possible to
> come up with a scenario where it matters more.
> 
> Should I push with this squashed in?
Yes, ACK

Thanks,

Rich.
> diff --git i/generator/generator w/generator/generator
> index 34e70da..cbf4e59 100755
> --- i/generator/generator
> +++ w/generator/generator
> @@ -2541,6 +2541,7 @@ let generate_lib_states_c () >    pr
"%s\n" state_machine_prologue;
>    pr "\n";
>    pr "#define SET_NEXT_STATE(s) (*blocked = false, *next_state =
(s))\n";
> +  pr "#define SET_NEXT_STATE_AND_BLOCK(s) (*next_state =
(s))\n";
>    pr "\n";
> 
>    (* The state machine C code fragments. *)
> diff --git i/generator/states-issue-command.c
> w/generator/states-issue-command.c
> index 9fc8c93..35f3c79 100644
> --- i/generator/states-issue-command.c
> +++ w/generator/states-issue-command.c
> @@ -33,9 +33,9 @@
>     */
>    if (h->wlen) {
>      if (h->in_write_payload)
> -      *next_state = %SEND_WRITE_PAYLOAD;
> +      SET_NEXT_STATE_AND_BLOCK (%SEND_WRITE_PAYLOAD);
>      else
> -      *next_state = %SEND_REQUEST;
> +      SET_NEXT_STATE_AND_BLOCK (%SEND_REQUEST);
>      return 0;
>    }
> 
> 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html