On 11/15/19 2:04 PM, George Dunlap wrote:
> On 11/15/19 1:55 PM, Andrew Cooper wrote:
>> On 15/11/2019 12:39, Jan Beulich wrote:
>>> On 15.11.2019 12:58, George Dunlap wrote:
>>>> On 11/15/19 11:12 AM, Jan Beulich wrote:
>>>>> On 15.11.2019 11:57, George Dunlap wrote:
>>>>>> --- a/tools/libxc/xc_cpuid_x86.c
>>>>>> +++ b/tools/libxc/xc_cpuid_x86.c
>>>>>> @@ -579,52 +579,68 @@ int xc_cpuid_apply_policy(xc_interface *xch,
>>>>>> uint32_t domid,
>>>>>> }
>>>>>> else
>>>>>> {
>>>>>> - /*
>>>>>> - * Topology for HVM guests is entirely controlled by Xen. For
>>>>>> now, we
>>>>>> - * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no
>>>>>> SMT.
>>>>>> - */
>>>>>> - p->basic.htt = true;
>>>>>> + p->basic.htt = false;
>>>>>> p->extd.cmp_legacy = false;
>>>>>>
>>>>>> - /*
>>>>>> - * Leaf 1 EBX[23:16] is Maximum Logical Processors Per Package.
>>>>>> - * Update to reflect vLAPIC_ID = vCPU_ID * 2, but make sure to
>>>>>> avoid
>>>>>> - * overflow.
>>>>>> - */
>>>>>> - if ( !(p->basic.lppp & 0x80) )
>>>>>> - p->basic.lppp *= 2;
>>>>>> -
>>>>> I appreciate you wanting to put all adjustments in a central place, but
>>>>> at least it makes patch review more difficult. How about you latch
>>>>> !getenv("XEN_LIBXC_DISABLE_FAKEHT") into a local boolean at the top of
>>>>> the function and then the above would become
>>>>>
>>>>> if ( !(p->basic.lppp & 0x80) )
>>>>> p->basic.lppp <<= fakeht;
>>>>>
>>>>> and e.g. ...
>>>>>
>>>>>> switch ( p->x86_vendor )
>>>>>> {
>>>>>> case X86_VENDOR_INTEL:
>>>>>> for ( i = 0; (p->cache.subleaf[i].type &&
>>>>>> i < ARRAY_SIZE(p->cache.raw)); ++i )
>>>>>> {
>>>>>> - p->cache.subleaf[i].cores_per_package =
>>>>>> - (p->cache.subleaf[i].cores_per_package << 1) | 1;
>>>>> ... this
>>>>>
>>>>> p->cache.subleaf[i].cores_per_package =
>>>>> (p->cache.subleaf[i].cores_per_package << fakeht) |
>>>>> fakeht;
>>>> I'm afraid I think the code itself would then become more difficult to
>>>> read;
>>> Slightly, but yes.
>>>
>>>> and it seems a bit strange to be architecting our code based on
>>>> limitations of the diff algorithm and/or diff viewer used.
>>> It's not entirely uncommon to (also) consider how the resulting
>>> diff would look like when putting together a change. And besides
>>> the review aspect, there's also the archeology one - "git blame"
>>> yields much more helpful results when code doesn't get moved
>>> around more than necessary. But yes, there's no very clear "this
>>> is the better option" here. I've taken another look at the code
>>> before your change though - everything is already nicely in one
>>> place with Andrew's most recent code reorg. So I'm now having an
>>> even harder time seeing why you want to move things around again.
>>
>> We don't. I've recommend twice now to have a single "else if" hunk
>> which is nearly empty, and much more obviously a gross "make it work for
>> 4.13" bodge.
>
> The results are a tiny bit better, but not much really (see attached).
I mean, if we *really* wanted to optimize for diff readability, we could
use `goto`s instead, but I thought that would be going a bit too far.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
