Hello Daniel, On Tue, Nov 23, 2021 at 6:45 AM Daniel P. Berrangé <berra...@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é <berra...@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-leob...@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