| Message ID | cover.1718717584.git.maciej.szmigiero@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Multifd | expand |
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. https://lore.kernel.org/r/20240620212111.29319-1-farosas@suse.de 1. Multifd device state support =============================== As Fabiano suggested in his RFC post, we may need one more layer of abstraction to represent VFIO's demand on allowing multifd to send arbitrary buffer to the wire. This can be more than "how to pass the device state buffer to the sender threads". So far, MultiFDMethods is only about RAM. If you pull the latest master branch Fabiano just merged yet two more RAM compressors that are extended on top of MultiFDMethods model. However still they're all about RAM. I think it's better to keep it this way, so maybe MultiFDMethods should some day be called MultiFDRamMethods. multifd_send_fill_packet() may only be suitable for RAM buffers, not adhoc buffers like what VFIO is using. multifd_send_zero_page_detect() may not be needed either for arbitrary buffers. Most of those are still page-based. I think it also means we shouldn't call ->send_prepare() when multifd send thread notices that it's going to send a VFIO buffer. So it should look like this: int type = multifd_payload_type(p->data); if (type == MULTIFD_PAYLOAD_RAM) { multifd_send_state->ops->send_prepare(p, &local_err); } else { // VFIO buffers should belong here assert(type == MULTIFD_PAYLOAD_DEVICE_STATE); ... } It also means it shouldn't contain code like: nocomp_send_prepare(): if (p->is_device_state_job) { return nocomp_send_prepare_device_state(p, errp); } else { return nocomp_send_prepare_ram(p, errp); } nocomp should only exist in RAM world, not VFIO's. And it looks like you agree with Fabiano's RFC proposal, please work on top of that to provide that layer. Please make sure it outputs the minimum in "$ git grep device_state migration/multifd.c" when you work on the new version. Currently: $ git grep device_state migration/multifd.c | wc -l 59 The hope is zero, or at least a minimum with good reasons. 2. Frequent mallocs/frees ========================= Fabiano's series can also help to address some of these, but it looks like this series used malloc/free more than the opaque data buffer. This is not required to get things merged, but it'll be nice to avoid those if possible. 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. Now in your series IIUC you resolved that by using vfio_load_bufs_thread(), holding off the load process but only until sequential buffers are received. I think that causes one issue that I'll mention below as a separate topic. But besides that, my point is, this is not the reason that you need to introduce VFIODeviceStatePacket, load_state_buffer() and so on. My understanding is that we do need one way to re-serialize the buffers, but it doesn't need load_state_buffer(), instead it can call vmstate_load() in order, properly invoke vfio_load_state() with the right buffers. It'll just be nice if VFIO can keep its "load state" logic at one place. One benefit of that is with such a more generic framework, QEMU can easily extend this infra to other device states, so that logically we can consider sending non-VFIO device states also in the multifd buffers. However with your current solution, new structures are needed, new hooks, a lot of new codes around, however less problems it solved.. That's not optimal. 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 (and I don't know why you don't care about VFIO iteration phase, if you cared enough on how VFIO works now with migration.. literally that should help VFIO migrates faster on 25G+ networks, with/without a shorter blackout window). 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.. 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. 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'd start looking at util/thread-pool.c, removing all the AIO implications but simply provide a raw thread pool for what thread_pool_submit() is doing. I know this is a lot, but I really think this is the right thing.. but we can discuss, and you can correct me on my mistakes if there is. If you want I can have a look at this pool model and prepare a patch, so you can work on other vfio relevant stuff and pick that up, if that helps you trying to reach the goal of landing this whole stuff in 9.1. But I hope I explained more or less in this email on why I think this feature is more involved than it looks like, and not yet mature from design. And I hope I'm not purly asking for too much: merging this VFIO series first then refactor on top can mean dropping too much unneeded code after adding them, not to mention the protocol going to need another break. It just doesn't sound like the right thing to do. Thanks,
Hi Peter, 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. > > 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. > 1. Multifd device state support > =============================== > > As Fabiano suggested in his RFC post, we may need one more layer of > abstraction to represent VFIO's demand on allowing multifd to send > arbitrary buffer to the wire. This can be more than "how to pass the > device state buffer to the sender threads". > > So far, MultiFDMethods is only about RAM. If you pull the latest master > branch Fabiano just merged yet two more RAM compressors that are extended > on top of MultiFDMethods model. However still they're all about RAM. I > think it's better to keep it this way, so maybe MultiFDMethods should some > day be called MultiFDRamMethods. > > multifd_send_fill_packet() may only be suitable for RAM buffers, not adhoc > buffers like what VFIO is using. multifd_send_zero_page_detect() may not be > needed either for arbitrary buffers. Most of those are still page-based. > > I think it also means we shouldn't call ->send_prepare() when multifd send > thread notices that it's going to send a VFIO buffer. So it should look > like this: > > int type = multifd_payload_type(p->data); > if (type == MULTIFD_PAYLOAD_RAM) { > multifd_send_state->ops->send_prepare(p, &local_err); > } else { > // VFIO buffers should belong here > assert(type == MULTIFD_PAYLOAD_DEVICE_STATE); > ... > } > > It also means it shouldn't contain code like: > > nocomp_send_prepare(): > if (p->is_device_state_job) { > return nocomp_send_prepare_device_state(p, errp); > } else { > return nocomp_send_prepare_ram(p, errp); > } > > nocomp should only exist in RAM world, not VFIO's. > > And it looks like you agree with Fabiano's RFC proposal, please work on top > of that to provide that layer. Please make sure it outputs the minimum in > "$ git grep device_state migration/multifd.c" when you work on the new > version. Currently: > > $ git grep device_state migration/multifd.c | wc -l > 59 > > The hope is zero, or at least a minimum with good reasons. I guess you mean "grep -i" in the above example, since otherwise the above command will find only lowercase "device_state". On the other hand, your example code above has uppercase "DEVICE_STATE", suggesting that it might be okay? Overall, using Fabiano's patch set as a base for mine makes sense to me. > 2. Frequent mallocs/frees > ========================= > > Fabiano's series can also help to address some of these, but it looks like > this series used malloc/free more than the opaque data buffer. This is not > required to get things merged, but it'll be nice to avoid those if possible. Ack - as long as its not making the code messy/fragile, of course. > 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. > Now in your series IIUC you resolved that by using vfio_load_bufs_thread(), > holding off the load process but only until sequential buffers are > received. I think that causes one issue that I'll mention below as a > separate topic. But besides that, my point is, this is not the reason that > you need to introduce VFIODeviceStatePacket, load_state_buffer() and so on. > My understanding is that we do need one way to re-serialize the buffers, > but it doesn't need load_state_buffer(), instead it can call vmstate_load() > in order, properly invoke vfio_load_state() with the right buffers. It'll > just be nice if VFIO can keep its "load state" logic at one place. Re-using the .load_state hook for multifd device state date has a few additional issues: * This hook accepts a QEMUFile parameter, not a buffer. * Due to the above, it (and the functions it calls) expects being able to read all the required data in one go. In other words, there's no way for this hook to suspend its execution and return because the next piece of data it wants hasn't arrived yet. Specifically, this hook is only able to exit at VFIO_MIG_FLAG_END_OF_STATE boundaries in the incoming stream. * The hook is expected to be called from the main migration thread, and so calls to it are expected to be effectively serialized. It can also safely call core QEMU functions, like it does from vfio_load_device_config_state() -> vfio_pci_load_config() -> vmstate_load_state(). This actually fails when called from any other thread (in some memory region modification function, as far as I remember). In contrast to that, .load_state_buffer hook is prepared to deal with getting calls from multiple multifd receive threads. > One benefit of that is with such a more generic framework, QEMU can easily > extend this infra to other device states, so that logically we can consider > sending non-VFIO device states also in the multifd buffers. However with > your current solution, new structures are needed, new hooks, a lot of new > codes around, however less problems it solved.. That's not optimal. This all relies on a promise than the current .load_state hooks can be efficiently called from multiple multifd receive threads, which isn't true today. Due to the reasons I specified above I think modifying these existing hooks would be more complex than just introducing a new one with the proper semantics. > 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. > (and I don't know why you don't care about VFIO iteration > phase, if you cared enough on how VFIO works now with migration.. literally > that should help VFIO migrates faster on 25G+ networks, with/without a > shorter blackout window). I do care about the VM live phase, too. Just to keep the complexity in bounds for the first version I wanted to deal with the most pressing issue first - downtime. I am not against accommodating the VM live phase changes if they don't significantly expand the patch set size. > 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. > 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. 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. > 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. > I'd start looking at util/thread-pool.c, removing all the AIO implications > but simply provide a raw thread pool for what thread_pool_submit() is > doing. > > I know this is a lot, but I really think this is the right thing.. but we > can discuss, and you can correct me on my mistakes if there is. > > If you want I can have a look at this pool model and prepare a patch, so > you can work on other vfio relevant stuff and pick that up, if that helps > you trying to reach the goal of landing this whole stuff in 9.1. I would certainly help with the amount of work (thanks for the offer!), but as I wrote above I am not really convinced that adapting the existing thread pool utils for this usage is really the way to go here. > But I hope I explained more or less in this email on why I think this > feature is more involved than it looks like, and not yet mature from > design. And I hope I'm not purly asking for too much: merging this VFIO > series first then refactor on top can mean dropping too much unneeded code > after adding them, not to mention the protocol going to need another break. > > It just doesn't sound like the right thing to do. > > Thanks, > Thanks, Maciej
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. > > > > 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. > > > 1. Multifd device state support > > =============================== > > > > As Fabiano suggested in his RFC post, we may need one more layer of > > abstraction to represent VFIO's demand on allowing multifd to send > > arbitrary buffer to the wire. This can be more than "how to pass the > > device state buffer to the sender threads". > > > > So far, MultiFDMethods is only about RAM. If you pull the latest master > > branch Fabiano just merged yet two more RAM compressors that are extended > > on top of MultiFDMethods model. However still they're all about RAM. I > > think it's better to keep it this way, so maybe MultiFDMethods should some > > day be called MultiFDRamMethods. > > > > multifd_send_fill_packet() may only be suitable for RAM buffers, not adhoc > > buffers like what VFIO is using. multifd_send_zero_page_detect() may not be > > needed either for arbitrary buffers. Most of those are still page-based. > > > > I think it also means we shouldn't call ->send_prepare() when multifd send > > thread notices that it's going to send a VFIO buffer. So it should look > > like this: > > > > int type = multifd_payload_type(p->data); > > if (type == MULTIFD_PAYLOAD_RAM) { > > multifd_send_state->ops->send_prepare(p, &local_err); > > } else { > > // VFIO buffers should belong here > > assert(type == MULTIFD_PAYLOAD_DEVICE_STATE); > > ... > > } > > > > It also means it shouldn't contain code like: > > > > nocomp_send_prepare(): > > if (p->is_device_state_job) { > > return nocomp_send_prepare_device_state(p, errp); > > } else { > > return nocomp_send_prepare_ram(p, errp); > > } > > > > nocomp should only exist in RAM world, not VFIO's. > > > > And it looks like you agree with Fabiano's RFC proposal, please work on top > > of that to provide that layer. Please make sure it outputs the minimum in > > "$ git grep device_state migration/multifd.c" when you work on the new > > version. Currently: > > > > $ git grep device_state migration/multifd.c | wc -l > > 59 > > > > The hope is zero, or at least a minimum with good reasons. > > I guess you mean "grep -i" in the above example, since otherwise > the above command will find only lowercase "device_state". > > On the other hand, your example code above has uppercase > "DEVICE_STATE", suggesting that it might be okay? Yes that's definitely ok. I could be over-cautious when I used the grep example, but I hope you get my point where we should remove that device_state pointer, rather than as generic as supporting multifd to take device state buffers. Especially, if that idea will also apply on top of non-VFIO devices, e.g. when we extend that to a normal VMSD buffer to be delivered too using that same mechanism, then I think it's even okay to have some "device_state" there. Personally I would still think those as justified usages that are generic enough; after all I don't expect multifd to send other things besides RAM and generic terms of device states. > > Overall, using Fabiano's patch set as a base for mine makes sense to me. > > > 2. Frequent mallocs/frees > > ========================= > > > > Fabiano's series can also help to address some of these, but it looks like > > this series used malloc/free more than the opaque data buffer. This is not > > required to get things merged, but it'll be nice to avoid those if possible. > > Ack - as long as its not making the code messy/fragile, of course. > > > 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. 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. > > > Now in your series IIUC you resolved that by using vfio_load_bufs_thread(), > > holding off the load process but only until sequential buffers are > > received. I think that causes one issue that I'll mention below as a > > separate topic. But besides that, my point is, this is not the reason that > > you need to introduce VFIODeviceStatePacket, load_state_buffer() and so on. > > My understanding is that we do need one way to re-serialize the buffers, > > but it doesn't need load_state_buffer(), instead it can call vmstate_load() > > in order, properly invoke vfio_load_state() with the right buffers. It'll > > just be nice if VFIO can keep its "load state" logic at one place. > > Re-using the .load_state hook for multifd device state date has a few > additional issues: > * This hook accepts a QEMUFile parameter, not a buffer. > > * Due to the above, it (and the functions it calls) expects being able to > read all the required data in one go. > > In other words, there's no way for this hook to suspend its execution and > return because the next piece of data it wants hasn't arrived yet. > > Specifically, this hook is only able to exit at VFIO_MIG_FLAG_END_OF_STATE > boundaries in the incoming stream. > > * The hook is expected to be called from the main migration thread, and > so calls to it are expected to be effectively serialized. > > It can also safely call core QEMU functions, like it does from > vfio_load_device_config_state() -> vfio_pci_load_config() -> vmstate_load_state(). > This actually fails when called from any other thread (in some memory region > modification function, as far as I remember). > > In contrast to that, .load_state_buffer hook is prepared to deal with getting > calls from multiple multifd receive threads. Ah yes, I forgot we're still using QEMUFiles to load states... that's a pity, and that makes sense. Also, when I read this again I noticed indeed any channel/file based approach won't work for VFIO at least, due to the fact that it needs to do caching for out-of-order buffers issue. Then when I looked closer, it's a pity that "next_packet_size" field is not after "flags"; that should really be part of your new MultiFDPacketHdr_t, but indeed we won't achieve that without breaking current protocol. When at it, if you want maybe you can also start to rename MultiFDPacket_t to MultiFDRAMPacket_t already. Now I think I agree with you: using a buffer as a generic concept to take device states seems like a good idea, and we can start doing that from VFIO. Please keep that in mind that in all these paths it should be generic enough for non-VFIO to do so. From that POV I think your series did a good job already indeed from this perspective, so now I think I'm ok with it. There's another slight unfortunate where MultiFDPacketDeviceState_t will always need to send the idstr[].. I think it is a good start, though, as generic VMSD migrations will also need that (QEMU_VM_SECTION_FULL) so it's not super efficient but generic enough. You might want to look at how current migration tackles that with load_section_id field, but that can be for later, just fyi. I wonder how find_se() could impact perf when there're lots of devices, I never really measured it. > > > One benefit of that is with such a more generic framework, QEMU can easily > > extend this infra to other device states, so that logically we can consider > > sending non-VFIO device states also in the multifd buffers. However with > > your current solution, new structures are needed, new hooks, a lot of new > > codes around, however less problems it solved.. That's not optimal. > > This all relies on a promise than the current .load_state hooks can be > efficiently called from multiple multifd receive threads, which isn't true > today. > > Due to the reasons I specified above I think modifying these existing > hooks would be more complex than just introducing a new one with the > proper semantics. > > > 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). > > > (and I don't know why you don't care about VFIO iteration > > phase, if you cared enough on how VFIO works now with migration.. literally > > that should help VFIO migrates faster on 25G+ networks, with/without a > > shorter blackout window). > > I do care about the VM live phase, too. > > Just to keep the complexity in bounds for the first version I wanted to > deal with the most pressing issue first - downtime. > > I am not against accommodating the VM live phase changes if they don't > significantly expand the patch set size. Makes sense. Again, as long as the qemu (with this series applied) can also take iterative vfio datas in the newer QEMUs if that'll come, I'll be perfectly happy with that. Or one step back, if that can't be achieved, I hope we figure out the complexity and justify that, rather than completely ignoring the iteration phase, then that'll be also good enough to me. > > 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()). > > > 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. 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. > > > 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. 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. 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. > > > I'd start looking at util/thread-pool.c, removing all the AIO implications > > but simply provide a raw thread pool for what thread_pool_submit() is > > doing. > > > > I know this is a lot, but I really think this is the right thing.. but we > > can discuss, and you can correct me on my mistakes if there is. > > > > If you want I can have a look at this pool model and prepare a patch, so > > you can work on other vfio relevant stuff and pick that up, if that helps > > you trying to reach the goal of landing this whole stuff in 9.1. > > I would certainly help with the amount of work (thanks for the offer!), > but as I wrote above I am not really convinced that adapting the existing > thread pool utils for this usage is really the way to go here. Yes let's figure that out first on the uncertainties, the plan is for later. Thanks,
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. >>> >>> 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). > 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. (..) >>> 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. (..) >>> 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. 2) Submit this operation to the thread pool and wait for it to complete, 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, 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", 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 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. 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. 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. > 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. (..) > > Thanks, > Thanks, Maciej
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. > > > > > > > > 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. > > (..) > > > > 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). > > (..) > > > > 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? Can it be as simple as func(opaque) to be queued, then here func==vfio_save_complete_precopy_async_thread, opaque=VFIODevice*? > > 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. > > 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)? > > 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 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. > > 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. > > 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. (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? > > > 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. Thanks,
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. >>>>> >>>>> 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
On Wed, Jun 26, 2024 at 05:47:34PM +0200, Maciej S. Szmigiero wrote: > 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. > > > > > > > > > > > > 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. OK. > > 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. Yes you may want to test it with a new logic to be able to throttle sending on src qemu (because otherwise when dest qemu is very unlucky its buffer is full but still the initial index chunk is missing), then making sure it's relatively small but hopefully still keep the flow running as much as possible. > > > > > > > (..) > > > > > > 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. Again, it's not about the number of threads that I worry. If you create one thread but hard to manage it's the same. OTOH if it's a common model I think it's fine if you create 16 or 32, especially if when most of them are either idle or doing block IOs, then they'll be put to sleep anyway. That's not a concern at all. > > 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. I don't understand what did you mean by "hard code". What I was saying is if we target the worker thread pool to be used for "concurrently dump vmstates", then it'll make sense to make sure all the jobs there were flushed after qemu dumps all non-iterables (because this should be the last step of the switchover). I expect it looks like this: while (pool->active_threads) { qemu_sem_wait(&pool->job_done); } > > > > > > > 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. Yes, that's how we normally should do it. Obviously you still want to push that in 9.1, so if you want you can create that pool implementation under migration/, and we can try to move it over as a future rework, having block people review that later. > > > > > > > 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. I don't think "correct" is the correct word to put it.. it's really a matter of whether you want to push this earlier in-tree. The 2nd proposal will be more than correct to me, IMHO. That'll be really helpful too also to VFIO when RAM can be saved concurrently, then it means these things can be done all concurrently: - VFIO, one thread per one device - RAM, one thread - non-iterables Otherwise 2+3 needs to be serialized. If you're looking for downtime optimizations that may also relevant, afaiu. And that's also one of the major points why I want to convince you not to use a separate vfio thread, because AFAICT we simply have other users. > > > > > > > > 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. It's not about my flavour of maintenance. We used to work on issues where we see a dangling thread operate on migration objects even if it was created in the _previous_ migration, cancelled and retried. And the thread doesn't know that. It was kind of leaked and it causes issues hard to debug. VFIO can cause similar thing if it can create some threads that migration developers may overlook and not easy to manage. Then it'll be the same challenge when a vfio thread dangled for some reason and it'll just make things harder to debug when issue happens. I want to make sure if ever possible migration framework manages threads on its own, so no thread will be fiddling around without being noticed. Not to mention as I mentioned previously, that "having some async model to dump vmstate" isn't something special to VFIO, it can easily be extended to either RAM, or other normal VMSDs if we can tackle other issues here and there. The general request is the same. It'll be a chaos if vfio starts to create its own threads, then vDPA and others. It is much saner to make it a generic model to me. Thanks,
On 26.06.2024 18:23, Peter Xu wrote: > On Wed, Jun 26, 2024 at 05:47:34PM +0200, Maciej S. Szmigiero wrote: >> 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. >>>>>>> >>>>>>> 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. >>>>>> (..) >>>>>>> 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. > > Yes you may want to test it with a new logic to be able to throttle sending > on src qemu (because otherwise when dest qemu is very unlucky its buffer is > full but still the initial index chunk is missing), then making sure it's > relatively small but hopefully still keep the flow running as much as > possible. > >> >>>> >>>> (..) >>>>>>> 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. > > Again, it's not about the number of threads that I worry. > > If you create one thread but hard to manage it's the same. > > OTOH if it's a common model I think it's fine if you create 16 or 32, > especially if when most of them are either idle or doing block IOs, then > they'll be put to sleep anyway. That's not a concern at all. > >> >> 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. > > I don't understand what did you mean by "hard code". "Hard code" wasn't maybe the best expression here. I meant the move of the functionality that's provided by vfio_save_complete_precopy_async_thread_thread_terminate() in this patch set to the common migration code. > What I was saying is if we target the worker thread pool to be used for > "concurrently dump vmstates", then it'll make sense to make sure all the > jobs there were flushed after qemu dumps all non-iterables (because this > should be the last step of the switchover). > > I expect it looks like this: > > while (pool->active_threads) { > qemu_sem_wait(&pool->job_done); > } > >> >>>> >>>> 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. > > Yes, that's how we normally should do it. Obviously you still want to push > that in 9.1, so if you want you can create that pool implementation under > migration/, and we can try to move it over as a future rework, having block > people review that later. I think that with this thread pool introduction we'll unfortunately almost certainly need to target this patch set at 9.2, since these overall changes (and Fabiano patches too) will need good testing, might uncover some performance regressions (for example related to the number of buffers limit or Fabiano multifd changes), bring some review comments from other people, etc. In addition to that, we are in the middle of holiday season and a lot of people aren't available - like Fabiano said he will be available only in a few weeks. >> >>>> >>>> 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. > > I don't think "correct" is the correct word to put it.. it's really a > matter of whether you want to push this earlier in-tree. > > The 2nd proposal will be more than correct to me, IMHO. That'll be really > helpful too also to VFIO when RAM can be saved concurrently, then it means > these things can be done all concurrently: > > - VFIO, one thread per one device > - RAM, one thread > - non-iterables > > Otherwise 2+3 needs to be serialized. > > If you're looking for downtime optimizations that may also relevant, afaiu. Having RAM sent in parallel with non-iterables would make sense to me, but I am not 100% sure this is a safe thing to do - after all, currently non-iterables can rely on the whole RAM being already transferred. Currently, it seems that only RAM, VFIO, block-dirty-bitmap and some s390x + ppc specific stuff implements .save_live_complete_precopy hooks. While I am not really concerned about s390x and ppc we'd need to make sure that any data transferred via these hooks is transferred asynchronously, to not delay starting the VFIO transmission. Anyway, that's probably not for this patch set, since if we start widening its scope beyond the basic device state transfer framework + VFIO we risk missing 9.2 too. > And that's also one of the major points why I want to convince you not to > use a separate vfio thread, because AFAICT we simply have other users. > >> >>>> >>>>> 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. > > It's not about my flavour of maintenance. > > We used to work on issues where we see a dangling thread operate on > migration objects even if it was created in the _previous_ migration, > cancelled and retried. And the thread doesn't know that. It was kind of > leaked and it causes issues hard to debug. > > VFIO can cause similar thing if it can create some threads that migration > developers may overlook and not easy to manage. Then it'll be the same > challenge when a vfio thread dangled for some reason and it'll just make > things harder to debug when issue happens. > > I want to make sure if ever possible migration framework manages threads on > its own, so no thread will be fiddling around without being noticed. > > Not to mention as I mentioned previously, that "having some async model to > dump vmstate" isn't something special to VFIO, it can easily be extended to > either RAM, or other normal VMSDs if we can tackle other issues here and > there. The general request is the same. It'll be a chaos if vfio starts > to create its own threads, then vDPA and others. It is much saner to make > it a generic model to me. I more or less know now how the v2 of this patch set needs to look like (at least architecturally). Will try to prepare something in the coming weeks. > Thanks, > Thanks, Maciej
On Thu, Jun 27, 2024 at 11:14:28AM +0200, Maciej S. Szmigiero wrote: > On 26.06.2024 18:23, Peter Xu wrote: > > On Wed, Jun 26, 2024 at 05:47:34PM +0200, Maciej S. Szmigiero wrote: > > > 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. > > > > > > > > > > > > > > > > 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. > > > > > > > > (..) > > > > > > > > 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. > > > > Yes you may want to test it with a new logic to be able to throttle sending > > on src qemu (because otherwise when dest qemu is very unlucky its buffer is > > full but still the initial index chunk is missing), then making sure it's > > relatively small but hopefully still keep the flow running as much as > > possible. > > > > > > > > > > > > > > > (..) > > > > > > > > 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. > > > > Again, it's not about the number of threads that I worry. > > > > If you create one thread but hard to manage it's the same. > > > > OTOH if it's a common model I think it's fine if you create 16 or 32, > > especially if when most of them are either idle or doing block IOs, then > > they'll be put to sleep anyway. That's not a concern at all. > > > > > > > > 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. > > > > I don't understand what did you mean by "hard code". > > "Hard code" wasn't maybe the best expression here. > > I meant the move of the functionality that's provided by > vfio_save_complete_precopy_async_thread_thread_terminate() in this patch set > to the common migration code. I see. That function only does a thread_join() so far. So can I understand it as below [1] should work for us, and it'll be clean too (with nothing to hard-code)? The time to join() the worker threads can be even later, until migrate_fd_cleanup() on sender side. You may have a better idea on when would be the best place to do it when start working on it. > > > What I was saying is if we target the worker thread pool to be used for > > "concurrently dump vmstates", then it'll make sense to make sure all the > > jobs there were flushed after qemu dumps all non-iterables (because this > > should be the last step of the switchover). > > > > I expect it looks like this: > > > > while (pool->active_threads) { > > qemu_sem_wait(&pool->job_done); > > } [1] > > > > > > > > > > > > > > > 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. > > > > Yes, that's how we normally should do it. Obviously you still want to push > > that in 9.1, so if you want you can create that pool implementation under > > migration/, and we can try to move it over as a future rework, having block > > people review that later. > > I think that with this thread pool introduction we'll unfortunately almost certainly > need to target this patch set at 9.2, since these overall changes (and Fabiano > patches too) will need good testing, might uncover some performance regressions > (for example related to the number of buffers limit or Fabiano multifd changes), > bring some review comments from other people, etc. > > In addition to that, we are in the middle of holiday season and a lot of people > aren't available - like Fabiano said he will be available only in a few weeks. Right, that's unfortunate. Let's see, but still I really hope we can also get some feedback from Fabiano before it lands, even with that we have chance for 9.1 but it's just challenging, it's the same condition I mentioned since the 1st email. And before Fabiano's back (he's the active maintainer for this release), I'm personally happy if you can propose something that can land earlier in this release partly. E.g., if you want we can at least upstream Fabiano's idea first, or some more on top. For that, also feel to have a look at my comment today: https://lore.kernel.org/r/Zn15y693g0AkDbYD@x1n Feel free to comment there too. There's a tiny uncertainty there so far on specifying "max size for a device state" if do what I suggested, as multifd setup will need to allocate an enum buffer suitable for both ram + device. But I think that's not an issue and you'll tackle that properly when working on it. It's more about whether you agree on what I said as a general concept. > > > > > > > > > > > > > > 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. > > > > I don't think "correct" is the correct word to put it.. it's really a > > matter of whether you want to push this earlier in-tree. > > > > The 2nd proposal will be more than correct to me, IMHO. That'll be really > > helpful too also to VFIO when RAM can be saved concurrently, then it means > > these things can be done all concurrently: > > > > - VFIO, one thread per one device > > - RAM, one thread > > - non-iterables > > > > Otherwise 2+3 needs to be serialized. > > > > If you're looking for downtime optimizations that may also relevant, afaiu. > > Having RAM sent in parallel with non-iterables would make sense to me, > but I am not 100% sure this is a safe thing to do - after all, currently > non-iterables can rely on the whole RAM being already transferred. > > Currently, it seems that only RAM, VFIO, block-dirty-bitmap and some > s390x + ppc specific stuff implements .save_live_complete_precopy hooks. > > While I am not really concerned about s390x and ppc we'd need to make > sure that any data transferred via these hooks is transferred asynchronously, > to not delay starting the VFIO transmission. > > Anyway, that's probably not for this patch set, since if we start widening > its scope beyond the basic device state transfer framework + VFIO we risk > missing 9.2 too. IMHO targetting that in 9.1 was simply too optimistic. Next time if you want to make sure it'll be in (or at least showing that is the goal), you should really start early to spin with non-rfc series, rather than waiting. Maybe that'll make the chance higher. This series, as the 1st one to introduce (1) device state migrations on multifd, and (2) async vmstate transfers, just can be involved as I mentioned, because we may want to do this first and do it right, paving way for others. > > > And that's also one of the major points why I want to convince you not to > > use a separate vfio thread, because AFAICT we simply have other users. > > > > > > > > > > > > > > > > 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. > > > > It's not about my flavour of maintenance. > > > > We used to work on issues where we see a dangling thread operate on > > migration objects even if it was created in the _previous_ migration, > > cancelled and retried. And the thread doesn't know that. It was kind of > > leaked and it causes issues hard to debug. > > > > VFIO can cause similar thing if it can create some threads that migration > > developers may overlook and not easy to manage. Then it'll be the same > > challenge when a vfio thread dangled for some reason and it'll just make > > things harder to debug when issue happens. > > > > I want to make sure if ever possible migration framework manages threads on > > its own, so no thread will be fiddling around without being noticed. > > > > Not to mention as I mentioned previously, that "having some async model to > > dump vmstate" isn't something special to VFIO, it can easily be extended to > > either RAM, or other normal VMSDs if we can tackle other issues here and > > there. The general request is the same. It'll be a chaos if vfio starts > > to create its own threads, then vDPA and others. It is much saner to make > > it a generic model to me. > > I more or less know now how the v2 of this patch set needs to look like > (at least architecturally). > > Will try to prepare something in the coming weeks. Thanks.
On Thu, Jun 27, 2024 at 11:14:28AM +0200, Maciej S. Szmigiero wrote: > Having RAM sent in parallel with non-iterables would make sense to me, > but I am not 100% sure this is a safe thing to do - after all, currently > non-iterables can rely on the whole RAM being already transferred. And I forgot to comment on this one.. but that's a good point. I think we need further investigation indeed on this one. Some devices may need special dependency like what you said either on memory fully loaded, or something else like BQL, so at least concurrent load() won't work for the latter. What I was hoping is that we can start to collect some time-consuming objects into async-model if they do not have such dependencies. The thing in my mind is still vcpus so far: that's what I observed a major uncertainty on causing major downtimes as well. I remember vcpu only needs a loaded memory until KVM_RUN triggering loading of CR3 so _maybe_ that'll be fine, but that needs some double checks. Thanks,
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/ Changes from the RFC (v0): * Extend the existing multifd packet format instead of introducing a new migration channel header. * As the replacement of switching the migration channel header on or off introduce "x-migration-multifd-transfer" VFIO device property instead that allows configuring at runtime whether to send the particular device state via multifd channels when live migrating that device. This property defaults to "false" for bit stream compatibility with older QEMU versions. * Remove the support for having dedicated device state transfer multifd channels since the same downtime performance can be attained by simply reducing the total number of multifd channels in a shared channel configuration to the number of channels available for RAM transfer in the dedicated device state channels configuration. For example, the best downtime from the dedicated device state config on my setup (achieved in configuration of 10 total multifd channels / 4 dedicated device state channels) can also be achieved in the shared RAM/device state channel configuration by reducing the total multifd channel count to 6. It looks like not having too many RAM transfer multifd channels is key to having a good downtime since the results are reproducibly worse with 15 shared channels total, while they are as good as with 6 shared channels if there are 15 total channels but 4 of them are dedicated to transferring device state (leaving 11 for RAM transfer). * Make the next multifd channel selection more fair when converting multifd_send_pages::next_channel to atomic. * Convert the code to use QEMU thread sync primitives (QemuMutex with QemuLockable, QemuCond) instead of their Glib equivalents (GMutex, GMutexLocker and GCond). * Rename complete_precopy_async{,_wait} to complete_precopy_{begin,_end} as suggested. * Rebase onto the last week's QEMU git master and retest. When working on the updated patch set version I also investigated the possibility of refactoring VM live phase (non-downtime) transfers to happen via multifd channels. However, the VM live phase transfer works differently: it happens opportunistically until the remaining data drops below the switchover threshold, rather that transferring always the whole device state data until their exhaustion. For this reason, there would need to be some way in the migration framework to update the remaining data estimate from per-device saving/transfer queuing thread and then stop these threads when the decision has been reached in the migration core to stop the VM and switch over. Such functionality would need to be introduced first. There would also need to be some fairness guarantees so every device gets similar access to multifd channels - otherwise there could be a situation that the remaining data never drops below switchover threshold because some devices are starved with respect to access to the multifd transfer channels - as in the VM live phase additional device data is constantly being generated. Moreover, there's nothing stopping a QEMU device driver from requiring different handling (loading, etc.) of VM live phase data from the post-switchover data. For cases like this some kind of a new device VM live phase incoming data load handler would need to be introduced too. For the above reasons, the VM live phase multifd transfer functionality isn't a simple extension of the functionality introduced by this patch set. For convenience, this patch set is also available as a git tree: https://github.com/maciejsszmigiero/qemu/tree/multifd-device-state-transfer-vfio Maciej S. Szmigiero (13): vfio/migration: Add save_{iterate,complete_precopy}_started trace events migration/ram: Add load start trace event migration/multifd: Zero p->flags before starting filling a packet migration: Add save_live_complete_precopy_{begin,end} handlers migration: Add qemu_loadvm_load_state_buffer() and its handler migration: Add load_finish handler and associated functions migration/multifd: Device state transfer support - receive side migration/multifd: Convert multifd_send_pages::next_channel to atomic migration/multifd: Device state transfer support - send side migration/multifd: Add migration_has_device_state_support() vfio/migration: Multifd device state transfer support - receive side vfio/migration: Add x-migration-multifd-transfer VFIO property vfio/migration: Multifd device state transfer support - send side hw/vfio/migration.c | 545 +++++++++++++++++++++++++++++++++- hw/vfio/pci.c | 7 + hw/vfio/trace-events | 15 +- include/hw/vfio/vfio-common.h | 27 ++ include/migration/misc.h | 5 + include/migration/register.h | 70 +++++ migration/migration.c | 6 + migration/migration.h | 3 + migration/multifd-zlib.c | 2 +- migration/multifd-zstd.c | 2 +- migration/multifd.c | 336 +++++++++++++++++---- migration/multifd.h | 57 +++- migration/ram.c | 1 + migration/savevm.c | 112 +++++++ migration/savevm.h | 7 + migration/trace-events | 1 + 16 files changed, 1132 insertions(+), 64 deletions(-)