On 17.07.2024 22:19, Fabiano Rosas wrote:
Peter Xu <pet...@redhat.com> writes:
On Tue, Jul 16, 2024 at 10:10:12PM +0200, Maciej S. Szmigiero wrote:
On 27.06.2024 16:56, Peter Xu wrote:
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.szmigi...@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.szmigi...@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-faro...@suse.de
That's a long list, thanks for these comments.
I have responded to them inline below.
(..)
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)?
It will need some signal to the worker threads pool to terminate before
waiting for them to finish (as the code in [1] just waits).
In the case of current vfio_save_complete_precopy_async_thread() implementation,
this signal isn't necessary as this thread simply terminates when it has read
all the date it needs from the device.
In a worker threads pool case there will be some threads waiting for
jobs to be queued to them and so they will need to be somehow signaled
to exit.
Right. We may need something like multifd_send_should_exit() +
MultiFDSendParams.sem. It'll be nicer if we can generalize that part so
multifd threads can also rebase to that thread model, but maybe I'm asking
too much.
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]
(..)
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.
Since it seems that the discussion on Fabiano's patch set has subsided I think
I will start by basing my updated patch set on top of his RFC and then if
Fabiano wants to submit v1/v2 of his patch set then I will rebase mine on top
of it.
Otherwise, you can wait until I have a v2 ready and then we can work with that.
Oh I thought you already started modifying his patchset.
In this case, AFAIR Fabiano has plan to rework that RFC series, so maybe
you want to double check with him, and can also wait for his new version if
that's easier, because I do expect there'll be major changes.
Fabiano?
Don't wait on me. I think I can make the changes Peter suggested without
affecting too much the interfaces used by this series. If it comes to
it, I can rebase this series "under" Maciej's.
So to be clear, I should base my series on top of your existing RFC patch set
and then we'll swap these RFC patches for the updated versions, correct?
Thanks,
Maciej
