| Message ID | 20200204050841.44453-1-zhukeqian1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Optimization about wait-unplug migration state | expand |
Keqian Zhu <zhukeqian1@huawei.com> wrote: > qemu_savevm_nr_failover_devices() is originally designed to > get the number of failover devices, but it actually returns > the number of "unplug-pending" failover devices now. Moreover, > what drives migration state to wait-unplug should be the number > of "unplug-pending" failover devices, not all failover devices. > > We can also notice that qemu_savevm_state_guest_unplug_pending() > and qemu_savevm_nr_failover_devices() is equivalent almost (from > the code view). So the latter is incorrect semantically and > useless, just delete it. > > In the qemu_savevm_state_guest_unplug_pending(), once hit a > unplug-pending failover device, then it can return true right > now to save cpu time. > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> Reviewed-by: Juan Quintela <quintela@redhat.com> For my reading you are right: - 1st function naming is not right - 2nd function achieves exactly the same effect I will wait until Jens says anything, but then I will queue it. Thanks, Juan.
On Tue, Feb 04, 2020 at 01:08:41PM +0800, Keqian Zhu wrote: >qemu_savevm_nr_failover_devices() is originally designed to >get the number of failover devices, but it actually returns >the number of "unplug-pending" failover devices now. Moreover, >what drives migration state to wait-unplug should be the number >of "unplug-pending" failover devices, not all failover devices. > >We can also notice that qemu_savevm_state_guest_unplug_pending() >and qemu_savevm_nr_failover_devices() is equivalent almost (from >the code view). So the latter is incorrect semantically and >useless, just delete it. > >In the qemu_savevm_state_guest_unplug_pending(), once hit a >unplug-pending failover device, then it can return true right >now to save cpu time. > >Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >--- >Cc: jfreimann@redhat.com >Cc: Juan Quintela <quintela@redhat.com> >Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >--- > migration/migration.c | 2 +- > migration/savevm.c | 24 +++--------------------- > migration/savevm.h | 1 - > 3 files changed, 4 insertions(+), 23 deletions(-) > >diff --git a/migration/migration.c b/migration/migration.c >index 3a21a4686c..deedc968cf 100644 >--- a/migration/migration.c >+++ b/migration/migration.c >@@ -3333,7 +3333,7 @@ static void *migration_thread(void *opaque) > > qemu_savevm_state_setup(s->to_dst_file); > >- if (qemu_savevm_nr_failover_devices()) { >+ if (qemu_savevm_state_guest_unplug_pending()) { > migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, > MIGRATION_STATUS_WAIT_UNPLUG); > >diff --git a/migration/savevm.c b/migration/savevm.c >index f19cb9ec7a..1d4220ece8 100644 >--- a/migration/savevm.c >+++ b/migration/savevm.c >@@ -1140,36 +1140,18 @@ void qemu_savevm_state_header(QEMUFile *f) > } > } > >-int qemu_savevm_nr_failover_devices(void) >+bool qemu_savevm_state_guest_unplug_pending(void) > { > SaveStateEntry *se; >- int n = 0; > > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > if (se->vmsd && se->vmsd->dev_unplug_pending && > se->vmsd->dev_unplug_pending(se->opaque)) { >- n++; >- } >- } >- >- return n; >-} >- >-bool qemu_savevm_state_guest_unplug_pending(void) >-{ >- SaveStateEntry *se; >- int n = 0; >- >- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { >- if (!se->vmsd || !se->vmsd->dev_unplug_pending) { >- continue; >- } >- if (se->vmsd->dev_unplug_pending(se->opaque)) { >- n++; >+ return true; > } > } > >- return n > 0; >+ return false; > } > > void qemu_savevm_state_setup(QEMUFile *f) >diff --git a/migration/savevm.h b/migration/savevm.h >index c42b9c80ee..ba64a7e271 100644 >--- a/migration/savevm.h >+++ b/migration/savevm.h >@@ -31,7 +31,6 @@ > > bool qemu_savevm_state_blocked(Error **errp); > void qemu_savevm_state_setup(QEMUFile *f); >-int qemu_savevm_nr_failover_devices(void); > bool qemu_savevm_state_guest_unplug_pending(void); > int qemu_savevm_state_resume_prepare(MigrationState *s); > void qemu_savevm_state_header(QEMUFile *f); >-- >2.19.1 Looks good to me. I tested it and it still works, so Tested-by: Jens Freimann <jfreimann@redhat.com> Reviewed-by: Jens Freimann <jfreimann@redhat.com> regards Jens
On 2020/2/4 17:14, Juan Quintela wrote: > Keqian Zhu <zhukeqian1@huawei.com> wrote: >> qemu_savevm_nr_failover_devices() is originally designed to >> get the number of failover devices, but it actually returns >> the number of "unplug-pending" failover devices now. Moreover, >> what drives migration state to wait-unplug should be the number >> of "unplug-pending" failover devices, not all failover devices. >> >> We can also notice that qemu_savevm_state_guest_unplug_pending() >> and qemu_savevm_nr_failover_devices() is equivalent almost (from >> the code view). So the latter is incorrect semantically and >> useless, just delete it. >> >> In the qemu_savevm_state_guest_unplug_pending(), once hit a >> unplug-pending failover device, then it can return true right >> now to save cpu time. >> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > For my reading you are right: > - 1st function naming is not right > - 2nd function achieves exactly the same effect > > I will wait until Jens says anything, but then I will queue it. > > Thanks, Juan. > > > . > Thanks, Keqian.
On 2020/2/5 22:40, Jens Freimann wrote: > On Tue, Feb 04, 2020 at 01:08:41PM +0800, Keqian Zhu wrote: >> qemu_savevm_nr_failover_devices() is originally designed to >> get the number of failover devices, but it actually returns >> the number of "unplug-pending" failover devices now. Moreover, >> what drives migration state to wait-unplug should be the number >> of "unplug-pending" failover devices, not all failover devices. >> >> We can also notice that qemu_savevm_state_guest_unplug_pending() >> and qemu_savevm_nr_failover_devices() is equivalent almost (from >> the code view). So the latter is incorrect semantically and >> useless, just delete it. >> >> In the qemu_savevm_state_guest_unplug_pending(), once hit a >> unplug-pending failover device, then it can return true right >> now to save cpu time. >> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> Cc: jfreimann@redhat.com >> Cc: Juan Quintela <quintela@redhat.com> >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> --- >> migration/migration.c | 2 +- >> migration/savevm.c | 24 +++--------------------- >> migration/savevm.h | 1 - >> 3 files changed, 4 insertions(+), 23 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 3a21a4686c..deedc968cf 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -3333,7 +3333,7 @@ static void *migration_thread(void *opaque) >> >> qemu_savevm_state_setup(s->to_dst_file); >> >> - if (qemu_savevm_nr_failover_devices()) { >> + if (qemu_savevm_state_guest_unplug_pending()) { >> migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, >> MIGRATION_STATUS_WAIT_UNPLUG); >> >> diff --git a/migration/savevm.c b/migration/savevm.c >> index f19cb9ec7a..1d4220ece8 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -1140,36 +1140,18 @@ void qemu_savevm_state_header(QEMUFile *f) >> } >> } >> >> -int qemu_savevm_nr_failover_devices(void) >> +bool qemu_savevm_state_guest_unplug_pending(void) >> { >> SaveStateEntry *se; >> - int n = 0; >> >> QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { >> if (se->vmsd && se->vmsd->dev_unplug_pending && >> se->vmsd->dev_unplug_pending(se->opaque)) { >> - n++; >> - } >> - } >> - >> - return n; >> -} >> - >> -bool qemu_savevm_state_guest_unplug_pending(void) >> -{ >> - SaveStateEntry *se; >> - int n = 0; >> - >> - QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { >> - if (!se->vmsd || !se->vmsd->dev_unplug_pending) { >> - continue; >> - } >> - if (se->vmsd->dev_unplug_pending(se->opaque)) { >> - n++; >> + return true; >> } >> } >> >> - return n > 0; >> + return false; >> } >> >> void qemu_savevm_state_setup(QEMUFile *f) >> diff --git a/migration/savevm.h b/migration/savevm.h >> index c42b9c80ee..ba64a7e271 100644 >> --- a/migration/savevm.h >> +++ b/migration/savevm.h >> @@ -31,7 +31,6 @@ >> >> bool qemu_savevm_state_blocked(Error **errp); >> void qemu_savevm_state_setup(QEMUFile *f); >> -int qemu_savevm_nr_failover_devices(void); >> bool qemu_savevm_state_guest_unplug_pending(void); >> int qemu_savevm_state_resume_prepare(MigrationState *s); >> void qemu_savevm_state_header(QEMUFile *f); >> -- >> 2.19.1 > > Looks good to me. I tested it and it still works, so > Tested-by: Jens Freimann <jfreimann@redhat.com> > Reviewed-by: Jens Freimann <jfreimann@redhat.com> > regards > Jens > > > . > Thanks, Keqian.
diff --git a/migration/migration.c b/migration/migration.c index 3a21a4686c..deedc968cf 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -3333,7 +3333,7 @@ static void *migration_thread(void *opaque) qemu_savevm_state_setup(s->to_dst_file); - if (qemu_savevm_nr_failover_devices()) { + if (qemu_savevm_state_guest_unplug_pending()) { migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_WAIT_UNPLUG); diff --git a/migration/savevm.c b/migration/savevm.c index f19cb9ec7a..1d4220ece8 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1140,36 +1140,18 @@ void qemu_savevm_state_header(QEMUFile *f) } } -int qemu_savevm_nr_failover_devices(void) +bool qemu_savevm_state_guest_unplug_pending(void) { SaveStateEntry *se; - int n = 0; QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { if (se->vmsd && se->vmsd->dev_unplug_pending && se->vmsd->dev_unplug_pending(se->opaque)) { - n++; - } - } - - return n; -} - -bool qemu_savevm_state_guest_unplug_pending(void) -{ - SaveStateEntry *se; - int n = 0; - - QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { - if (!se->vmsd || !se->vmsd->dev_unplug_pending) { - continue; - } - if (se->vmsd->dev_unplug_pending(se->opaque)) { - n++; + return true; } } - return n > 0; + return false; } void qemu_savevm_state_setup(QEMUFile *f) diff --git a/migration/savevm.h b/migration/savevm.h index c42b9c80ee..ba64a7e271 100644 --- a/migration/savevm.h +++ b/migration/savevm.h @@ -31,7 +31,6 @@ bool qemu_savevm_state_blocked(Error **errp); void qemu_savevm_state_setup(QEMUFile *f); -int qemu_savevm_nr_failover_devices(void); bool qemu_savevm_state_guest_unplug_pending(void); int qemu_savevm_state_resume_prepare(MigrationState *s); void qemu_savevm_state_header(QEMUFile *f);
qemu_savevm_nr_failover_devices() is originally designed to get the number of failover devices, but it actually returns the number of "unplug-pending" failover devices now. Moreover, what drives migration state to wait-unplug should be the number of "unplug-pending" failover devices, not all failover devices. We can also notice that qemu_savevm_state_guest_unplug_pending() and qemu_savevm_nr_failover_devices() is equivalent almost (from the code view). So the latter is incorrect semantically and useless, just delete it. In the qemu_savevm_state_guest_unplug_pending(), once hit a unplug-pending failover device, then it can return true right now to save cpu time. Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> --- Cc: jfreimann@redhat.com Cc: Juan Quintela <quintela@redhat.com> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> --- migration/migration.c | 2 +- migration/savevm.c | 24 +++--------------------- migration/savevm.h | 1 - 3 files changed, 4 insertions(+), 23 deletions(-)