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

[Nina Schoetterl-Glausch]

On Tue, 2023-04-04 at 09:31 +0200, Cédric Le Goater wrote:
> On 4/3/23 18:28, Pierre Morel wrote:
> > The topology information are attributes of the CPU and are
> > specified during the CPU device creation.
> > 
> > On hot plug we:
> > - calculate the default values for the topology for drawers,
> >    books and sockets in the case they are not specified.
> > - verify the CPU attributes
> > - check that we have still room on the desired socket
> > 
> > The possibility to insert a CPU in a mask is dependent on the
> > number of cores allowed in a socket, a book or a drawer, the
> > checking is done during the hot plug of the CPU to have an
> > immediate answer.
> > 
> > If the complete topology is not specified, the core is added
> > in the physical topology based on its core ID and it gets
> > defaults values for the modifier attributes.
> > 
> > This way, starting QEMU without specifying the topology can
> > still get some advantage of the CPU topology.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   MAINTAINERS                        |   1 +
> >   include/hw/s390x/cpu-topology.h    |  44 +++++
> >   include/hw/s390x/s390-virtio-ccw.h |   1 +
> >   hw/s390x/cpu-topology.c            | 282 +++++++++++++++++++++++++++++
> >   hw/s390x/s390-virtio-ccw.c         |  22 ++-
> >   hw/s390x/meson.build               |   1 +
> >   6 files changed, 349 insertions(+), 2 deletions(-)
> >   create mode 100644 hw/s390x/cpu-topology.c

[...]

> > +
> > +/**
> > + * 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.
> 
> 
> > +{
> > +    return (cpu->env.drawer_id * s390_topology.smp->books + 
> > cpu->env.book_id) *
> > +           s390_topology.smp->sockets + cpu->env.socket_id;
> > +}

[...]

> > +/**
> > + * 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.
> 
> 
> > +    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.

An alternative to consider would be to define a

struct topology_pos {
    int socket;
    int book;
    int drawer;
};

or similar so you can do

old_socket_entry = s390_socket_nb(cpu->env.topology_pos, smp);

struct topology_pos topology_pos = { socket_id, book_id, drawer_id };
new_socket_entry = s390_socket_nb(topology_pos, smp);

It might also make sense to pass a topology_pos around instead of three ids,
since that is quite common.

> 
> > +    }
> > +
> > +    /* 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;
> > +    }
> > +}

[...]