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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
