Re: [PATCH v2 12/17] migration/multifd: Device state transfer support - send side

[Peter Xu <peterx@redhat.com>]

On Tue, Sep 17, 2024 at 07:07:10PM +0200, Cédric Le Goater wrote:
> [ ... ]
> 
> > > > I as a patch writer always like to do that when it's essential.  Normally
> > > > the case is I don't have enough reviewer resources to help me get a better
> > > > design, or discuss about it.
> > > 
> > > Right, but we can't keep providing a moving target. See the thread pool
> > > discussion for an example. It's hard to work that way. The discussion
> > > here is similar, we introduced the union, now we're moving to the
> > > struct. And you're right that the changes here are small, so let's not
> > > get caught in that.
> > 
> > What's your suggestion on the thread pool?  Should we merge the change
> > where vfio creates the threads on its own (assuming vfio maintainers are ok
> > with it)?
> > 
> > I would say no, that's what I suggested.  I'd start with reusing
> > ThreadPool, then we found issue when Stefan reported worry on abusing the
> > API.  All these discussions seem sensible to me so far.  We can't rush on
> > these until we figure things out step by step.  I don't see a way.
> > 
> > I saw Cedric suggesting to not even create a thread on recv side.  I am not
> > sure whether that's easy, but I'd agree with Cedric if possible.  I think
> > Maciej could have a point where it can block mutlifd threads, aka, IO
> > threads, which might be unwanted.
> 
> Sorry, If I am adding noise on this topic. I made this suggestion
> because I spotted some asymmetry in the proposal.
> 
> The send and recv implementation in VFIO relies on different
> interfaces with different level of complexity. The send part is
> using a set of multifd callbacks called from multifd threads,
> if I am correct. Whereas the recv part is directly implemented
> in VFIO with local thread(s?) doing their own state receive cookery.

Yeh, the send/recv sides are indeed not fully symmetrical in the case of
multifd - the recv side is more IO-driven, e.g., QEMU reacts based on what
it receives (which was encoded in the headers of the received packets).

The src is more of a generic consumer / producer model where threads can
enqueue tasks / data to different iochannels.

> 
> I was expecting a common interface to minimize assumptions on both
> ends. It doesn't have to be callback based. It could be a set of
> services a subsystem could use to transfer state in parallel.
> <side note>
>      VFIO migration is driven by numerous callbacks and it is
>      difficult to understand the context in which these are called.
>      Adding more callbacks might not be the best approach.
> </side note>
> 
> The other comment was on optimisation. If this is an optimisation
> then I would expect, first, a non-optimized version not using threads
> (on the recv side).

As commented in a previous email, I had a feeling that Maciej wanted to
avoid blocking multifd threads when applying VFIO data chunks to the kernel
driver, but Maciej please correct me.. I could be wrong.

To me I think I'm fine even if it blocks multifd threads, as it'll only
happen when with VFIO (we may want to consider n_multifd_threads to be
based on num of vfio devices then, so we still always have some idle
threads taking IOs out of the NIC buffers).

So I agree with Cedric that if we can provide a functional working version
first then we can at least go with the simpler approach first.

> 
> VFIO Migration is a "new" feature which needs some more run-in.
> That said, it is stable, MLX5 VFs devices have good support, you
> can rely on me to evaluate the future respins.
> 
> Thanks,
> 
> C.
> 

-- 
Peter Xu