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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190625/398d4997/attachment.sig>