On Thu, Jun 07, 2018 at 01:24:29PM +0800, Wei Wang wrote:
On 06/06/2018 07:02 PM, Peter Xu wrote:
On Wed, Jun 06, 2018 at 06:04:23PM +0800, Wei Wang wrote:
On 06/06/2018 01:42 PM, Peter Xu wrote:
IMHO migration states do not suite here. IMHO bitmap syncing is too
frequently an operation, especially at the end of a precopy migration.
If you really want to introduce some notifiers, I would prefer
something new rather than fiddling around with migration state. E.g.,
maybe a new migration event notifiers, then introduce two new events
for both start/end of bitmap syncing.
Please see if below aligns to what you meant:
MigrationState {
...
+ int ram_save_state;
}
typedef enum RamSaveState {
RAM_SAVE_BEGIN = 0,
RAM_SAVE_END = 1,
RAM_SAVE_MAX = 2
}
then at the step 1) and 3) you concluded somewhere below, we change the
state and invoke the callback.
I mean something like this:
1693c64c27 ("postcopy: Add notifier chain", 2018-03-20)
That was a postcopy-only notifier. Maybe we can generalize it into a
more common notifier for the migration framework so that we can even
register with non-postcopy events like bitmap syncing?
Precopy already has its own notifiers: git 99a0db9b
If we want to reuse, that one would be more suitable. I think mixing
non-related events into one notifier list isn't nice.
I think that's only for migration state changes?
Btw, the migration_state_notifiers is already there, but seems not really
used (I only tracked spice-core.c called
add_migration_state_change_notifier). I thought adding new migration states
can reuse all that we have.
What's your real concern about that? (not sure how defining new events would
make a difference)
Migration state is exposed via control path (QMP). Adding new states
mean that the QMP clients will see more. IMO that's not really
anything that a QMP client will need to know, instead we can keep it
internally. That's a reason from compatibility pov.
Meanwhile, it's not really a state-thing at all for me. It looks
really more like hook or event (start/stop of sync).
Thanks for sharing your concerns in detail, which are quite helpful for the
discussion. To reuse 99a0db9b, we can also add sub-states (or say events),
instead of new migration states.
For example, we can still define "enum RamSaveState" as above, which can be
an indication for the notifier queued on the 99a0db9b notider_list to decide
whether to call start or stop.
Does this solve your concern?
Frankly speaking I don't fully understand how you would add that
sub-state. If you are confident with the idea, maybe you can post
your new version with the change, then I can read the code.
Besides, if you are going to use a busy loop, then I would be not
quite sure about whether you really want to share that iothread with
others, since AFAIU that's not how iothread is designed (which is
mostly event-based and should not welcome out-of-control blocking in
the handler of events). Though I am not 100% confident about my
understaning on that, I only raise this question up. Anyway you'll
just take over the thread for a while without sharing, and after the
burst IOs it's mostly never used (until the next bitmap sync). Then
it seems a bit confusing to me on why you need to share that after
all.
I'm a bit curious on how much time will it use to do
one round of the free page hints (e.g., an idle guest with 8G mem, or
any configuration you tested)? I suppose during that time the
iothread will be held steady with 100% cpu usage, am I right?
Compared to the time spent by the legacy migration to send free pages, that
small amount of CPU usage spent on filtering free pages could be neglected.
Grinding a chopper will not hold up the work of cutting firewood :)
Sorry I didn't express myself clearly.
My question was that, have you measured how long time it will take
from starting of the free page hints (when balloon state is set to
FREE_PAGE_REPORT_S_REQUESTED), until it completes (when QEMU receives
the VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID, then set the status to
FREE_PAGE_REPORT_S_STOP)?