Re: [PATCH v5 1/4] qapi/machine.json: Add cluster-id

[Daniel P . Berrangé]

On Thu, Apr 14, 2022 at 10:27:25AM +0800, wangyanan (Y) wrote:
> Hi Gavin,
> 
> Cc: Daniel and Markus
> On 2022/4/14 8:06, Gavin Shan wrote:
> > Hi Yanan,
> > 
> > On 4/13/22 7:49 PM, wangyanan (Y) wrote:
> > > On 2022/4/3 22:59, Gavin Shan wrote:
> > > > This adds cluster-id in CPU instance properties, which will be used
> > > > by arm/virt machine. Besides, the cluster-id is also verified or
> > > > dumped in various spots:
> > > > 
> > > >    * hw/core/machine.c::machine_set_cpu_numa_node() to associate
> > > >      CPU with its NUMA node.
> > > > 
> > > >    * hw/core/machine.c::machine_numa_finish_cpu_init() to associate
> > > >      CPU with NUMA node when no default association isn't provided.
> > > > 
> > > >    * hw/core/machine-hmp-cmds.c::hmp_hotpluggable_cpus() to dump
> > > >      cluster-id.
> > > > 
> > > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > > > ---
> > > >   hw/core/machine-hmp-cmds.c |  4 ++++
> > > >   hw/core/machine.c          | 16 ++++++++++++++++
> > > >   qapi/machine.json          |  6 ++++--
> > > >   3 files changed, 24 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
> > > > index 4e2f319aeb..5cb5eecbfc 100644
> > > > --- a/hw/core/machine-hmp-cmds.c
> > > > +++ b/hw/core/machine-hmp-cmds.c
> > > > @@ -77,6 +77,10 @@ void hmp_hotpluggable_cpus(Monitor *mon,
> > > > const QDict *qdict)
> > > >           if (c->has_die_id) {
> > > >               monitor_printf(mon, "    die-id: \"%" PRIu64
> > > > "\"\n", c->die_id);
> > > >           }
> > > > +        if (c->has_cluster_id) {
> > > > +            monitor_printf(mon, "    cluster-id: \"%" PRIu64 "\"\n",
> > > > +                           c->cluster_id);
> > > > +        }
> > > >           if (c->has_core_id) {
> > > >               monitor_printf(mon, "    core-id: \"%" PRIu64
> > > > "\"\n", c->core_id);
> > > >           }
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index d856485cb4..8748b64657 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -677,6 +677,11 @@ void machine_set_cpu_numa_node(MachineState
> > > > *machine,
> > > >               return;
> > > >           }
> > > > +        if (props->has_cluster_id && !slot->props.has_cluster_id) {
> > > > +            error_setg(errp, "cluster-id is not supported");
> > > > +            return;
> > > > +        }
> > > > +
> > > >           if (props->has_socket_id && !slot->props.has_socket_id) {
> > > >               error_setg(errp, "socket-id is not supported");
> > > >               return;
> > > > @@ -696,6 +701,11 @@ void machine_set_cpu_numa_node(MachineState
> > > > *machine,
> > > >                   continue;
> > > >           }
> > > > +        if (props->has_cluster_id &&
> > > > +            props->cluster_id != slot->props.cluster_id) {
> > > > +                continue;
> > > > +        }
> > > > +
> > > >           if (props->has_die_id && props->die_id !=
> > > > slot->props.die_id) {
> > > >                   continue;
> > > >           }
> > > > @@ -990,6 +1000,12 @@ static char *cpu_slot_to_string(const
> > > > CPUArchId *cpu)
> > > >           }
> > > >           g_string_append_printf(s, "die-id: %"PRId64,
> > > > cpu->props.die_id);
> > > >       }
> > > > +    if (cpu->props.has_cluster_id) {
> > > > +        if (s->len) {
> > > > +            g_string_append_printf(s, ", ");
> > > > +        }
> > > > +        g_string_append_printf(s, "cluster-id: %"PRId64,
> > > > cpu->props.cluster_id);
> > > > +    }
> > > >       if (cpu->props.has_core_id) {
> > > >           if (s->len) {
> > > >               g_string_append_printf(s, ", ");
> > > > diff --git a/qapi/machine.json b/qapi/machine.json
> > > > index 9c460ec450..ea22b574b0 100644
> > > > --- a/qapi/machine.json
> > > > +++ b/qapi/machine.json
> > > > @@ -868,10 +868,11 @@
> > > >   # @node-id: NUMA node ID the CPU belongs to
> > > >   # @socket-id: socket number within node/board the CPU belongs to
> > > >   # @die-id: die number within socket the CPU belongs to (since 4.1)
> > > > -# @core-id: core number within die the CPU belongs to
> > > > +# @cluster-id: cluster number within die the CPU belongs to
> We also need a "(since 7.1)" tag for cluster-id.
> > > I remember this should be "cluster number within socket..."
> > > according to Igor's comments in v3 ?
> > 
> > Igor had suggestion to correct the description for 'core-id' like
> > below, but he didn't suggest anything for 'cluster-id'. The question
> > is clusters are sub-components of die, instead of socket, if die
> > is supported? You may want to me change it like below and please
> > confirm.
> > 
> >   @cluster-id: cluster number within die/socket the CPU belongs to
> > 
> > suggestion from Ignor in v3:
> > 
> >    > +# @core-id: core number within cluster the CPU belongs to
> > 
> >    s:cluster:cluster/die:
> > 
> We want "within cluster/die" description for core-id because we
> support both "cores in cluster" for ARM and "cores in die" for X86.
> Base on this routine, we only need "within socket" for cluster-id
> because we currently only support "clusters in socket". Does this
> make sense?
> 
> Alternatively, the plainest documentation for the IDs is to simply
> range **-id only to its next level topo like below. This may avoid
> increasing complexity when more topo-ids are inserted middle.
> But whether this way is acceptable is up to the Maintainers. :)

Rather saying we only support cluster on ARM and only dies on x86,
I tend to view it as, we only support greater than 1 cluster on
ARM, and greater than 1 die on x86.

IOW, x86 implicitly always has exactly 1-and-only-1 cluster,
and arm implicitly always has exactly 1-and-only-1 die.

With this POV, then we can keep the description simple, only
refering to the immediately above level in the hierarchy.

> 
> # @socket-id: socket number within node/board the CPU belongs to
> # @die-id: die number within socket the CPU belongs to (since 4.1)
> # @cluster-id: cluster number within die the CPU belongs to (since 7.1)
> # @core-id: core number within cluster the CPU belongs to
> # @thread-id: thread number within core the CPU belongs to

So this suggested text is fine with me.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|