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.
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.
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?
Thanks,
Thanks,
Maciej
