qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1 00/13] Multifd 🔀 device state transfer support with VFIO c

From: Maciej S. Szmigiero
Subject: Re: [PATCH v1 00/13] Multifd 🔀 device state transfer support with VFIO consumer
Date: Wed, 26 Jun 2024 17:47:34 +0200
User-agent: Mozilla Thunderbird 
On 26.06.2024 03:51, Peter Xu wrote:

On Wed, Jun 26, 2024 at 12:44:29AM +0200, Maciej S. Szmigiero wrote:

On 25.06.2024 19:25, Peter Xu wrote:

On Mon, Jun 24, 2024 at 09:51:18PM +0200, Maciej S. Szmigiero wrote:

Hi Peter,


Hi, Maciej,



On 23.06.2024 22:27, Peter Xu wrote:

On Tue, Jun 18, 2024 at 06:12:18PM +0200, Maciej S. Szmigiero wrote:

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

This is an updated v1 patch series of the RFC (v0) series located here:
https://lore.kernel.org/qemu-devel/cover.1713269378.git.maciej.szmigiero@oracle.com/


OK I took some hours thinking about this today, and here's some high level
comments for this series.  I'll start with which are more relevant to what
Fabiano has already suggested in the other thread, then I'll add some more.

20240620212111.29319-1-farosas@suse.de">https://lore.kernel.org/r/20240620212111.29319-1-farosas@suse.de


That's a long list, thanks for these comments.

I have responded to them inline below.


(..)



3. load_state_buffer() and VFIODeviceStatePacket protocol
=========================================================

VFIODeviceStatePacket is the new protocol you introduced into multifd
packets, along with the new load_state_buffer() hook for loading such
buffers.  My question is whether it's needed at all, or.. whether it can be
more generic (and also easier) to just allow taking any device state in the
multifd packets, then load it with vmstate load().

I mean, the vmstate_load() should really have worked on these buffers, if
after all VFIO is looking for: (1) VFIO_MIG_FLAG_DEV_DATA_STATE as the
first flag (uint64), size as the 2nd, then (2) load that rest buffer into
VFIO kernel driver.  That is the same to happen during the blackout window.
It's not clear to me why load_state_buffer() is needed.

I also see that you're also using exactly the same chunk size for such
buffering (VFIOMigration.data_buffer_size).

I think you have a "reason": VFIODeviceStatePacket and loading of the
buffer data resolved one major issue that wasn't there before but start to
have now: multifd allows concurrent arrivals of vfio buffers, even if the
buffer *must* be sequentially loaded.

That's a major pain for current VFIO kernel ioctl design, IMHO.  I think I
used to ask nVidia people on whether the VFIO get_state/set_state interface
can allow indexing or tagging of buffers but I never get a real response.
IMHO that'll be extremely helpful for migration purpose on concurrency if
it can happen, rather than using a serialized buffer.  It means
concurrently save/load one VFIO device could be extremely hard, if not
impossible.


I am pretty sure that the current kernel VFIO interface requires for the
buffers to be loaded in-order - accidentally providing the out of order
definitely breaks the restore procedure.


Ah, I didn't mean that we need to do it with the current API.  I'm talking
about whether it's possible to have a v2 that will support those otherwise
we'll need to do "workarounds" like what you're doing with "unlimited
buffer these on dest, until we receive continuous chunk of data" tricks.


Better kernel API might be possible in the long term but for now we have
to live with what we have right now.

After all, adding true unordered loading - I mean not just moving the
reassembly process from QEMU to the kernel but making the device itself
accept buffers out out order - will likely be pretty complex (requiring
adding such functionality to the device firmware, etc).


I would expect the device will need to be able to provision the device
states so it became smaller objects rather than one binary object, then
either tag-able or address-able on those objects.




And even with that trick, it'll still need to be serialized on the read()
syscall so it won't scale either if the state is huge.  For that issue
there's no workaround we can do from userspace.


The read() calls for multiple VFIO devices can be issued in parallel,
and in fact they are in my patch set.


I was talking about concurrency for one device.


AFAIK with the current hardware the read speed is limited by the device
itself, so adding additional reading threads wouldn't help.

Once someone has the hardware which is limited by single reading thread
that person can add the necessary kernel API (including unordered
loading) and then extend QEMU with such support.



(..)

4. Risk of OOM on unlimited VFIO buffering
==========================================

This follows with above bullet, but my pure question to ask here is how
does VFIO guarantees no OOM condition by buffering VFIO state?

I mean, currently your proposal used vfio_load_bufs_thread() as a separate
thread to only load the vfio states until sequential data is received,
however is there an upper limit of how much buffering it could do?  IOW:

vfio_load_state_buffer():

     if (packet->idx >= migration->load_bufs->len) {
         g_array_set_size(migration->load_bufs, packet->idx + 1);
     }

     lb = &g_array_index(migration->load_bufs, typeof(*lb), packet->idx);
     ...
     lb->data = g_memdup2(&packet->data, data_size - sizeof(*packet));
     lb->len = data_size - sizeof(*packet);
     lb->is_present = true;

What if garray keeps growing with lb->data allocated, which triggers the
memcg limit of the process (if QEMU is in such process)?  Or just deplete
host memory and causing OOM kill.

I think we may need to find a way to throttle max memory usage of such
buffering.

So far this will be more of a problem indeed if this will be done during
VFIO iteration phases, but I still hope a solution can work with both
iteration phase and the switchover phase, even if you only do that in
switchover phase


Unfortunately, this issue will be hard to fix since the source can
legitimately send the very first buffer (chunk) of data as the last one
(at the very end of the transmission).

In this case, the target will need to buffer nearly the whole data.

We can't stop the receive on any channel, either, since the next missing
buffer can arrive at that channel.

However, I don't think purposely DoSing the target QEMU is a realistic
security concern in the typical live migration scenario.

I mean the source can easily force the target QEMU to exit just by
feeding it wrong migration data.

In case someone really wants to protect against the impact of
theoretically unbounded QEMU memory allocations during live migration
on the rest of the system they can put the target QEMU process
(temporally) into a memory-limited cgroup.


Note that I'm not worrying about DoS of a malicious src QEMU, and I'm
exactly talking about the generic case where QEMU (either src or dest, in
that case normally both) is put into the memcg and if QEMU uses too much
memory it'll literally get killed even if no DoS issue at all.

In short, we hopefully will have a design that will always work with QEMU
running in a container, without 0.5% chance dest qemu being killed, if you
see what I meant.

The upper bound of VFIO buffering will be needed so the admin can add that
on top of the memcg limit and as long as QEMU keeps its words it'll always
work without sudden death.

I think I have some idea about resolving this problem.  That idea can
further complicate the protocol a little bit.  But before that let's see
whether we can reach an initial consensus on this matter first, on whether
this is a sane request.  In short, we'll need to start to have a
configurable size to say how much VFIO can buffer, maybe per-device, or
globally.  Then based on that we need to have some logic guarantee that
over-mem won't happen, also without heavily affecting concurrency (e.g.,
single thread is definitely safe and without caching, but it can be
slower).


Here, I think I can add a per-device limit parameter on the number of
buffers received out-of-order or waiting to be loaded into the device -
with a reasonable default.


Yes that should work.

I don't even expect people would change that, but this might be the
information people will need to know before putting it into a container if
it's larger than how qemu dynamically consumes memories here and there.
I'd expect it is still small enough so nobody will notice it (maybe a few
tens of MBs? but just wildly guessing, where tens of MBs could fall into
the "noise" memory allocation window of a VM).


The single buffer size is 8 MiB so I think the safe default should be
allowing 2 times the number of multifd channels.

With 5 multifd channels that's 10 buffers * 8 MiB = 80 MiB worst
case buffering per device.

But this will need to be determined experimentally once such parameter
is added to be sure it's enough.



(..)

5. Worker thread model
======================

I'm so far not happy with what this proposal suggests on creating the
threads, also the two new hooks mostly just to create these threads..


That VFIO .save_live_complete_precopy_begin handler crates a new
per-device thread is an implementation detail for this particular
driver.

The whole idea behind this and save_live_complete_precopy_end hook was
that details how the particular device driver does its own async saving
is abstracted away from the migration core.

The device then can do what's best / most efficient for it to do.


Yes, and what I was thinking is whether it does it in form of "enqueue a
task to migration worker threads", rather than "creating its own threads in
the device hooks, and managing those threads alone".

It's all about whether such threading can be reused by non-VFIO devices.
They can't be reused if VFIO is in charge here, and it will make migration
less generic.

My current opinion is they can and should be re-usable. Consider if someone
starts to teach multifd carry non-vfio data (e.g. a generic VMSD), then we
can enqueue a task, do e.g. ioctl(KVM_GET_REGS) in those threads (rather
than VFIO read()).


Theoretically, it's obviously possible to wrap every operation in a request
to some thread pool.


But that would bring a lot of complexity, since instead of performing these
operation directly now the requester will need to:
1) Prepare some "Operation" structure with the parameters of the requested
operation (task).
In your case this could be QEMU_OP_GET_VCPU_REGS operation using
"OperationGetVCPURegs" struct containing vCPU number parameter = 1.


Why such complexity is needed?


I just gave an example how implementing running a individual task like
"ioctl(KVM_GET_REGS)" (that you suggested above) in such thread pool would
look like.

Can it be as simple as func(opaque) to be queued, then here
func==vfio_save_complete_precopy_async_thread, opaque=VFIODevice*?


That would be possible, although in both implementations of:
1) adding a new thread pool type and wrapping device reading thread
creation around such pool, OR:
2) a direct qemu_thread_create() call.
the number of threads actually created would be the same.

That's unless someone sets the multifd channel count below the number
of VFIO devices - but one might argue that's not really a configuration
where good performance is expected anyway.



2) Submit this operation to the thread pool and wait for it to complete,


VFIO doesn't need to have its own code waiting.  If this pool is for
migration purpose in general, qemu migration framework will need to wait at
some point for all jobs to finish before moving on.  Perhaps it should be
at the end of the non-iterative session.


So essentially, instead of calling save_live_complete_precopy_end handlers
from the migration code you would like to hard-code its current VFIO
implementation of calling 
vfio_save_complete_precopy_async_thread_thread_terminate().

Only it wouldn't be then called VFIO precopy async thread terminate but some
generic device state async precopy thread terminate function.



3) Thread pool needs to check whether it has any free threads in the pool
available to perform this operation.

If not, and the count of threads that are CPU-bound (~aren't sleeping on
some I/O operation) is less than the number of logical CPUs in the system
the thread pool needs to spawn a new thread since there's some CPU capacity
available,


For this one it can follow what thread-pool.c is doing, and the upper bound
of n-threads can start from simple, e.g. min(n_channels_multifd, 8)?


It needs to be min(n_channels_multifd, n_device_state_devices), because
with 9 such devices and 9 multifd channels we need at least 9 threads.



4) The operation needs to be dispatched to the actual execution thread,

5) The execution thread needs to figure out which operation it needs to
actually do, fetch the necessary parameters from the proper "Operation"
structure, maybe take the necessary locks, before it can actually perform
the requested operation,

6) The execution thread needs to serialize (write) the operation result
back into some "OperationResult" structure, like "OperationGetVCPURegsResult",


I think in this simplest case, the thread should simply run fn(opaque), in
which it should start to call multifd_queue_device_state() and queue
multifd jobs from the worker thread instead of the vfio dedicated threads.
I don't yet expect much to change in your code from that regard inside what
vfio_save_complete_precopy_async_thread() used to do.



7) The execution thread needs to submit this result back to the requester,

8) The thread pool needs to decide whether to keep this (now idle) execution
thread in the pool as a reserve thread or terminate it immediately,

9) The requester needs to be resumed somehow (returned from wait) now that
the operation it requested is complete,

10) The requester needs the fetch the operation results from the proper
"OperationResult" structure and decode them accordingly.


As you can see, that's *a lot* of extra code that needs to be maintained
for just a single operation type.


I don't yet know why you designed it so complicated, but if I missed
something above please let me know.


I explained above that's how running your example of "ioctl(KVM_GET_REGS)"
in such thread pool would look like.
(It wasn't a proposal to be actually implemented to be clear)






I know I suggested that.. but that's comparing to what I read in the even
earlier version, and sorry I wasn't able to suggest something better at
that time because I simply thought less.

As I mentioned in the other reply elsewhere, I think we should firstly have
these threads ready to take data at the start of migration, so that it'll
work when someone wants to add vfio iteration support.  Then the jobs
(mostly what vfio_save_complete_precopy_async_thread() does now) can be
enqueued into the thread pools.


I'm not sure that we can get way with using fewer threads than devices
as these devices might not support AIO reads from their migration file
descriptor.


It doesn't need to use AIO reads - I'll be happy if the thread model can be
generic, VFIO can still enqueue a task that does blocking reads.

It can take a lot of time, but it's fine: others who like to enqueue too
and see all threads busy, they should simply block there and waiting for
the worker threads to be freed again.  It's the same when there's no
migration worker threads as it means the read() will block the main
migration thread.


Oh no, waiting for another device blocking read to complete before
scheduling another device blocking read is surely going to negatively
impact the performance.


There can be e.g. 8 worker threads.  If you want you can make sure the
worker threads are at least more than vfio threads.  Then it will guarantee
vfio will dump / save() one device per thread concurrently.


Yes, I wrote this requirement above as
n_threads = min(n_channels_multifd, n_device_state_devices).



For best performance we need to maximize parallelism - that means
reading (and loading) all the VFIO devices present in parallel.

The whole point of having per-device threads is for the whole operation
to be I/O bound but never CPU bound on a reasonably fast machine - and
especially not number-of-threads-in-pool bound.


Now we can have multiple worker threads doing things concurrently if
possible (some of them may not, especially when BQL will be required, but
that's a separate thing, and many device save()s may not need BQL, and when
it needs we can take it in the enqueued tasks).



mlx5 devices, for example, seems to support only poll()ed / non-blocking
reads at best - with unknown performance in comparison with issuing
blocking reads from dedicated threads.

On the other hand, handling a single device from multiple threads in
parallel is generally not possible due to difficulty of establishing in
which order the buffers were read.

And if we need a per-VFIO device thread anyway then using a thread pool
doesn't help much - but brings extra complexity.

In terms of starting the loading thread earlier to load also VM live
phase data it looks like a small change to the code so it shouldn't be
a problem.


That's good to know.  Please still consider a generic thread model and see
what that would also work for your VFIO use case.

If you see what thread-pool.c did right now is it'll dynamically create
threads on the fly.  I think that's something we can do too but just apply
an upper limit to the thread numbers.


We have an upper limit on the count of saving threads already - it's the
count of VFIO devices in the VM.

The API in util/thread-pool.c is very basic and basically only allows
submitting either AIO operations or generic function call operation
but still within some AioContext.


What I'm saying is a thread pool _without_ aio.  I think it might be called
ThreadPoolRaw and let ThreadPool depend on it, but I didn't further check yet.


So it's not using an existing thread pool implementation from util/thread-pool.c
but essentially creating a new one - with probably some code commonality
with the existing AIO one.

That's possible but since util/thread-pool.c AFAIK isn't owned by the
migration subsystem such new implementation will probably need also review by
other QEMU maintainers.



There's almost none of the operation execution logic I described above -
all of these would need to be written and maintained.




It's better to create the thread pool owned by migration, rather than
threads owned by VFIO, because it also paves way for non-VFIO device state
save()s, as I mentioned also above on the multifd packet header.  Maybe we
can have a flag in the packet header saying "this is device xxx's state,
just load it".


I think the same could be done by simply implementing these hooks in other
device types than VFIO, right?

And if we notice that these implementations share a bit of code then we
can think about making a common helper library out of this code.

After, all that's just an implementation detail that does not impact
the underlying bit stream protocol.


You're correct.

However, it still affects a few things.

Firstly, it may mean that we may not even need those two extra vmstate
hooks: the enqueue can happen already with save_state() if the migration
worker model exists.

So instead of this:

          vfio_save_state():
          if (migration->multifd_transfer) {
                  /* Emit dummy NOP data */
                  qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
                  return;
          }

We can already do:

          if (migration->multifd_transfer) {
                  // enqueue task to load state for this vfio device
                  ...
                  return;
          }

IMHO it'll be much cleaner in VFIO code, and much cleaner too for migration
code.


The save_state hook is executed too late - only after all iterable
hooks have already transferred all their data.

We want to start saving this device state as early as possible to not
have to wait for any other device to transfer its data first.

That's why the code introduces save_live_complete_precopy_begin hook
that's guaranteed to be the very first hook called during switchover
phase device state saving.


I think I mis-typed.. What I wanted to say is vfio_save_complete_precopy(),
not vfio_save_state().

There will be one challenge though where RAM is also an iterable, so RAM's
save_live_complete_precopy() can delay VFIO's, even if it simply only need
to enqueue a job.

Two solutions I can think of:

   (1) Provide a separate hook, e.g. save_live_complete_precopy_async(),
   when save_live_complete_precopy_async(opaque) is provided, instead of
   calling save_live_complete_precopy(), we inject that job into the worker
   threads.  In that case we can loop over *_precopy_async() before all the
   rest *_precopy() calls.


That's basically the approach the current patch set is using, just not using
pool worker threads (yet).

Only the hook was renamed from save_live_complete_precopy_async to
save_live_complete_precopy_begin upon your comment on RFC requesting that.


   (2) Make RAM's save_live_complete_precopy() also does similar enqueue
   when multifd enabled, so RAM will be saved in the worker thread too.

However (2) can have other issues to work out.  Do you think (1) is still
doable?



Yes, I think (1) is the correct way to do it.




Another (possibly personal) reason is, I will not dare to touch VFIO code
too much to do such a refactoring later.  I simply don't have the VFIO
devices around and I won't be able to test.  So comparing to other things,
I hope VFIO stuff can land more stable than others because I am not
confident at least myself to clean it.


That's a fair request, will keep this on mind.


I simply also don't like random threads floating around, considering that
how we already have slightly a mess with migration on other reasons (we can
still have random TLS threads floating around, I think... and they can
cause very hard to debug issues). I feel shaky to maintain it when any
device can also start to create whatever threads they can during migration.


The threads themselves aren't very expensive as long as their number
is kept within reasonable bounds.

4 additional threads (present only during active migration operation)
with 4 VFIO devices is really not a lot.


It's not about number, it's about management, and when something crashed at
some unwanted point, then we may want to know what happened to those
threads and how to recycle them.


I guess if you are more comfortable with maintaining code written in such
way then that's some argument for it too.



Thanks,



Thanks,
Maciej
reply via email to
[Prev in Thread] Current Thread [Next in Thread]