On Wed, May 4, 2022 at 4:53 PM Peter Xu <[email protected]> wrote: > > On Wed, May 04, 2022 at 04:18:31PM -0300, Leonardo Bras wrote: > > +/* > > + * Zero-copy defines bellow are included to avoid breaking builds on > > systems > > + * that don't support MSG_ZEROCOPY, while keeping the functions more > > readable > > + * (without a lot of ifdefs). > > + */ > > +#ifndef MSG_ZEROCOPY > > +#define MSG_ZEROCOPY 0x4000000 > > +#endif > > +#ifndef SO_ZEROCOPY > > +#define SO_ZEROCOPY 60 > > +#endif > > So this will define these two values on e.g. FreeBSD, while they do not > make sense at all there because these numbers are pure magics and > meaningless outside Linux..
Correct. But since only in Linux it's possible to set the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY flag, sflags will always be zero and it would never try using MSG_ZEROCOPY outside Linux. > I don't think it's anything dangerous, but IMHO it's another way of being > not clean comparing of using some "#ifdef"s. Comparing to this approach > the "use #ifdef" approach is actually slightly more cleaner to me. :) > This requires: - Creating a define such as 'QEMU_MSG_ZEROCOPY', that needs to include <sys/socket.h> to get some flags: #define QEMU_MSG_ZEROCOPY defined(CONFIG_LINUX) && defined(MSG_ZEROCOPY) && defined(SO_ZEROCOPY) - Making it available for all code in this patchset that does "ifdef CONFIG_LINUX' (migration/migration.c/h, qapi/migration.json, monitor/hmp-cmds.c, io/channel-socket.c) - Replace current usage of CONFIG_LINUX in this patchset for QEMU_MSG_ZEROCOPY - Change qio_channel_socket_writev() so the current 2 usages of MSG_ZEROCOPY are surrounded by ifdef QEMU_MSG_ZEROCOPY. Pros of above approach (1): - Smaller binary: The whole MSG_ZEROCOPY code is compiled out if the building system does not support it. - Since it's compiled out, there is a couple lines of less code running if the building system does not support it - It's not even possible to set this option in MigrationSetParams, which will return an error. Pros of current approach (2): - Define is local to file (I am not sure if it's ok to create a 'global' define for above approach, including <sys/socket.h> bits) - A build system that does not support MSG_ZEROCOPY can produce a binary that can use MSG_ZEROCOPY if the target system supports it. - There are no #ifdefs on qio_channel_socket_writev() (2) is already implemented in v11, but I have no issue implementing (1) for v12 if it's ok to create this 'global' define. > Let's wait for some other inputs. Agree. Having the pros of each approach clear, I would like some input on what is better for the project. Best regards, Leo
