Re: [Xen-devel] [PATCH v3 15/28] x86/cpu: Sysctl and common infrastructure for levelling context switching

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.16 at 16:57, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 21/03/16 16:23, Jan Beulich wrote:
>>>>> On 15.03.16 at 16:35, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/cpu/common.c
>>> +++ b/xen/arch/x86/cpu/common.c
>>> @@ -36,6 +36,12 @@ integer_param("cpuid_mask_ext_ecx", 
>>> opt_cpuid_mask_ext_ecx);
>>>  unsigned int opt_cpuid_mask_ext_edx = ~0u;
>>>  integer_param("cpuid_mask_ext_edx", opt_cpuid_mask_ext_edx);
>>>  
>>> +unsigned int __initdata expected_levelling_cap;
>> Especially for an __initdata item to be non-static, its use(s) should
>> be visible in a patch adding such a variable.
> 
> From the commit message:
>> This interface will currently report no capabilities.  This change is
>> scaffolding for future patches, which will introduce detection and switching
>> logic, after which the interface will report hardware capabilities 
> correctly.
> 
> This is specifically to reduce the complexity of the following patches. 
> How much more clearly do you want this stating?

The problem isn't that this wasn't stated, but that (as said) namely
for __initdata objects, which put restrictions on where use of them
is valid, introducing them in one patch and adding uses only later is
difficult to review. Quite a bit more difficult than if such variables
got added only once needed (which "bloats" the affected patch by
just a handful of lines at most).

But anyway, I've got through it, and I'm not insisting on this needing
to change. I merely wanted to point out that while there are cases
where it's reasonable to add unused objects or functions to make
later stuff easier to review, I don't think this is such a case (at least
not as far as the variables here are concerned).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel