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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/04/2019 13:14, Jan Beulich wrote:
>>>> On 08.04.19 at 19:02, <andrew.cooper3@citrix.com> wrote:
>> Currently, a user can in combine the output of `xl info -n`, the APCI tables,
> Stray "in"?

Oh yes.  I think I first phrased this as "can in principle combine",
then changed my mind.

>
>> 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.

I'll see about extending the comments.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Hypervisor side
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> 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?

I specifically wanted one of the bitmap representations, so you can
easily identify the layout.  It also makes it very obvious when maxcpu=
or other (un)plugging has gone on.

We can switch to %*pb.  I haven't tested this yet on a large enough
system to notice an issue, due to all the other bugs it has uncovered.

~Andrew

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