On Fri, Dec 03, 2021 at 02:42:19AM -0300, Leonardo Bras Soares Passos wrote:
> Hello Daniel,
>
> On Tue, Nov 23, 2021 at 6:56 AM Daniel P. Berrangé <berra...@redhat.com>
> wrote:
> >
> > On Tue, Nov 23, 2021 at 01:46:44AM -0300, Leonardo Bras Soares Passos wrote:
> > > Hello Daniel,
> > >
> > > On Fri, Nov 12, 2021 at 7:54 AM Daniel P. Berrangé <berra...@redhat.com>
> > > wrote:
> > > > > +
> > > > > +#ifdef CONFIG_LINUX
> > > > > +
> > > > > +static int qio_channel_socket_poll(QIOChannelSocket *sioc, bool
> > > > > zerocopy,
> > > > > + Error **errp)
> > > >
> > > > There's only one caller and it always passes zerocopy=true,
> > > > so this parmeter looks pointless.
> > >
> > > I did that for possible reuse of this function in the future:
> > > - As of today, this is certainly compiled out, but if at some point
> > > someone wants to use poll for something other
> > > than the reading of an zerocopy errqueue, it could be reused.
> > >
> > > But sure, if that's not desirable, I can remove the parameter (and the
> > > if clause for !zerocopy).
> > >
> > > >
> > > > > +{
> > > > > + struct pollfd pfd;
> > > > > + int ret;
> > > > > +
> > > > > + pfd.fd = sioc->fd;
> > > > > + pfd.events = 0;
> > > > > +
> > > > > + retry:
> > > > > + ret = poll(&pfd, 1, -1);
> > > > > + if (ret < 0) {
> > > > > + switch (errno) {
> > > > > + case EAGAIN:
> > > > > + case EINTR:
> > > > > + goto retry;
> > > > > + default:
> > > > > + error_setg_errno(errp, errno,
> > > > > + "Poll error");
> > > > > + return ret;
> > > >
> > > > return -1;
> > > >
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + if (pfd.revents & (POLLHUP | POLLNVAL)) {
> > > > > + error_setg(errp, "Poll error: Invalid or disconnected fd");
> > > > > + return -1;
> > > > > + }
> > > > > +
> > > > > + if (!zerocopy && (pfd.revents & POLLERR)) {
> > > > > + error_setg(errp, "Poll error: Errors present in errqueue");
> > > > > + return -1;
> > > > > + }
> > > >
> > > > > +
> > > > > + return ret;
> > > >
> > > > return 0;
> > >
> > > In the idea of future reuse I spoke above, returning zero here would
> > > make this function always look like the poll timed out. Some future
> > > users may want to repeat the waiting if poll() timed out, or if
> > > (return > 0) stop polling.
> >
> > Now that I'm looking again, we should not really use poll() at all,
> > as GLib provides us higher level APIs. We in fact already have the
> > qio_channel_wait() method as a general purpose helper for waiting
> > for an I/O condition to occur.;
> >
>
> So you suggest using
> qio_channel_wait(sioc, G_IO_IN);
> instead of creating the new qio_channel_socket_poll().
>
> Is the above correct? I mean, is it as simple as that?
Yes, hopefully it is that simple.
> > > I understand the idea of testing SO_EE_CODE_ZEROCOPY_COPIED to be able
> > > to tell whenever zerocopy fell back to copying for some reason, but I
> > > don't see how this can be helpful here.
> > >
> > > Other than that I would do rv++ instead of rv=1 here, if I want to
> > > keep track of how many buffers were sent with zerocopy and how many
> > > ended up being copied.
> >
> > Sure, we could do "ret > 0 == number of buffers that were copied"
> > as the API contract, rather than just treating it as a boolean.
>
> Ok, then you suggest the responsibility of checking the number of
> writes with SO_EE_CODE_ZEROCOPY_COPIED, comparing with the total
> number of writes, and deciding whether to disable or not zerocopy
> should be on the caller.
Yep, its a usage policy so nicer to allow caller to decide the
policy.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
