On Mon, Jun 13, 2022 at 06:21:18PM -0300, Leonardo Bras Soares Passos wrote: > On Fri, Jun 10, 2022 at 5:25 AM Daniel P. Berrangé <berra...@redhat.com> > wrote: > > > > [...] > > > Ok, so if it is checked earlier then we merely need an assert. > > > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) { > > #ifdef QEMU_MSG_ZEROCOPY > > sflags = MSG_ZEROCOPY; > > zero_copy_enabled = true; > > #else > > g_assert_unreachable(); > > #endif > > > } > > Ok, I will add that in the next version. > > > > > > > > > > > > @@ -592,15 +594,13 @@ static ssize_t > > > > > qio_channel_socket_writev(QIOChannel *ioc, > > > > > return QIO_CHANNEL_ERR_BLOCK; > > > > > case EINTR: > > > > > goto retry; > > > > > -#ifdef QEMU_MSG_ZEROCOPY > > > > > case ENOBUFS: > > > > > - if (sflags & MSG_ZEROCOPY) { > > > > > + if (zero_copy_enabled) { > > > > > > > > if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) > > > > > > > > avoids the #ifdef without needing to add yet another > > > > variable expressing what's already expressed in both > > > > 'flags' and 'sflags'. > > > > > > Yes, it does, but at the cost of not compiling-out the zero-copy part > > > when it's not supported, > > > since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This > > > ends up > > > meaning there will be at least one extra test for every time this > > > function is called (the one in the next patch). > > > > The cost of a simple bit test is between negligible-and-non-existant > > with branch prediction. I doubt it would be possible to even measure > > it. > > Yeah, you are probably right on that. > So the main learning point here is that it's not worth creating a new > boolean for compiling-out > code that should not impact performance ?
As ever "it depends" so there's no hard rule, and sometimes it can verge on bikeshed colouring :-) I didn't like the variable in this case, because it introduces a 3rd variable to the method for representing whether zero copy is need, which is excessive. I'm not a fan of redundancy as it can often then lead to inconsistency. So it would need a compelling reason why it is better, which is difficult for such a simple method. If the code was more complex, a variable might have benefit of clarity, but in this case IMHO it was just overkill. With 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 :|