Re: [Qemu-ppc] [Qemu-devel] [PATCH for-2.10 07/23] pc: add node-id property to CPU

[David Gibson]

On Thu, Apr 27, 2017 at 01:32:25PM -0300, Eduardo Habkost wrote:
> On Thu, Apr 27, 2017 at 03:14:06PM +0200, Igor Mammedov wrote:
> > On Wed, 26 Apr 2017 09:21:38 -0300
> > Eduardo Habkost <address@hidden> wrote:
> > 
> > adding Peter to CC list
> > 
> > [...]
> > 
> > > On Wed, Apr 19, 2017 at 01:14:58PM +0200, Igor Mammedov wrote:
> > > > On Wed, 12 Apr 2017 18:02:39 -0300
> > > > Eduardo Habkost <address@hidden> wrote:
> > > >   
> > > > > On Wed, Mar 22, 2017 at 02:32:32PM +0100, Igor Mammedov wrote:  
> > > > > > it will allow switching from cpu_index to property based
> > > > > > numa mapping in follow up patches.    
> > > > > 
> > > > > I am not sure I understand all the consequences of this, so I
> > > > > will give it a try:
> > > > > 
> > > > > "node-id" is an existing field in CpuInstanceProperties.
> > > > > CpuInstanceProperties is used on both query-hotpluggable-cpus
> > > > > output and in MachineState::possible_cpus.
> > > > > 
> > > > > We will start using MachineState::possible_cpus to keep track of
> > > > > NUMA CPU affinity, and that means query-hotpluggable-cpus will
> > > > > start reporting a "node-id" property when a NUMA mapping is
> > > > > configured.
> > > > > 
> > > > > To allow query-hotpluggable-cpus to report "node-id", the CPU
> > > > > objects must have a "node-id" property that can be set. This
> > > > > patch adds the "node-id" property to X86CPU.
> > > > > 
> > > > > Is this description accurate? Is the presence of "node-id" in
> > > > > query-hotpluggable-cpus the only reason we really need this
> > > > > patch, or is there something else that requires the "node-id"
> > > > > property?  
> > > > That accurate description, node-id is in the same 'address'
> > > > properties category as socket/core/thread-id. So if you have
> > > > numa enabled machine you'd see node-id property in
> > > > query-hotpluggable-cpus.  
> > > 
> > > I agree that we can make -numa cpu affect query-hotpluggable-cpus
> > > output (i.e. affect some field on HotpluggableCPU.props).
> > > 
> > > But it looks like we disagree about the purpose of
> > > HotpluggableCPU.props:
> > > 
> > > I believe HotpluggableCPU.props is just an opaque identifier for
> > > the location we want to plug the CPU, and the only requirement is
> > > that it should be unique and have all the information device_add
> > > needs. As socket IDs are already unique on our existing machines,
> > > and socket<=>node mapping is already configured using -numa cpu,
> > > node-id doesn't need to be in HotpluggableCPU.props. (see example
> > > below)
> > node-id is also location property which logically complements
> > to socket/core/thread properties.  Also socket is not necessarily
> > unique id that maps 1:1 to node-id from generic pov.
> > BTW -numa cpu[s] is not the only way to specify mapping,
> > it could be specified like we do with pc-dimm:
> >    device_add pc-dimm,node=x
> > 
> > Looking at it more genericly, there could be the same
> > socket-ids for different nodes, then we would have to add
> > node-id to props anyway and end up with 2 node-id, one in props
> > and another in the parent struct.
> 
> This is where my expectations are different: I think
> HotpluggableCPU.props is just an identifier property for CPU
> slots that is used for device_add (and will be used for -numa
> cpu), and isn't supposed to be be interpreted by clients.
> 
> The problem I see is that the property has two completely
> different purposes: identifying a given CPU slot for device_add
> (and -numa cpu), and introspection of topology information about
> the CPU slot. Today we are lucky and those goals don't conflict
> with each other, but I worry this might cause trouble in the
> future.

Yeah, I share your concern.  And even if we allow that the topology
information may be read by the user, at the moment the
socket/core/thread values are "read only" in the sense that the client
should do nothing by read them from the query (possibly look at them
for its own interest) and echo them back verbatim to device_add.

node id is different because it's something the user/management might
want to actually choose.  So it seems dubious to me that it's in the
same structure.

-- 
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

signature.asc
Description: PGP signature