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
