Re: [PATCH v14 08/11] qapi/s390/cpu topology: change-topology monitor command

[Pierre Morel]

On 1/16/23 22:09, Nina Schoetterl-Glausch wrote:
> On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote:
> > The modification of the CPU attributes are done through a monitor
> > commands.
> >
> > It allows to move the core inside the topology tree to optimise
> > the cache usage in the case the host's hypervizor previously
> > moved the CPU.
> >
> > The same command allows to modifiy the CPU attributes modifiers
> > like polarization entitlement and the dedicated attribute to notify
> > the guest if the host admin modified scheduling or dedication of a vCPU.
> >
> > With this knowledge the guest has the possibility to optimize the
> > usage of the vCPUs.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   qapi/machine-target.json |  29 ++++++++
> >   include/monitor/hmp.h    |   1 +
> >   hw/s390x/cpu-topology.c  | 141 +++++++++++++++++++++++++++++++++++++++
> >   hmp-commands.hx          |  16 +++++
> >   4 files changed, 187 insertions(+)
> >
> > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > index 2e267fa458..75b0aa254d 100644
> > --- a/qapi/machine-target.json
> > +++ b/qapi/machine-target.json
> > @@ -342,3 +342,32 @@
> >                      'TARGET_S390X',
> >                      'TARGET_MIPS',
> >                      'TARGET_LOONGARCH64' ] } }
> > +
> > +##
> > +# @change-topology:
> > +#
> > +# @core: the vCPU ID to be moved
> > +# @socket: the destination socket where to move the vCPU
> > +# @book: the destination book where to move the vCPU
> > +# @drawer: the destination drawer where to move the vCPU
> > +# @polarity: optional polarity, default is last polarity set by the guest
> > +# @dedicated: optional, if the vCPU is dedicated to a real CPU
> > +#
> > +# Modifies the topology by moving the CPU inside the topology
> > +# tree or by changing a modifier attribute of a CPU.
> > +#
> > +# Returns: Nothing on success, the reason on failure.
> > +#
> > +# Since: <next qemu stable release, eg. 1.0>
> > +##
> > +{ 'command': 'change-topology',
> > +  'data': {
> > +      'core': 'int',
> > +      'socket': 'int',
> > +      'book': 'int',
> > +      'drawer': 'int',
> > +      '*polarity': 'int',
> > +      '*dedicated': 'bool'
> > +  },
> > +  'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
> > +}
> > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> > index 27f86399f7..15c36bf549 100644
> > --- a/include/monitor/hmp.h
> > +++ b/include/monitor/hmp.h
> > @@ -144,5 +144,6 @@ void hmp_human_readable_text_helper(Monitor *mon,
> >                                       HumanReadableText *(*qmp_handler)(Error 
> > **));
> >   void hmp_info_stats(Monitor *mon, const QDict *qdict);
> >   void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict);
> > +void hmp_change_topology(Monitor *mon, const QDict *qdict);
> >   
> >   #endif
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > index b69955a1cd..0faffe657e 100644
> > --- a/hw/s390x/cpu-topology.c
> > +++ b/hw/s390x/cpu-topology.c
> > @@ -18,6 +18,10 @@
> >   #include "target/s390x/cpu.h"
> >   #include "hw/s390x/s390-virtio-ccw.h"
> >   #include "hw/s390x/cpu-topology.h"
> > +#include "qapi/qapi-commands-machine-target.h"
> > +#include "qapi/qmp/qdict.h"
> > +#include "monitor/hmp.h"
> > +#include "monitor/monitor.h"
> >   
> >   /*
> >    * s390_topology is used to keep the topology information.
> > @@ -203,6 +207,21 @@ static void s390_topology_set_entry(S390TopologyEntry 
> > *entry,
> >       s390_topology.sockets[s390_socket_nb(id)]++;
> >   }
> >   
> > +/**
> > + * s390_topology_clear_entry:
> > + * @entry: Topology entry to setup
> > + * @id: topology id to use for the setup
> > + *
> > + * Clear the core bit inside the topology mask and
> > + * decrements the number of cores for the socket.
> > + */
> > +static void s390_topology_clear_entry(S390TopologyEntry *entry,
> > +                                      s390_topology_id id)
> > +{
> > +    clear_bit(63 - id.core, &entry->mask);
>
> This doesn't take the origin into account.

Yes, this error has been done everywhere, I modify this.

> > +    s390_topology.sockets[s390_socket_nb(id)]--;
>
> I suppose this function cannot run concurrently, so the same CPU doesn't get 
> removed twice.

This is only called on a change of topology and the qapi commands are 
serialized by the BQL.

> > +}
> > +
> >   /**
> >    * s390_topology_new_entry:
> >    * @id: s390_topology_id to add
> > @@ -383,3 +402,125 @@ void s390_topology_set_cpu(MachineState *ms, S390CPU 
> > *cpu, Error **errp)
> >   
> >       s390_topology_insert(id);
> >   }
> > +
> > +/*
> > + * qmp and hmp implementations
> > + */
> > +
> > +static S390TopologyEntry *s390_topology_core_to_entry(int core)
> > +{
> > +    S390TopologyEntry *entry;
> > +
> > +    QTAILQ_FOREACH(entry, &s390_topology.list, next) {
> > +        if (entry->mask & (1UL << (63 - core))) {
>
> origin here also.

yes

> > +            return entry;
> > +        }
> > +    }
> > +    return NULL;
>
> This should not return NULL unless the core id is invalid.

yes

> Might be better to validate that somewhere else.

How?
I can use CPU_FOREACH() but it would be quite the same and I will need 
to rerun a loop for setting the topology anyway.

> > +}
> > +
> > +static void s390_change_topology(Error **errp, int64_t core, int64_t socket,
> > +                                 int64_t book, int64_t drawer,
> > +                                 int64_t polarity, bool dedicated)
> > +{
> > +    S390TopologyEntry *entry;
> > +    s390_topology_id new_id;
> > +    s390_topology_id old_id;
> > +    Error *local_error = NULL;
>
> I think you could use ERRP_GUARD here also.

You are right, I could.
Should I ?
I do not need it and find it obfuscating but if you insist I can do it.

> > +
> > +    /* Get the old entry */
> > +    entry = s390_topology_core_to_entry(core);
> > +    if (!entry) {
> > +        error_setg(errp, "No core %ld", core);
> > +        return;
> > +    }
> > +
> > +    /* Compute old topology id */
> > +    old_id = entry->id;
> > +    old_id.core = core;
> > +
> > +    /* Compute new topology id */
> > +    new_id = entry->id;
> > +    new_id.core = core;
> > +    new_id.socket = socket;
> > +    new_id.book = book;
> > +    new_id.drawer = drawer;
> > +    new_id.p = polarity;
> > +    new_id.d = dedicated;
> > +    new_id.type = S390_TOPOLOGY_CPU_IFL;
> > +
> > +    /* Same topology entry, nothing to do */
> > +    if (entry->id.id == new_id.id) {
> > +        return;
> > +    }
> > +
> > +    /* Check for space on the socket if ids are different */
> > +    if ((s390_socket_nb(old_id) != s390_socket_nb(new_id)) &&
> > +        (s390_topology.sockets[s390_socket_nb(new_id)] >=
> > +         s390_topology.smp->sockets)) {
> > +        error_setg(errp, "No more space on this socket");
> > +        return;
> > +    }
> > +
> > +    /* Verify the new topology */
> > +    s390_topology_check(&local_error, new_id);
> > +    if (local_error) {
> > +        error_propagate(errp, local_error);
> > +        return;
> > +    }
> > +
> > +    /* Clear the old topology */
> > +    s390_topology_clear_entry(entry, old_id);
> > +
> > +    /* Insert the new topology */
> > +    s390_topology_insert(new_id);
> > +
> > +    /* Remove unused entry */
> > +    if (!entry->mask) {
> > +        QTAILQ_REMOVE(&s390_topology.list, entry, next);
> > +        g_free(entry);
> > +    }
> > +
> > +    /* Advertise the topology change */
> > +    s390_cpu_topology_set();
> > +}
> > +
> > +void qmp_change_topology(int64_t core, int64_t socket,
> > +                         int64_t book, int64_t drawer,
> > +                         bool has_polarity, int64_t polarity,
> > +                         bool has_dedicated, bool dedicated,
> > +                         Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +
> > +    if (!s390_has_topology()) {
> > +        error_setg(&local_err, "This machine doesn't support topology");
> > +        return;
> > +    }
>
> Do you really want to ignore has_polarity and has_dedicated here?
> What happens in this case?

no, seems I forgot to check it.

Thanks,

Regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen