Re: [PATCH v7 10/13] selftests/resctrl: Add vendor detection mechanism

[Andrà Przywara]

On 14/05/2019 18:20, James Morse wrote:

Hi James,

> (thanks for digging into this!)
> 
> On 10/05/2019 18:39, Andre Przywara wrote:
>> On Sat,  9 Feb 2019 18:50:39 -0800
>> Fenghua Yu <fenghua.yu@xxxxxxxxx> wrote:
>>> From: Babu Moger <babu.moger@xxxxxxx>
>>>
>>> RESCTRL feature is supported both on Intel and AMD now. Some features
>>> are implemented differently. Add vendor detection mechanism. Use the vendor
>>> check where there are differences.
>>
>> I don't think vendor detection is the right approach. The Linux userland
>> interface should be even architecture agnostic, not to speak of different
>> vendors.
>>
>> But even if we need this for some reason ...
> 
>>> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
>>> index 3959b2b0671a..1d9adcfbdb4c 100644
>>> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
>>> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
>>> @@ -14,6 +14,66 @@
>>>  #define BENCHMARK_ARGS		64
>>>  #define BENCHMARK_ARG_SIZE	64
>>>  
>>> +/**
>>> + * This variables provides information about the vendor
>>> + */
>>> +int genuine_intel;
>>> +int authentic_amd;
>>> +
>>> +void lcpuid(const unsigned int leaf, const unsigned int subleaf,
>>> +	    struct cpuid_out *out)
>>
>> There is a much easier way to detect the vendor, without resorting to
>> (unchecked) inline assembly in userland:
> 
>> I changed this to scan /proc/cpuinfo for a line starting with vendor_id,
>> then use the information there. This should work everywhere.
> 
> Everywhere x86. /proc/cpuinfo is unfortunately arch specific, arm64 spells that field 'CPU
> implementer'. Short of invoking lscpu, I don't know a portable way of finding this string.

Well, what I meant is: It works everywhere to tell whether this box has
an x86 AMD CPU. Yes, it probably won't match anything on other
architectures, but that's fine, as at least it will compile and won't crash.

> What do we need it for? Surely it indicates something is wrong with the kernel interface
> if you need to know which flavour of CPU this is.

As you mentioned, we should not need it. I just couldn't find a better
way (yet) to differentiate between L3 cache ID and physical package ID
(see patch 11/13). So this is a kludge for now to not break this
particular code.

Out of curiosity: Is there any userland tool meant to control the
resources? I guess poking around in sysfs is not how admins are expected
to use this?
This tool would surely run into the same problems, which somewhat tell
me that the interface is not really right.

Cheers,
Andre.

> The only differences I've spotted are the 'MAX_MBA_BW' on Intel is a percentage, on AMD
> its a number between 1 and 2K. If user-space needs to know we could add another file with
> the 'max_bandwidth'. (I haven't spotted how the MBA default_ctrl gets exposed).
> 
> The other one is the non-contiguous CBM values, where user-space is expected to try it and
> see [0]. If user-space needs to know, we could expose some 'sparse_bitmaps=[0 1]', unaware
> user-space keeps working.
> 
> 
> Thanks,
> 
> James
> 
> [0] https://lore.kernel.org/patchwork/patch/991236/#1179944
>