On Fri, Sep 20, 2024 at 05:23:20PM +0200, Maciej S. Szmigiero wrote:
> On 19.09.2024 23:17, Peter Xu wrote:
> > 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.
>
> I mean the risk here is somebody providing idstr that somehow gets free'd
> or overwritten before the device state buffer gets sent.
>
> With a static idstr that's obviously not an issue, but I see that, for
> example,
> vmstate_register_with_alias_id() generates idstr dynamically and this API
> is used by all qdevs that have a VMSD (in device_set_realized()).
>
> > > 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.
> >
>
> Will try to include these patches in my patch set if they don't cause any
> downtime regressions.
Thanks, note that it's not my request to have patch 2-3. :) Please take
your own judgement.
Again, I'd run a round of perf (only if my patches can directly run through
without heavy debugging on top of yours), if nothing shows a benefit I'd go
with what you have right now, and we drop patch 2-3 - they're not justified
if they provide zero perf benefit.
--
Peter Xu
