Re: [PATCH v11 2/7] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

[Leonardo Bras Soares Passos]

On Wed, May 4, 2022 at 4:53 PM Peter Xu <peterx@redhat.com> 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