Re: [PATCH v9 14/26] x86/resctrl: Introduce interface to display number of free counters

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 11/15/24 18:31, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/29/24 4:21 PM, Babu Moger wrote:
>> Provide the interface to display the number of free monitoring counters
>> available for assignment in each doamin when mbm_cntr_assign is supported.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v9: New patch.
>> ---
>>  Documentation/arch/x86/resctrl.rst     |  4 ++++
>>  arch/x86/kernel/cpu/resctrl/monitor.c  |  1 +
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 33 ++++++++++++++++++++++++++
>>  3 files changed, 38 insertions(+)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index 2f3a86278e84..2bc58d974934 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -302,6 +302,10 @@ with the following files:
>>  	memory bandwidth tracking to a single memory bandwidth event per
>>  	monitoring group.
>>  
>> +"available_mbm_cntrs":
>> +	The number of free monitoring counters available assignment in each domain
> 
> "The number of free monitoring counters available assignment" -> "The number of monitoring
> counters available for assignment"?
> 
> (not taking into account how text may change after addressing Peter's feedback)

How about this?

"The number of monitoring counters available for assignment in each domain
when the architecture supports mbm_cntr_assign mode. There are a total of
"num_mbm_cntrs" counters are available for assignment. Counters can be
assigned or unassigned individually in each domain. A counter is available
for new assignment if it is unassigned in all domains."

> 
>> +	when the architecture supports mbm_cntr_assign mode.
>> +
>>  "max_threshold_occupancy":
>>  		Read/write file provides the largest value (in
>>  		bytes) at which a previously used LLC_occupancy
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 3996f7528b66..e8d38a963f39 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1268,6 +1268,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>  			cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
>>  			r->mon.num_mbm_cntrs = (ebx & GENMASK(15, 0)) + 1;
>>  			resctrl_file_fflags_init("num_mbm_cntrs", RFTYPE_MON_INFO);
>> +			resctrl_file_fflags_init("available_mbm_cntrs", RFTYPE_MON_INFO);
>>  		}
>>  	}
>>  
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 654cdfee1b00..ef0c1246fa2a 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -898,6 +898,33 @@ static int rdtgroup_num_mbm_cntrs_show(struct kernfs_open_file *of,
>>  	return 0;
>>  }
>>  
>> +static int rdtgroup_available_mbm_cntrs_show(struct kernfs_open_file *of,
>> +					     struct seq_file *s, void *v)
>> +{
>> +	struct rdt_resource *r = of->kn->parent->priv;
>> +	struct rdt_mon_domain *dom;
>> +	bool sep = false;
>> +	u32 val;
>> +
>> +	cpus_read_lock();
>> +	mutex_lock(&rdtgroup_mutex);
>> +
>> +	list_for_each_entry(dom, &r->mon_domains, hdr.list) {
>> +		if (sep)
>> +			seq_puts(s, ";");
>> +
>> +		val = r->mon.num_mbm_cntrs - hweight64(*dom->mbm_cntr_map);
> 
> This should probably be bitmap_weight() to address warnings like below that are
> encountered by build testing with various configs (32bit in this case). 0day does
> not seem to automatically pick up patches just based on submission but it sure will
> when these are merged to tip so this needs a clean slate.

Sure.

> 
>>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:916:32: warning: shift count >= width of type [-Wshift-count-overflow]
>      916 |                 val = r->mon.num_mbm_cntrs - hweight64(*dom->mbm_cntr_map);
>          |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    include/asm-generic/bitops/const_hweight.h:29:49: note: expanded from macro 'hweight64'
>       29 | #define hweight64(w) (__builtin_constant_p(w) ? __const_hweight64(w) : __arch_hweight64(w))
>          |                                                 ^~~~~~~~~~~~~~~~~~~~
>    include/asm-generic/bitops/const_hweight.h:21:76: note: expanded from macro '__const_hweight64'
>       21 | #define __const_hweight64(w) (__const_hweight32(w) + __const_hweight32((w) >> 32))
>          |                                                                            ^  ~~
>    include/asm-generic/bitops/const_hweight.h:20:49: note: expanded from macro '__const_hweight32'
>       20 | #define __const_hweight32(w) (__const_hweight16(w) + __const_hweight16((w) >> 16))
>          |                                                 ^
>    include/asm-generic/bitops/const_hweight.h:19:48: note: expanded from macro '__const_hweight16'
>       19 | #define __const_hweight16(w) (__const_hweight8(w)  + __const_hweight8((w)  >> 8 ))
>          |                                                ^
>    include/asm-generic/bitops/const_hweight.h:10:9: note: expanded from macro '__const_hweight8'
>       10 |          ((!!((w) & (1ULL << 0))) +     \
>          |                ^
> 
> 
> Reinette
> 
> 
> 

-- 
Thanks
Babu Moger