Re: [Qemu-devel] [RFC PATCH v2 2/5] cpu: Introduce CPUState::stable_cpu_id

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5152 bytes --]

On Mon, Jul 11, 2016 at 09:58:21AM +0200, Igor Mammedov wrote:
> On Mon, 11 Jul 2016 13:22:37 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote:
> > > On Fri, 8 Jul 2016 15:19:58 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote:  
> > > > > Add CPUState::stable_cpu_id and use that as instance_id in
> > > > > vmstate_register() call.
> > > > > 
> > > > > Introduce has-stable_cpu_id property that allows target machines to
> > > > > optionally switch to using stable_cpu_id instead of cpu_index.
> > > > > This will help allow successful migration in cases where holes are
> > > > > introduced in cpu_index range after CPU hot removals.
> > > > > 
> > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > >  exec.c            | 6 ++++--
> > > > >  include/qom/cpu.h | 5 +++++
> > > > >  qom/cpu.c         | 6 ++++++
> > > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/exec.c b/exec.c
> > > > > index fb73910..3b36fe5 100644
> > > > > --- a/exec.c
> > > > > +++ b/exec.c
> > > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu)
> > > > >  void cpu_vmstate_register(CPUState *cpu)
> > > > >  {
> > > > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > +    int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id :
> > > > > +                      cpu->cpu_index;
> > > > >  
> > > > >      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > > -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> > > > > +        vmstate_register(NULL, instance_id, &vmstate_cpu_common, cpu);
> > > > >      }
> > > > >      if (cc->vmsd != NULL) {
> > > > > -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > > > > +        vmstate_register(NULL, instance_id, cc->vmsd, cpu);
> > > > >      }
> > > > >  }
> > > > >  
> > > > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > > > > index 331386f..527c021 100644
> > > > > --- a/include/qom/cpu.h
> > > > > +++ b/include/qom/cpu.h
> > > > > @@ -273,6 +273,9 @@ struct qemu_work_item {
> > > > >   * @kvm_fd: vCPU file descriptor for KVM.
> > > > >   * @work_mutex: Lock to prevent multiple access to queued_work_*.
> > > > >   * @queued_work_first: First asynchronous work pending.
> > > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls
> > > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id
> > > > > + *     over cpu_index during vmstate registration.
> > > > >   *
> > > > >   * State of one CPU core or thread.
> > > > >   */
> > > > > @@ -360,6 +363,8 @@ struct CPUState {
> > > > >         (absolute value) offset as small as possible.  This reduces code
> > > > >         size, especially for hosts without large memory offsets.  */
> > > > >      uint32_t tcg_exit_req;
> > > > > +    int stable_cpu_id;
> > > > > +    bool has_stable_cpu_id;
> > > > >  };
> > > > >  
> > > > >  QTAILQ_HEAD(CPUTailQ, CPUState);
> > > > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > > > index 1095ea1..bae1bf7 100644
> > > > > --- a/qom/cpu.c
> > > > > +++ b/qom/cpu.c
> > > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
> > > > >      return cpu->cpu_index;
> > > > >  }
> > > > >  
> > > > > +static Property cpu_common_properties[] = {
> > > > > +    DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false),    
> > > > 
> > > > It seems odd to me that stable_cpu_id itself isn't exposed as a
> > > > property.  Even if we don't need to set it externally for now, it
> > > > really should be QOM introspectable.  
> > > Should it? Why?  
> > 
> > Well, for one thing it's really strange to have the boolean flag
> > exposed, but not the value itself.
> property doesn't always means that it's intended as an external interface
> 
> > 
> > > It's QEMU internal detail and outside world preferably shouldn't
> > > know anything about it.  
> > 
> > Hrm.. I guess kinda.  But I think it's less an internal detail than
> > the existing cpu_index is.
> so it' better not to start to advertise it as an external interface.

Right.  My comments were based on the assumption that this was
intended as some sort of generally useful stable CPU id, rather than
something narrowly focussed on migration.

Having understood things better from our IRC discussion, I withdraw
this objection.  Things also make more sense to me once this is made a
class property instead of an instance property, which you've done in
your proposed patches.

> Should be add some flag to generic property to mark it as internal?

If such a thing exists that would be good.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]