On 09/04/2019 13:14, Jan Beulich wrote:
>>>> On 08.04.19 at 19:02, <andrew.coop...@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.coop...@citrix.com>
> Hypervisor side
> Reviewed-by: Jan Beulich <jbeul...@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
