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

[Leonardo Bras Soares Passos]

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 ?



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

Best regards,
Leo