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

[Gavin Shan]

Hi Daniel,

On 4/19/22 11:59 PM, Daniel P. Berrangé wrote:
> 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.

Agreed and thanks a lot for the elaboration.

> > # @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.

Ok. The description will be included into v7, as v6 was posted
two days ago.

Thanks,
Gavin