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 <gs...@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 :|
