Re: 32bit resctrl? (was Re: [PATCH v2] fs/resctrl: fix domid loss precision issue)

[Reinette Chatre]

Hi Thomas,

On 3/15/2024 4:32 PM, Thomas Gleixner wrote:
> On Thu, Mar 14 2024 at 08:25, Reinette Chatre wrote:
>>> On some platforms(e.g.,x86), the max cache_id is the amount of L3 caches,
>>> so it is not in the range of 0x3fff. But some platforms use higher
>>> cache_id, e.g., arm uses cache_id as locator for cache MSC. This will
>>> cause below issue if cache_id > 0x3fff likes:
>>> /sys/fs/resctrl/mon_groups/p1/mon_data/mon_L3_1048564 # cat llc_occupancy
>>> cat: read error: No such file or directory
>>>
>>> This is the call trace when cat llc_occupancy:
>>> rdtgroup_mondata_show()
>>> 	domid = md.u.domid
>>> 	d = resctrl_arch_find_domain(r, domid)
>>>
>>> d is null here because of lossing precision
>>>
>>> Signed-off-by: Rex Nie <rex.nie@xxxxxxxxxxxxxxx>
>>> ---
>>>  fs/resctrl/internal.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>>> index 7a6f46b4edd0..096317610949 100644
>>> --- a/fs/resctrl/internal.h
>>> +++ b/fs/resctrl/internal.h
>>> @@ -94,7 +94,7 @@ union mon_data_bits {
>>>  	struct {
>>>  		unsigned int rid		: 10;
>>>  		enum resctrl_event_id evtid	: 8;
>>> -		unsigned int domid		: 14;
>>> +		u32				domid;
>>>  	} u;
>>>  };
>>>  
>>
>> resctrl currently supports 32bit builds. Fixing this issue* in this way
>> would first require that resctrl (the architecture independent fs part)
>> depend on X86_64. Is this a change that everybody will be comfortable
>> with?
> 
> Why? Making mon_data_bits::u larger in the way it has been done does not
> have any dependency on 32 or 64 builds unless I'm missing something.

I should have expanded the diff. The expanded view of current code below
gives more insight into how a pointer is used to store data:

union mon_data_bits {
	void *priv;
	struct {
		unsigned int rid		: 10;
		enum resctrl_event_id evtid	: 8;
		unsigned int domid		: 14;
	} u;
}

> 
>> (Of course, there are other solutions available to address the issue mentioned
>> in this patch that do not require depending on X86_64, but I would like
>> to take this moment to understand the sentiment surrounding continuing support
>> for 32bit resctrl.)
> 
> Caring about 32biit resctrl on x86 is a pointless exercise.
> 

Thank you Thomas. This code is what will soon be moved into the architecture
agnostic "resctrl filesystem". Are there expectations from more generic
interfaces like this regarding 32-bit/64-bit? "resctrl filesystem" is on a
path to support more architectures (x86, Arm, and RISC-V) and I am not familiar
with the architectures and platforms to know what the impact of such a change
would be nor what existing usages there may be for 32-bit builds.

Reinette