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

[Richard W.M. Jones]

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 at 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