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
