>> +#include <linux/mod_devicetable.h>
>
> I didn't see the need for this include.
struct x86_cpu_id is defined in this #include file.
>> +static void snc_remap_rmids(int cpu)
>
> While adding the new functions, i see that new function names start with
> resctrl_ prefix. However, we are all not very consistent. Can ypu rename
> this function to resctrl_snc_remap_rmids?
I try to put a subsystem prefix on any global symbols to avoid random
conflicts in other parts of the kernel. But I'm less sure of the value for
static functions and variables that are only visible inside a single ".c"
file.
If it must have a prefix, should it be "intel_" rather than "resctrl_" to
indicate that it is an Intel specific function?
>> +static __init int get_snc_config(void)
>
> Same comment as above.
Same answer.
Reinette: Any opinions on these?
-Tony
On 9/26/23 14:40, Luck, Tony wrote:
>>> +#include <linux/mod_devicetable.h>
>>
>> I didn't see the need for this include.
>
> struct x86_cpu_id is defined in this #include file.
Actually, the file <asm/cpu_device_id.h> already includes the above file.
>
>>> +static void snc_remap_rmids(int cpu)
>>
>> While adding the new functions, i see that new function names start with
>> resctrl_ prefix. However, we are all not very consistent. Can ypu rename
>> this function to resctrl_snc_remap_rmids?
>
> I try to put a subsystem prefix on any global symbols to avoid random
> conflicts in other parts of the kernel. But I'm less sure of the value for
> static functions and variables that are only visible inside a single ".c"
> file.
>
> If it must have a prefix, should it be "intel_" rather than "resctrl_" to
> indicate that it is an Intel specific function?
If you add it it should be resctrl_.
Thanks
Babu
>
>
>>> +static __init int get_snc_config(void)
>>
>> Same comment as above.
>
> Same answer.
>
>
> Reinette: Any opinions on these?
>
> -Tony
Hi Tony,
On 9/26/2023 12:40 PM, Luck, Tony wrote:
>>> +#include <linux/mod_devicetable.h>
>>
>> I didn't see the need for this include.
>
> struct x86_cpu_id is defined in this #include file.
>
>>> +static void snc_remap_rmids(int cpu)
>>
>> While adding the new functions, i see that new function names start with
>> resctrl_ prefix. However, we are all not very consistent. Can ypu rename
>> this function to resctrl_snc_remap_rmids?
>
> I try to put a subsystem prefix on any global symbols to avoid random
> conflicts in other parts of the kernel. But I'm less sure of the value for
> static functions and variables that are only visible inside a single ".c"
> file.
>
> If it must have a prefix, should it be "intel_" rather than "resctrl_" to
> indicate that it is an Intel specific function?
>
>
>>> +static __init int get_snc_config(void)
>>
>> Same comment as above.
>
> Same answer.
>
>
> Reinette: Any opinions on these?
>
I agree with you that a consistent prefix is expected from the global
symbols and a prefix of resctrl_ is indeed the goal as can be seen in
the growing include/linux/resctrl.h. resctrl has no established pattern
for static functions (look at the other static functions in this file
being changed) or even those non-static functions and data
structures shared between the resctrl files (see
arch/x86/kernel/cpu/resctrl/internal.h).
I'm ok with the static functions named snc_remap_rmids() and get_snc_config().
We can surely start a discussion if there is concern about resctrl static
function prefixes but that discussion will have larger scope than
this series.
If we do want to focus on function naming I do think a change that may
benefit this work is to be consistent with verb placement in the function
names. i.e either always have verb first or have a consistent snc_ prefix
followed by a verb. I do not recall if there are x86 requirements in this
regard.
Reinette
>>>> +#include <linux/mod_devicetable.h>
>>>
>>> I didn't see the need for this include.
>>
>> struct x86_cpu_id is defined in this #include file.
>
> Actually, the file <asm/cpu_device_id.h> already includes the above file.
It does today. Will it include it next week when somebody has to
re-arrange things to resolve some #include dependency?
I thought there was a guideline somewhere that says to #include
the files that define the things you use. Not just rely on chains of
#includes. But some simple grep's haven't found that written in
Documentation/*
-Tony