Re: [PATCH v5 1/6] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks

[Daniel P . Berrangé]

On Mon, Nov 22, 2021 at 08:18:09PM -0300, Leonardo Bras Soares Passos wrote:
> Hello Daniel,
> Thanks for the feedback!
> 
> On Fri, Nov 12, 2021 at 7:13 AM Daniel P. Berrangé <berrange@redhat.com> 
> wrote:
> >
> > On Fri, Nov 12, 2021 at 02:10:36AM -0300, Leonardo Bras wrote:
> > > -int qio_channel_writev_all(QIOChannel *ioc,
> > > -                           const struct iovec *iov,
> > > -                           size_t niov,
> > > -                           Error **erp);
> > > +int qio_channel_writev_all_flags(QIOChannel *ioc,
> > > +                                 const struct iovec *iov,
> > > +                                 size_t niov,
> > > +                                 int flags,
> > > +                                 Error **errp);
> > > +#define qio_channel_writev_all(ioc, iov, niov, errp) \
> > > +    qio_channel_writev_all_flags(ioc, iov, niov, 0, errp)
> >
> > We already have separate methods for zerocopy, instead of adding
> > flags, so we shouldn't add flags to this either.
> >
> > Add a qio_channel_writev_zerocopy_all method instead.
> >
> > Internally, we can still make both qio_channel_writev_zerocopy_all
> > and qio_channel_writev_all use the same helper method, just don't
> > expose flags in the public API. Even internally we don't really
> > need flags, just a bool
> 
> I see.
> The idea of having a flag was to make it easier to expand the
> interface in the future.
> I got some feedback on v1 that would suggest it would be desired:
> http://patchwork.ozlabs.org/project/qemu-devel/patch/20210831110238.299458-2-leobras@redhat.com/
> 
> 
> >
> [...]
> > > +#define qio_channel_writev_full_all(ioc, iov, niov, fds, nfds, errp) \
> > > +    qio_channel_writev_full_all_flags(ioc, iov, niov, fds, nfds, 0, errp)
> >
> > There's no need for this at all. Since fd passing is not supported
> > with zerocopy, there's no reason to ever use this method.
> >
> > > +/**
> > > + * qio_channel_writev_zerocopy:
> > > + * @ioc: the channel object
> > > + * @iov: the array of memory regions to write data from
> > > + * @niov: the length of the @iov array
> > > + * @errp: pointer to a NULL-initialized error object
> > > + *
> > > + * Behaves like qio_channel_writev_full_all_flags, but may write
> >
> > qio_channel_writev
> >
> > > + * data asynchronously while avoiding unnecessary data copy.
> > > + * This function may return before any data is actually written,
> > > + * but should queue every buffer for writing.
> >
> > Callers mustn't rely on "should" docs - they must rely on the
> > return value indicating how many bytes were accepted.
> >
> > Also mention that this requires locked memory and can/will fail if
> > insufficient locked memory is available.
> >
> 
> Sure, I will update that.
> 
> > > +/**
> > > + * qio_channel_flush_zerocopy:
> > > + * @ioc: the channel object
> > > + * @errp: pointer to a NULL-initialized error object
> > > + *
> > > + * Will block until every packet queued with
> > > + * qio_channel_writev_zerocopy() is sent, or return
> > > + * in case of any error.
> > > + *
> > > + * Returns -1 if any error is found, 0 otherwise.
> >
> >   Returns -1 if any error is found, 0 if all data was sent,
> >            or 1 if all data was sent but at least some was copied.
> >
> 
> I don't really get the return 1 part, I mean, per description it will
> 'block until every queued packet was sent, so "at least some was
> copied" doesn't seem to fit here.
> Could you elaborate?

Passing the ZEROCOPY flag to the kernel does not guarantee
that the copy is avoided, it is merely a hint to the kernel

When getting the notification, the ee_code  field in the
notification struct will have the flag
SO_EE_CODE_ZEROCOPY_COPIED  set if the kernel could not
avoid the copy.

In this case, it is better for the application to stop
using the ZEROCOPY flag and just do normal writes, so
it avoids the overhead of the the notifications.

This is described in "Deferred copies" section of the
Documentation/networking/msg_zerocopy.rst in linux.git

So I'm suggesting that the return value of this method
be '0' if SO_EE_CODE_ZEROCOPY_COPIED was *NOT* set in
/all/ notifications, or '1' if SO_EE_CODE_ZEROCOPY_COPIED
was set in at least one notification.


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 :|