Re: [PATCH] migration: Optimization about wait-unplug migration state
On 2020/2/4 17:14, Juan Quintela wrote: > 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 > > Reviewed-by: Juan Quintela > > 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.
Re: [PATCH] migration: Optimization about wait-unplug migration state
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
>> ---
>> Cc: jfreim...@redhat.com
>> Cc: Juan Quintela
>> Cc: "Dr. David Alan Gilbert"
>> ---
>> 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
>> @@ -,7 +,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(>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, _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, _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
> Reviewed-by: Jens Freimann
> regards
> Jens
>
>
> .
>
Thanks,
Keqian.
Re: [PATCH] migration: Optimization about wait-unplug migration state
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
---
Cc: jfreim...@redhat.com
Cc: Juan Quintela
Cc: "Dr. David Alan Gilbert"
---
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
@@ -,7 +,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(>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, _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, _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
Reviewed-by: Jens Freimann
regards
Jens
Re: [PATCH] migration: Optimization about wait-unplug migration state
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 Reviewed-by: Juan Quintela 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.
[PATCH] migration: Optimization about wait-unplug migration state
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
---
Cc: jfreim...@redhat.com
Cc: Juan Quintela
Cc: "Dr. David Alan Gilbert"
---
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
@@ -,7 +,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(>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, _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, _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
