Re: [Xen-devel] [PATCH v2] x86/smt: Support for enabling/disabling SMT at runtime

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.04.19 at 19:02, <andrew.cooper3@xxxxxxxxxx> wrote:
> Currently, a user can in combine the output of `xl info -n`, the APCI tables,

Stray "in"?

> and some manual CPUID data to figure out which CPU numbers to feed into
> `xen-hptool cpu-offline` to effectively disable SMT at runtime.
> 
> A more convenient option is to teach Xen how to perform this action.
> 
> Extend XEN_SYSCTL_cpu_hotplug with two new operations.  Introduce a new
> smt_up_down_helper() which wrap the cpu_{up,down}_helper() helpers with logic
> which understands siblings based on their APIC_ID.
> 
> Add libxc stubs, and extend xen-hptool with smt-{enable,disable} options.
> These are intended to be shorthands for a loop over cpu-{online,offline}.  It
> is intended for use in production scenarios where debugging options such as
> `maxcpus=` or other manual plug/unplug configuration has not been used.

I'm still missing any mention of the restrictions of the new sub-ops.
I do notice that they're described in the public header comment now,
but perhaps at least briefly mentioning this here would be helpful.
While in the public header it could also be made more clear that the
restrictions are a choice of implementation, it being the sysctl one
(and hence changeable at any time), I don't mind the chosen
wording.

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Hypervisor side
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one further remark:

> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -114,6 +114,48 @@ long cpu_down_helper(void *data)
>      return ret;
>  }
>  
> +static long smt_up_down_helper(void *data)
> +{
> +    bool up = (bool)data;
> +    unsigned int cpu, sibling_mask = boot_cpu_data.x86_num_siblings - 1;
> +    int ret = 0;
> +
> +    opt_smt = up;
> +
> +    for_each_present_cpu ( cpu )
> +    {
> +        /* Skip primary siblings (those whose thread id is 0). */
> +        if ( !(x86_cpu_to_apicid[cpu] & sibling_mask) )
> +            continue;
> +
> +        ret = up ? cpu_up_helper(_p(cpu))
> +                 : cpu_down_helper(_p(cpu));
> +
> +        if ( ret && ret != -EEXIST )
> +            break;
> +
> +        /*
> +         * Ensure forward progress by only considering preemption when we 
> have
> +         * changed the state of one or more cpus.
> +         */
> +        if ( ret != -EEXIST && general_preempt_check() )
> +        {
> +            /* In tasklet context - can't create a contination. */
> +            ret = -EBUSY;
> +            break;
> +        }
> +
> +        ret = 0; /* Avoid exiting with -EEXIST in the success case. */
> +    }
> +
> +    if ( !ret )
> +        printk(XENLOG_INFO "SMT %s - online CPUs {%*pbl}\n",
> +               up ? "enabled" : "disabled",
> +               nr_cpu_ids, cpumask_bits(&cpu_online_map));

While %pbl is likely going to produce more readable output for
the "up" case, I'm afraid the "down" case will produce pretty
long a line on larger systems. Perhaps better to use %pb in
both cases, or alternatively simply log the number of active
CPUs?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel