|
| From: | wangyanan (Y) |
| Subject: | Re: [PATCH v5 1/4] qapi/machine.json: Add cluster-id |
| Date: | Thu, 14 Apr 2022 17:33:45 +0800 |
| User-agent: | Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 |
On 2022/4/14 15:56, Gavin Shan wrote:
Hi Yanan, On 4/14/22 10:27 AM, wangyanan (Y) wrote:On 2022/4/14 8:06, Gavin Shan wrote: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 toWe 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?Thanks for the explanation. So ARM64 doesn't have die and x86 doesn't have cluster?
At least for now, yes. :) Thanks, Yanan
If so, I need to adjust the description for 'cluster-id' as you suggested in v6:@cluster-id: cluster number within socket the CPU belongs to (since 7.1)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. :) # @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 toI like this scheme, but needs the confirms from Igor and maintainers. Igor and maintainers, please let us know if the scheme is good to have? :)+# @core-id: core number within cluster/die the CPU belongs to # @thread-id: thread number within core the CPU belongs to # -# Note: currently there are 5 properties that could be present +# Note: currently there are 6 properties that could be present # but management should be prepared to pass through other # properties with device_add command to allow for future# interface extension. This also requires the filed names to be kept in@@ -883,6 +884,7 @@ 'data': { '*node-id': 'int', '*socket-id': 'int', '*die-id': 'int', + '*cluster-id': 'int', '*core-id': 'int', '*thread-id': 'int' }Otherwise, looks good to me: Reviewed-by: Yanan Wang <wangyanan55@huawei.com> Please also keep the involved Maintainers on Cc list in next version, an Ack from them is best. :)Thanks again for your time to review. Sure, I will do in next posting.Thanks, Gavin .
| [Prev in Thread] | Current Thread | [Next in Thread] |