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
Attachments:
- signature.asc (application/pgp-signature — 488 bytes)