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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Dec 03, 2021 at 02:24:52AM -0300, Leonardo Bras Soares Passos wrote:
> Hello Daniel,
> 
> On Tue, Nov 23, 2021 at 6:45 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > 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.
> >
> 
> Correct,
> 
> > 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.
> 
> So, here you say that once a message has SO_EE_CODE_ZEROCOPY_COPIED we
> should return 1.
> Is the idea here to understand if zerocopy is working at all, so we
> can disable zerocopy and avoid overhead ?
> 
> If so, we should somehow check if every write was sent with
> SO_EE_CODE_ZEROCOPY_COPIED instead.
> I mean, we should not disable Zerocopy if a single write got
> SO_EE_CODE_ZEROCOPY_COPIED ?

Honestly I'm not sure whether there are scenarios where you'll get
one write fail and others succeed. If we want to be paranoid though,
we could check for all writes, rather than jhust one write, so we
don't disable zerocopy on transient issues. 


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