| Message ID | 20140527143920.GA5640@amt.cnet |
|---|---|
| State | New |
| Headers | show |
On Tue, May 27, 2014 at 11:39:20AM -0300, Marcelo Tosatti wrote: > > Migration blocker is redudant: blocking savevm is sufficient. > Removing the redundancy looks welcome, but at the same time the migrate_add_blocker() call ensured we had a clearer error message (I mean: if we did mention invtsc in the error message, which we still don't, but should). I am not familiar with how we deal with savevm/migration errors, so I don't know what's best here. Juan, do you have any suggestions? Additional small comment below: > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index f9ffa4b..b29098a 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -450,7 +450,7 @@ static bool hyperv_enabled(X86CPU *cpu) > cpu->hyperv_relaxed_timing); > } > > -static Error *invtsc_mig_blocker; > +bool invtsc_mig_blocked; > > #define KVM_MAX_CPUID_ENTRIES 100 > > @@ -708,13 +708,9 @@ int kvm_arch_init_vcpu(CPUState *cs) > } > > c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0); > - if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) { > - /* for migration */ > - error_setg(&invtsc_mig_blocker, > - "State blocked by non-migratable CPU device"); > - migrate_add_blocker(invtsc_mig_blocker); > - /* for savevm */ > + if (c && (c->edx & 1<<8) && invtsc_mig_blocked == false) { > vmstate_x86_cpu.unmigratable = 1; > + invtsc_mig_blocked = true; Why the invtsc_mig_blocked variable? Setting unmigratable=1 twice won't hurt anybody. > } >
On Tue, May 27, 2014 at 12:55:01PM -0300, Eduardo Habkost wrote: > On Tue, May 27, 2014 at 11:39:20AM -0300, Marcelo Tosatti wrote: > > > > Migration blocker is redudant: blocking savevm is sufficient. > > > > Removing the redundancy looks welcome, but at the same time the > migrate_add_blocker() call ensured we had a clearer error message (I > mean: if we did mention invtsc in the error message, which we still > don't, but should). Agree the error message should provide further information. For savevm its not possible, without further changes, to improve error message. Do you want to maintain migration blocker to provide the additional "blocked by invtsc feature of CPU device" or do you want to remove migration blocker and have "blocked by CPU device" ?
Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, May 27, 2014 at 11:39:20AM -0300, Marcelo Tosatti wrote: >> >> Migration blocker is redudant: blocking savevm is sufficient. >> > > Removing the redundancy looks welcome, but at the same time the > migrate_add_blocker() call ensured we had a clearer error message (I > mean: if we did mention invtsc in the error message, which we still > don't, but should). > > I am not familiar with how we deal with savevm/migration errors, so I > don't know what's best here. Juan, do you have any suggestions? CHange unmigratable to Error * and add the message there? First one wins (or a way to combine them?). Really I don't have a better idea. Later, Juan. > > Additional small comment below: > >> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> >> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c >> index f9ffa4b..b29098a 100644 >> --- a/target-i386/kvm.c >> +++ b/target-i386/kvm.c >> @@ -450,7 +450,7 @@ static bool hyperv_enabled(X86CPU *cpu) >> cpu->hyperv_relaxed_timing); >> } >> >> -static Error *invtsc_mig_blocker; >> +bool invtsc_mig_blocked; >> >> #define KVM_MAX_CPUID_ENTRIES 100 >> >> @@ -708,13 +708,9 @@ int kvm_arch_init_vcpu(CPUState *cs) >> } >> >> c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0); >> - if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) { >> - /* for migration */ >> - error_setg(&invtsc_mig_blocker, >> - "State blocked by non-migratable CPU device"); >> - migrate_add_blocker(invtsc_mig_blocker); >> - /* for savevm */ >> + if (c && (c->edx & 1<<8) && invtsc_mig_blocked == false) { >> vmstate_x86_cpu.unmigratable = 1; >> + invtsc_mig_blocked = true; > > Why the invtsc_mig_blocked variable? Setting unmigratable=1 twice won't > hurt anybody. > >> } >>
On Wed, May 28, 2014 at 11:14:14AM +0200, Juan Quintela wrote: > Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Tue, May 27, 2014 at 11:39:20AM -0300, Marcelo Tosatti wrote: > >> > >> Migration blocker is redudant: blocking savevm is sufficient. > >> > > > > Removing the redundancy looks welcome, but at the same time the > > migrate_add_blocker() call ensured we had a clearer error message (I > > mean: if we did mention invtsc in the error message, which we still > > don't, but should). > > > > I am not familiar with how we deal with savevm/migration errors, so I > > don't know what's best here. Juan, do you have any suggestions? > > > CHange unmigratable to Error * and add the message there? First one > wins (or a way to combine them?). > > Really I don't have a better idea. My first question is: why migrate_add_blocker() doesn't affect savevm? On which cases does it make sense to block migration only, and on which cases does it make sense to block migration and savevm?
On Tue, May 27, 2014 at 10:51:28PM -0300, Marcelo Tosatti wrote: > On Tue, May 27, 2014 at 12:55:01PM -0300, Eduardo Habkost wrote: > > On Tue, May 27, 2014 at 11:39:20AM -0300, Marcelo Tosatti wrote: > > > > > > Migration blocker is redudant: blocking savevm is sufficient. > > > > > > > Removing the redundancy looks welcome, but at the same time the > > migrate_add_blocker() call ensured we had a clearer error message (I > > mean: if we did mention invtsc in the error message, which we still > > don't, but should). > > Agree the error message should provide further information. > > For savevm its not possible, without further changes, to > improve error message. > > Do you want to maintain migration blocker to provide > the additional "blocked by invtsc feature of CPU device" > or do you want to remove migration blocker and have > "blocked by CPU device" ? > Having migration blocked by a CPU feature is something we never did before and likely to surprise a few users. So I believe a clearer error message mentioning invtsc is worth the extra code.
On Wed, May 28, 2014 at 01:25:38PM -0300, Eduardo Habkost wrote: > On Wed, May 28, 2014 at 11:14:14AM +0200, Juan Quintela wrote: > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, May 27, 2014 at 11:39:20AM -0300, Marcelo Tosatti wrote: > > >> > > >> Migration blocker is redudant: blocking savevm is sufficient. > > >> > > > > > > Removing the redundancy looks welcome, but at the same time the > > > migrate_add_blocker() call ensured we had a clearer error message (I > > > mean: if we did mention invtsc in the error message, which we still > > > don't, but should). > > > > > > I am not familiar with how we deal with savevm/migration errors, so I > > > don't know what's best here. Juan, do you have any suggestions? > > > > > > CHange unmigratable to Error * and add the message there? First one > > wins (or a way to combine them?). > > > > Really I don't have a better idea. > > My first question is: why migrate_add_blocker() doesn't affect savevm? > On which cases does it make sense to block migration only, and on which > cases does it make sense to block migration and savevm? Can't see any case in which blocking only migration makes sense. 1) savevm on source 2) loadvm on destination Is similar to migration regarding the state thats available on destination. Thinko?
On Thu, May 29, 2014 at 04:00:36PM -0300, Marcelo Tosatti wrote: > On Wed, May 28, 2014 at 01:25:38PM -0300, Eduardo Habkost wrote: > > On Wed, May 28, 2014 at 11:14:14AM +0200, Juan Quintela wrote: > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > On Tue, May 27, 2014 at 11:39:20AM -0300, Marcelo Tosatti wrote: > > > >> > > > >> Migration blocker is redudant: blocking savevm is sufficient. > > > >> > > > > > > > > Removing the redundancy looks welcome, but at the same time the > > > > migrate_add_blocker() call ensured we had a clearer error message (I > > > > mean: if we did mention invtsc in the error message, which we still > > > > don't, but should). > > > > > > > > I am not familiar with how we deal with savevm/migration errors, so I > > > > don't know what's best here. Juan, do you have any suggestions? > > > > > > > > > CHange unmigratable to Error * and add the message there? First one > > > wins (or a way to combine them?). > > > > > > Really I don't have a better idea. > > > > My first question is: why migrate_add_blocker() doesn't affect savevm? > > On which cases does it make sense to block migration only, and on which > > cases does it make sense to block migration and savevm? > > Can't see any case in which blocking only migration makes sense. > > 1) savevm on source > 2) loadvm on destination > > Is similar to migration regarding the state thats available on > destination. That's how it looks like to me. But I don't know if there are other use cases of savevm I am ignoring.
diff --git a/target-i386/kvm.c b/target-i386/kvm.c index f9ffa4b..b29098a 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -450,7 +450,7 @@ static bool hyperv_enabled(X86CPU *cpu) cpu->hyperv_relaxed_timing); } -static Error *invtsc_mig_blocker; +bool invtsc_mig_blocked; #define KVM_MAX_CPUID_ENTRIES 100 @@ -708,13 +708,9 @@ int kvm_arch_init_vcpu(CPUState *cs) } c = cpuid_find_entry(&cpuid_data.cpuid, 0x80000007, 0); - if (c && (c->edx & 1<<8) && invtsc_mig_blocker == NULL) { - /* for migration */ - error_setg(&invtsc_mig_blocker, - "State blocked by non-migratable CPU device"); - migrate_add_blocker(invtsc_mig_blocker); - /* for savevm */ + if (c && (c->edx & 1<<8) && invtsc_mig_blocked == false) { vmstate_x86_cpu.unmigratable = 1; + invtsc_mig_blocked = true; } cpuid_data.cpuid.padding = 0;
Migration blocker is redudant: blocking savevm is sufficient. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>