On Thu, Sep 19, 2024 at 09:49:43PM +0200, Maciej S. Szmigiero wrote:
> On 10.09.2024 21:48, Peter Xu wrote:
> > On Wed, Aug 28, 2024 at 09:41:17PM -0300, Fabiano Rosas wrote:
> > > > +size_t multifd_device_state_payload_size(void)
> > > > +{
> > > > + return sizeof(MultiFDDeviceState_t);
> > > > +}
> > >
> > > This will not be necessary because the payload size is the same as the
> > > data type. We only need it for the special case where the MultiFDPages_t
> > > is smaller than the total ram payload size.
> >
> > Today I was thinking maybe we should really clean this up, as the current
> > multifd_send_data_alloc() is indeed too tricky (blame me.. who requested
> > that more or less). Knowing that VFIO can use dynamic buffers with ->idstr
> > and ->buf (I was thinking it could be buf[1M].. but I was wrong...) made
> > that feeling stronger.
> >
> > I think we should change it now perhaps, otherwise we'll need to introduce
> > other helpers to e.g. reset the device buffers, and that's not only slow
> > but also not good looking, IMO.
> >
> > So I went ahead with the idea in previous discussion, that I managed to
> > change the SendData union into struct; the memory consumption is not super
> > important yet, IMHO, but we should still stick with the object model where
> > multifd enqueue thread switch buffer with multifd, as it still sounds a
> > sane way to do.
> >
> > Then when that patch is ready, I further tried to make VFIO reuse multifd
> > buffers just like what we do with MultiFDPages_t->offset[]: in RAM code we
> > don't allocate it every time we enqueue.
> >
> > I hope it'll also work for VFIO. VFIO has a specialty on being able to
> > dump the config space so it's more complex (and I noticed Maciej's current
> > design requires the final chunk of VFIO config data be migrated in one
> > packet.. that is also part of the complexity there). So I allowed that
> > part to allocate a buffer but only that. IOW, I made some API (see below)
> > that can either reuse preallocated buffer, or use a separate one only for
> > the final bulk.
> >
> > In short, could both of you have a look at what I came up with below? I
> > did that in patches because I think it's too much to comment, so patches
> > may work better. No concern if any of below could be good changes to you,
> > then either Maciej can squash whatever into existing patches (and I feel
> > like some existing patches in this series can go away with below design),
> > or I can post pre-requisite patch but only if any of you prefer that.
> >
> > Anyway, let me know, the patches apply on top of this whole series applied
> > first.
> >
> > I also wonder whether there can be any perf difference already (I tested
> > all multifd qtest with below, but no VFIO I can run), perhaps not that
> > much, but just to mention below should avoid both buffer allocations and
> > one round of copy (so VFIO read() directly writes to the multifd buffers
> > now).
>
> I am not against making MultiFDSendData a struct and maybe introducing
> some pre-allocated buffer.
>
> But to be honest, that manual memory management with having to remember
> to call multifd_device_state_finish() on error paths as in your
> proposed patch 3 really invites memory leaks.
>
> Will think about some other way to have a reusable buffer.
Sure. That's patch 3, and I suppose then it looks like patch 1 is still
OK in one way or another.
>
> In terms of not making idstr copy (your proposed patch 2) I am not
> 100% sure that avoiding such tiny allocation really justifies the risk
> of possible use-after-free of a dangling pointer.
Why there's risk? Someone strdup() on the stack? That only goes via VFIO
itself, so I thought it wasn't that complicated. But yeah as I said this
part (patch 2) is optional.
> Not 100% against it either if you are confident that it will never happen.
>
> By the way, I guess it makes sense to carry these changes in the main patch
> set rather than as a separate changes?
Whatever you prefer.
I wrote those patches only because I thought maybe you'd like to run some
perf test to see whether they would help at all, and when the patches are
there it'll be much easier for you, then you can decide whether it's worth
intergrating already, or leave that for later.
If not I'd say they're even lower priority, so feel free to stick with
whatever easier for you. I'm ok there.
However it'll be always good we can still have patch 1 as I mentioned
before (as part of your series, if you won't disagree), to make the
SendData interface slightly cleaner and easier to follow.
--
Peter Xu
