Re: [PATCH v19 02/21] s390x/cpu topology: add topology entries on CPU hotplug

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux