Re: [PATCH v18 06/17] x86/resctrl: Introduce snc_nodes_per_l3_cache

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 5/23/24 4:18 PM, Luck, Tony wrote:
>>> So (in arch/x86/kernel/cpu/resctrl/monitor.c)
>>>
>>> static int snc_nodes_per_l3_cache = 1;
>>>
>>> Set and use only in this (arch specific) file.
>>
>> Since this series initializes this value in
>> arch/x86/kernel/cpu/resctrl/core.c it is not clear to
>> me from just this one line how you envision the changes.
> 
> v18 did the initialization in core.c. But since SNC is all about monitor
> features it looks more logical to do this here:
> 
> resctrl_late_init()
>      get_rdt_resources()
> 	get_rdt_mon_resources()
> 	    rdt_get_mon_l3_config()  <<<< Do SNC enumeration here
> 	

ok.

> 
>> Just to be clear ... when I refer to arch specific and
>> filesystem code I am considering how this series integrates with [1]
>> since that is the direction resctrl is headed in.
>> Being "arch specific" thus does not require that it be moved into
>> monitor.c - it could be added to arch/x86/kernel/cpu/resctrl/internal.h
>> where it can remain after the fs/arch split.
> 
> The logical place to convert from logical RMID to physical RMID looks
> to be in __rmid_read(). Just need to pass in the domain pointer (from
> both resctrl_arch_reset_rmid() and resctrl_arch_rmid_read().

This is not obvious to me. Do you need domain pointer to figure out
which node the domain belongs to? It looks to me as though these
calls are already running on a CPU belonging to the domain so perhaps
smp_processor_id() is sufficient?

>> It will be very helpful if you view your series while taking
>> [1] into account. For example, when looking at [1] you will see that
>> mon_event_count() and __mon_event_count() are resctrl filesystem
>> functions. When you consider that it should be clear that adding
>> an arch specific get_node_rmid() between these functions will make
>> the arch/fs split more difficult.
> 
> I'll try to keep that in mind as I rework my series. In v18 my "sum" code
> went into __mon_event_count().  I'll see if I can push that down another
> level.

Thank you

Reinette