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

[Eduardo Habkost]

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.

> 
> 
> > I don't think clients should assume topology information in
> > HotpluggableCPU.props is always present, because the field has a
> > different purpose: letting clients know what are the required
> > device_add arguments. If we need introspection of CPU topology,
> > we can add new fields to HotpluggableCPU, outside 'props'.
> looking at existing clients (libvirt), it doesn't treat 'props'
> as opaque set, but parses it into topology information (my guess
> is that because it's the sole source such info from QEMU).
> Actually we never forbade this, the only requirement for
> props was that mgmt should provide those properties to create
> a cpu. Property names where designed in topology/location
> friendly terms so that clients could make the sense from them.
> 
> So I wouldn't try now to reduce meaning of 'props' to
> opaque as you suggest.

I see. This means my expectation is not met even today. I am not
thrilled about it, but that's OK.

> 
> [..]
> > My question is: if we use:
> > 
> >  -numa cpu,socket=2,core=1,thread=0,node-id=3
> > 
> > and then:
> > 
> >  device_add ...,socket=2,core=1,thread=0
> >  (omitting node-id on device_add)
> > 
> > won't it work exactly the same, and place the new CPU on NUMA
> > node 3?
> yep, it's allowed for compat reasons:
>   1: to allow DST start with old CLI variant, that didn't have node-id
>      (migration)
>   2: to let old libvirt hotplug CPUs, it doesn't treat 'props'
>      as opaque set that is just replayed to device_add,
>      instead it composes command from topo info it got
>      from QEMU and unfortunately node-id is only read but is
>      not emitted when device_add is composed
>     (I consider this bug but it's out in the wild so we have to deal with it)
> 
> we can't enforce presence in these cases or at least have to
> keep it relaxed for old machine types.

I see.

>  
> > In this case, if we don't need a node-id argument on device_add,
> > so node-id doesn't need to be in HotpluggableCPU.props.
> I'd say we currently don't have to (for above reasons) but
> it doesn't hurt and actually allows to use pc-dimm way of
> mapping CPUs to nodes as David noted. i.e.:
>   -device cpu-foo,node-id=x,...
> without any of -numa cpu[s] options on CLI.
> It's currently explicitly disabled but should work if one
> doesn't care about hotplug or if target doesn't care about
> mapping at startup (sPAPR), it also might work for x86 as
> well using _PXM method in ACPI.
> (But that's out of scope of this series and needs more
> testing as some guest OSes might expect populated SRAT
> to work correctly).

Yep. I understand that setting node-id is useful, I just didn't
expect it to be mandatory and included on HotpluggableCPU.props.

> 
> [...]
> > > 
> > > In future to avoid starting QEMU twice we were thinking about
> > > configuring QEMU from QMP at runtime, that's where preconfigure
> > > approach could be used to help solving it in the future:
> > > 
> > >   1. introduce pause before machine_init CLI option to allow
> > >      preconfig machine from qmp/monitor
> > >   2. make query-hotpluggable-cpus usable at preconfig time
> > >   3. start qemu with needed number of numa nodes and default mapping:
> > >          #qemu -smp ... -numa node,nodeid=0 -node node,nodeid=1
> > >   4. get possible cpus list  
> > 
> > This is where things can get tricky: if we have the default
> > mapping set, step 4 would return "node-id" already set on all
> > possible CPUs.
> that would depend on impl.
>  - display node-id with default preset values to override
>  - do not set defaults and force user to do mapping

Right. We could choose to initialize default values much later,
and leave it uninitialized.

> 
> > >   5. add qmp/monitor command variant for '-numa cpu' to set numa mapping  
> > 
> > This is where I think we would make things simpler: if node-id
> > isn't present on 'props', we can simply document the arguments
> > that identify the CPU for the numa-cpu command as "just use the
> > properties you get on query-hotpluggable-cpus.props". Clients
> > would be able to treat CpuInstanceProperties as an opaque CPU
> > slot identifier.
> > 
> > i.e. I think this would be a better way to define and document
> > the interface:
> > 
> > ##
> > # @NumaCpuOptions:
> > #
> > # Mapping of a given CPU (or a set of CPUs) to a NUMA node.
> > #
> > # @cpu: Properties identifying the CPU(s). Use the 'props' field of
> > #       query-hotpluggable-cpus for possible values for this
> > #       field.
> > #       TODO: describe what happens when 'cpu' matches
> > #       multiple slots.
> > # @node-id: NUMA node where the CPUs are going to be located.
> > ##
> > { 'struct': 'NumaCpuOptions',
> >   'data': {
> >    'cpu': 'CpuInstanceProperties',
> >    'node-id': 'int' } }
> > 
> > This separates "what identifies the CPU slot(s) we are
> > configuring" from "what identifies the node ID we are binding
> > to".
> Doesn't look any simpler to me, I'd better document node-id usage
> in props, like
> 

Well, it still looks simpler and more intuitive to me, but just
because it matches my initial expectations about the semantics of
query-hotpluggable-cpus and CpuInstanceProperties. If your
alternative is very clearly documented (like below), it is not a
problem to me.

>  #
>  # A discriminated record of NUMA options. (for OptsVisitor)
>  #
> +# For 'cpu' type as arguments use a set of cpu properties returned
> +# by query-hotpluggable-cpus[].props, where node-id could be used
> +# to override default node mapping. Since: 2.10
> +#
>  # Since: 2.1
>  ##
>  { 'union': 'NumaOptions',
>    'base': { 'type': 'NumaOptionsType' },
>    'discriminator': 'type',
>    'data': {
> -    'node': 'NumaNodeOptions' }}
> +    'node': 'NumaNodeOptions',
> +    'cpu' : 'CpuInstanceProperties' }}

I worry about not being able to add extra options to "-numa cpu"
in the future without affecting HotpluggableCPU.props too. Being
able to document the semantics of -numa cpu inside a dedicated
NumaCpuOptions struct would be nice too.

I believe this can be addressed by defing "NumaCpuOptions" with
"CpuInstanceProperties" as base:

 { 'union': 'NumaOptions',
   'base': { 'type': 'NumaOptionsType' },
   'discriminator': 'type',
   'data': {
     'node': 'NumaNodeOptions',
     'cpu' : 'NumaCpuOptions' }}

##
# Options for -numa cpu,...
#
# "-numa cpu" accepts the same set of cpu properties returned by
# query-hotpluggable-cpus[].props, where node-id could be used to
# override default node mapping.
#
# Since: 2.10
##
{ 'struct': 'NumaCpuOptions',
  'base': 'CpuInstanceProperties' }

>  


>  ##
>  # @NumaNodeOptions:
> 
> 
> > In case we have trouble making this struct work with QemuOpts, we
> > could do this (temporarily?):
> > 
> > ##
> > # @NumaCpuOptions:
> > #
> > # Mapping of a given CPU (or a set of CPUs) to a NUMA node.
> > #
> > # @cpu: Properties identifying the CPU(s). Use the 'props' field of
> > #       query-hotpluggable-cpus for possible values for this
> > #       field.
> > #       TODO: describe what happens when 'cpu' matches
> > #       multiple slots.
> > # @node-id: NUMA node where the CPUs are going to be located.
> > #
> > # @socket-id: Shortcut for cpu.socket-id, to make this struct
> > #             friendly to QemuOpts.
> > # @core-id: Shortcut for cpu.core-id, to make this struct
> > #           friendly to QemuOpts.
> > # @thread-id: Shortcut for cpu.thread-id, to make this struct
> > #             friendly to QemuOpts.
> > ##
> > { 'struct': 'NumaCpuOptions',
> >   'data': {
> >    '*cpu': 'CpuInstanceProperties',
> >    '*socket-id': 'int',
> >    '*core-id': 'int',
> >    '*thread-id': 'int',
> >    'node-id': 'int' } }
> > 
> > >   6. optionally, set new numa mapping and get updated
> > >      possible cpus list with query-hotpluggable-cpus
> > >   7. optionally, add extra cpus with device_add using updated
> > >      cpus list and get updated cpus list as it's been changed again.
> > >   8. unpause preconfig stage and let qemu continue to execute
> > >      machine_init and the rest.
> > > 
> > > Since we would need to implement QMP configuration for '-device cpu',
> > > we as well might reuse it for custom numa mapping.
> > > 
> > >  [...]  
> > 
> 

-- 
Eduardo