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

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