|
From: | Pierre Morel |
Subject: | Re: [PATCH v19 02/21] s390x/cpu topology: add topology entries on CPU hotplug |
Date: | Tue, 4 Apr 2023 13:39:37 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 |
On 4/4/23 09:31, Cédric Le Goater wrote:
On 4/3/23 18:28, Pierre Morel wrote:
[...]
+ +/** + * s390_socket_nb: + * @cpu: s390x CPU + * + * Returns the socket number used inside the cores_per_socket array + * for a cpu. + */ +int s390_socket_nb(S390CPU *cpu)s390_socket_nb() doesn't seem to be used anywhere else than in hw/s390x/cpu-topology.c. It should be static.
right [...]
+/** + * s390_topology_add_core_to_socket: + * @cpu: the new S390CPU to insert in the topology structure + * @drawer_id: new drawer_id + * @book_id: new book_id + * @socket_id: new socket_id+ * @creation: if is true the CPU is a new CPU and there is no old socket+ * to handle.+ * if is false, this is a moving the CPU and old socket count+ * must be decremented. + * @errp: the error pointer + * + */+static void s390_topology_add_core_to_socket(S390CPU *cpu, int drawer_id, + int book_id, int socket_id, + bool creation, Error **errp)+{Since this routine is called twice, in s390_topology_setup_cpu() for creation, and in s390_change_topology() for socket migration, we could duplicate the code in two distinct routines. I think this would simplify a bit each code path and avoid the 'creation' parameter which is confusing.
right Thanks.
+ int old_socket_entry = s390_socket_nb(cpu); + int new_socket_entry; + + if (creation) { + new_socket_entry = old_socket_entry; + } else {+ new_socket_entry = (drawer_id * s390_topology.smp->books + book_id) *+ s390_topology.smp->sockets + socket_id;A helper common routine that s390_socket_nb() could use also would be a plus.
Yes, thanks
+ } + + /* Check for space on new socket */ + if ((new_socket_entry != old_socket_entry) && + (s390_topology.cores_per_socket[new_socket_entry] >= + s390_topology.smp->cores)) { + error_setg(errp, "No more space on this socket"); + return; + } + + /* Update the count of cores in sockets */ + s390_topology.cores_per_socket[new_socket_entry] += 1; + if (!creation) { + s390_topology.cores_per_socket[old_socket_entry] -= 1; + } +} + +/** + * s390_update_cpu_props: + * @ms: the machine state+ * @cpu: the CPU for which to update the properties from the environment.+ * + */ +static void s390_update_cpu_props(MachineState *ms, S390CPU *cpu) +{ + CpuInstanceProperties *props; + + props = &ms->possible_cpus->cpus[cpu->env.core_id].props; + + props->socket_id = cpu->env.socket_id; + props->book_id = cpu->env.book_id; + props->drawer_id = cpu->env.drawer_id; +} + +/** + * s390_topology_setup_cpu: + * @ms: MachineState used to initialize the topology structure on + * first call. + * @cpu: the new S390CPU to insert in the topology structure + * @errp: the error pointer + * + * Called from CPU Hotplug to check and setup the CPU attributes + * before to insert the CPU in the topology.... before the CPU is inserted in the topology.
OK
+ * There is no use to update the MTCR explicitely here because it... is no need ... sounds better.
OK
+ * will be updated by KVM on creation of the new vCPU."CPU" is used everywhere else.
OK
+ */+void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error **errp)+{ + ERRP_GUARD(); + + /* + * We do not want to initialize the topology if the cpu model + * does not support topology, consequently, we have to wait for + * the first CPU to be realized, which realizes the CPU model + * to initialize the topology structures. + * + * s390_topology_setup_cpu() is called from the cpu hotplug. + */ + if (!s390_topology.cores_per_socket) { + s390_topology_init(ms); + } + + s390_topology_cpu_default(cpu, errp); + if (*errp) {May be having s390_topology_cpu_default() return a bool would be cleaner. Same comment for the routines below. This is minor.
Yes and it is more readable. I do it. Thanks for the comments. Regards, Pierre
[Prev in Thread] | Current Thread | [Next in Thread] |