Thanks for this feedback Peter! I ended up reading/replying the e-mails in thread order, so I may have been redundant with your argument, sorry about that.
I will add my comments inline, but I will add references to the previous mail I sent Daniel, so please read it too. On Tue, Aug 31, 2021 at 5:27 PM Peter Xu <pet...@redhat.com> wrote: [snip] > > > > I can't see how we can introduce MSG_ZEROCOPY in any seemless > > way. The buffer lifetime requirements imply need for an API > > design that is fundamentally different for asynchronous usage, > > with a callback to notify when the write has finished/failed. > > Regarding buffer reuse - it indeed has a very deep implication on the buffer > being available and it's not obvious at all. Just to mention that the initial > user of this work will make sure all zero copy buffers will be guest pages > only > (as it's only used in multi-fd), so they should always be there during the > process. Thanks for pointing that out, what's what I had in mind at the time. > > I think asking for a complete design still makes sense. Agree, since I am touching QIOChannel, it makes sense to make it work for other code that uses it too, not only our case. > E.g., for precopy > before we flush device states and completes the migration, we may want to at > least have a final ack on all the zero-copies of guest pages to guarantee they > are flushed. > > IOW, we need to make sure the last piece of migration stream lands after the > guest pages so that the dest VM will always contain the latest page data when > dest VM starts. So far I don't see how current code guaranteed that. > > In short, we may just want to at least having a way to make sure all zero > copied buffers are finished using and they're sent after some function returns > (e.g., qio_channel_flush()). That may require us to do some accounting on > when > we called sendmsg(MSG_ZEROCOPY), meanwhile we should need to read out the > ee_data field within SO_EE_ORIGIN_ZEROCOPY msg when we do recvmsg() for the > error queue and keep those information somewhere too. Yeah, that's correct. I haven't fully studied what the returned data represents, but I suppose this could be a way to fix that. In my previous reply to Daniel I pointed out a way we may achieve a flush behavior with poll() too, but it could be a little hacky. > > Some other side notes that reached my mind.. > > The qio_channel_writev_full() may not be suitable for async operations, as the > name "full" implies synchronous to me. So maybe we can add a new helper for > zero copy on the channel? > > We may also want a new QIOChannelFeature as QIO_CHANNEL_FEATURE_ZEROCOPY, then > we fail qio_channel_writv_zerocopy() (or whatever name we come up with) if > that > bit is not set in qio channel features. I also suggested something like that, but I thought it could be good if we could fall back to io_writev() if we didn't have the zerocopy feature (or the async feature). What do you think? > > Thanks, > > -- > Peter Xu > I really appreciate your suggestions, thanks Peter! Best regards, Leonardo