[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Wed, 3 Apr 2019 12:33:09 +0100
  • Autocrypt: addr=andrew.cooper3@xxxxxxxxxx; prefer-encrypt=mutual; keydata= mQINBFLhNn8BEADVhE+Hb8i0GV6mihnnr/uiQQdPF8kUoFzCOPXkf7jQ5sLYeJa0cQi6Penp VtiFYznTairnVsN5J+ujSTIb+OlMSJUWV4opS7WVNnxHbFTPYZVQ3erv7NKc2iVizCRZ2Kxn srM1oPXWRic8BIAdYOKOloF2300SL/bIpeD+x7h3w9B/qez7nOin5NzkxgFoaUeIal12pXSR Q354FKFoy6Vh96gc4VRqte3jw8mPuJQpfws+Pb+swvSf/i1q1+1I4jsRQQh2m6OTADHIqg2E ofTYAEh7R5HfPx0EXoEDMdRjOeKn8+vvkAwhviWXTHlG3R1QkbE5M/oywnZ83udJmi+lxjJ5 YhQ5IzomvJ16H0Bq+TLyVLO/VRksp1VR9HxCzItLNCS8PdpYYz5TC204ViycobYU65WMpzWe LFAGn8jSS25XIpqv0Y9k87dLbctKKA14Ifw2kq5OIVu2FuX+3i446JOa2vpCI9GcjCzi3oHV e00bzYiHMIl0FICrNJU0Kjho8pdo0m2uxkn6SYEpogAy9pnatUlO+erL4LqFUO7GXSdBRbw5 gNt25XTLdSFuZtMxkY3tq8MFss5QnjhehCVPEpE6y9ZjI4XB8ad1G4oBHVGK5LMsvg22PfMJ ISWFSHoF/B5+lHkCKWkFxZ0gZn33ju5n6/FOdEx4B8cMJt+cWwARAQABtClBbmRyZXcgQ29v cGVyIDxhbmRyZXcuY29vcGVyM0BjaXRyaXguY29tPokCOgQTAQgAJAIbAwULCQgHAwUVCgkI CwUWAgMBAAIeAQIXgAUCWKD95wIZAQAKCRBlw/kGpdefoHbdD/9AIoR3k6fKl+RFiFpyAhvO 59ttDFI7nIAnlYngev2XUR3acFElJATHSDO0ju+hqWqAb8kVijXLops0gOfqt3VPZq9cuHlh IMDquatGLzAadfFx2eQYIYT+FYuMoPZy/aTUazmJIDVxP7L383grjIkn+7tAv+qeDfE+txL4 SAm1UHNvmdfgL2/lcmL3xRh7sub3nJilM93RWX1Pe5LBSDXO45uzCGEdst6uSlzYR/MEr+5Z JQQ32JV64zwvf/aKaagSQSQMYNX9JFgfZ3TKWC1KJQbX5ssoX/5hNLqxMcZV3TN7kU8I3kjK mPec9+1nECOjjJSO/h4P0sBZyIUGfguwzhEeGf4sMCuSEM4xjCnwiBwftR17sr0spYcOpqET ZGcAmyYcNjy6CYadNCnfR40vhhWuCfNCBzWnUW0lFoo12wb0YnzoOLjvfD6OL3JjIUJNOmJy RCsJ5IA/Iz33RhSVRmROu+TztwuThClw63g7+hoyewv7BemKyuU6FTVhjjW+XUWmS/FzknSi dAG+insr0746cTPpSkGl3KAXeWDGJzve7/SBBfyznWCMGaf8E2P1oOdIZRxHgWj0zNr1+ooF /PzgLPiCI4OMUttTlEKChgbUTQ+5o0P080JojqfXwbPAyumbaYcQNiH1/xYbJdOFSiBv9rpt TQTBLzDKXok86LkCDQRS4TZ/ARAAkgqudHsp+hd82UVkvgnlqZjzz2vyrYfz7bkPtXaGb9H4 Rfo7mQsEQavEBdWWjbga6eMnDqtu+FC+qeTGYebToxEyp2lKDSoAsvt8w82tIlP/EbmRbDVn 7bhjBlfRcFjVYw8uVDPptT0TV47vpoCVkTwcyb6OltJrvg/QzV9f07DJswuda1JH3/qvYu0p vjPnYvCq4NsqY2XSdAJ02HrdYPFtNyPEntu1n1KK+gJrstjtw7KsZ4ygXYrsm/oCBiVW/OgU g/XIlGErkrxe4vQvJyVwg6YH653YTX5hLLUEL1NS4TCo47RP+wi6y+TnuAL36UtK/uFyEuPy wwrDVcC4cIFhYSfsO0BumEI65yu7a8aHbGfq2lW251UcoU48Z27ZUUZd2Dr6O/n8poQHbaTd 6bJJSjzGGHZVbRP9UQ3lkmkmc0+XCHmj5WhwNNYjgbbmML7y0fsJT5RgvefAIFfHBg7fTY/i kBEimoUsTEQz+N4hbKwo1hULfVxDJStE4sbPhjbsPCrlXf6W9CxSyQ0qmZ2bXsLQYRj2xqd1 bpA+1o1j2N4/au1R/uSiUFjewJdT/LX1EklKDcQwpk06Af/N7VZtSfEJeRV04unbsKVXWZAk uAJyDDKN99ziC0Wz5kcPyVD1HNf8bgaqGDzrv3TfYjwqayRFcMf7xJaL9xXedMcAEQEAAYkC HwQYAQgACQUCUuE2fwIbDAAKCRBlw/kGpdefoG4XEACD1Qf/er8EA7g23HMxYWd3FXHThrVQ HgiGdk5Yh632vjOm9L4sd/GCEACVQKjsu98e8o3ysitFlznEns5EAAXEbITrgKWXDDUWGYxd pnjj2u+GkVdsOAGk0kxczX6s+VRBhpbBI2PWnOsRJgU2n10PZ3mZD4Xu9kU2IXYmuW+e5KCA vTArRUdCrAtIa1k01sPipPPw6dfxx2e5asy21YOytzxuWFfJTGnVxZZSCyLUO83sh6OZhJkk b9rxL9wPmpN/t2IPaEKoAc0FTQZS36wAMOXkBh24PQ9gaLJvfPKpNzGD8XWR5HHF0NLIJhgg 4ZlEXQ2fVp3XrtocHqhu4UZR4koCijgB8sB7Tb0GCpwK+C4UePdFLfhKyRdSXuvY3AHJd4CP 4JzW0Bzq/WXY3XMOzUTYApGQpnUpdOmuQSfpV9MQO+/jo7r6yPbxT7CwRS5dcQPzUiuHLK9i nvjREdh84qycnx0/6dDroYhp0DFv4udxuAvt1h4wGwTPRQZerSm4xaYegEFusyhbZrI0U9tJ B8WrhBLXDiYlyJT6zOV2yZFuW47VrLsjYnHwn27hmxTC/7tvG3euCklmkn9Sl9IAKFu29RSo d5bD8kMSCYsTqtTfT6W4A3qHGvIDta3ptLYpIAOD2sY3GYq2nf3Bbzx81wZK14JdDDHUX2Rs 6+ahAA==
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>, Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • Delivery-date: Wed, 03 Apr 2019 11:33:17 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 03/04/2019 11:44, Jan Beulich wrote:
>>>> On 03.04.19 at 12:17, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 03/04/2019 10:33, Jan Beulich wrote:
>>>>>> On 02.04.19 at 21:57, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> Slightly RFC.  I'm not very happy with the contination situation, but 
>>>> -EBUSY
>>>> is the preexisting style and it seems like it is the only option from 
>>>> tasklet
>>>> context.
>>> Well, offloading the re-invocation to the caller isn't really nice.
>>> Looking at the code, is there any reason why couldn't use
>>> the usual -ERESTART / hypercall_create_continuation()? This
>>> would require a little bit of re-work, in particular to allow
>>> passing the vCPU into hypercall_create_continuation(), but
>>> beyond that I can't see any immediate obstacles. Though
>>> clearly I wouldn't make this a prereq requirement for the work
>>> here.
>> The problem isn't really the ERESTART.  We could do some plumbing and
>> make it work, but the real problem is that I can't stash the current cpu
>> index in the sysctl data block across the continuation point.
>>
>> At the moment, the loop depends on, once all CPUs are in the correct
>> state, getting through the for_each_present_cpu() loop without taking a
>> further continuation.
> But these are two orthogonal things: One is how to invoke the
> continuation, and the other is where the continuation is to
> resume from. I think the former is more important to address,
> as it affects how the tools side code needs to look like.

Right, but -EBUSY is consistent with how the single online/offline ops
function at the moment, which is why I reused it here.

>
>>>> +    for_each_present_cpu ( cpu )
>>>> +    {
>>>> +        if ( cpu == 0 )
>>>> +            continue;
>>> Is this special case really needed? If so, perhaps worth a brief
>>> comment?
>> Trying to down cpu 0 is a hard -EINVAL.
> But here we're on the CPU-up path. Plus, for eventually supporting
> the offlining of CPU 0, it would feel slightly better if you used
> smp_processor_id() here.

Are there any processors where you can actually take CPU 0 offline?  Its
certainly not possible on any Intel or AMD CPUs.

While I can appreciate the theoretical end goal, it isn't a reality and
I see no signs of that changing.  Xen very definitely cannot take CPU 0
offline, nor can hardware, and I don't see any value in jumping through
hoops for an end goal which doesn't exist.

>>>> +        if ( cpu >= max_cpus )
>>>> +            break;
>>>> +
>>>> +        if ( x86_cpu_to_apicid[cpu] & sibling_mask )
>>>> +            ret = cpu_up_helper(_p(cpu));
>>> Shouldn't this be restricted to CPUs a sibling of which is already
>>> online? And widened at the same time, to also online thread 0
>>> if one of the other threads is already online?
>> Unfortunately, that turns into a rats nest very very quickly, which is
>> why I gave up and simplified the semantics to strictly "this shall
>> {of,off}line the nonzero siblings threads".
> Okay, if that's the intention, then I can certainly live with this.
> But it needs to be called out at the very least in the public header.
> (It might be worthwhile setting up a flag right away for "full"
> behavior, but leave acting upon it unimplemented). It also wouldn't
> hurt if the patch description already set expectations accordingly.
>
> Then again, considering your "maxcpus=" related question,
> it would certainly be odd for people to see non-zero threads
> come online here when they've intentionally left entire cores
> or nodes offline for whatever reason. Arguably that's not
> something to expect people would commonly do, and hence it
> may not be worth wasting meaningful extra effort on. But as
> above, and such "oddities" should be spelled out, such that it
> can be recognized that they're not oversights.

And we come back to Xen's perennial problem of having no documentation. 
I'll see if I can find some time to put some Sphinx/RST together for this.

As for the maxcpus behaviour, I think that is sufficiently niche to
debugging circumstances only that perhaps we can ignore it.  I certainly
don't expect to see maxcpus= used in production.

~Andrew

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.