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

[Pierre Morel]

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