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

[James Morse]

Hi Reinette,

On 15/03/2024 22:42, Reinette Chatre wrote:
> On 3/15/2024 11:00 AM, James Morse wrote:
>> On 15/03/2024 16:56, Peter Newman wrote:
>>> On Fri, Mar 15, 2024 at 9:17 AM Moger, Babu <bmoger@xxxxxxx> wrote:
>>>> On 3/14/2024 10:25 AM, Reinette Chatre wrote:
>>>>> On 3/12/2024 12:53 AM, Rex Nie wrote:
>>>>>> 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
>>>>
>>>> I have never bothered about 32bit builds.   Is Intel still testing 32bit
>>>> builds?
>>>
>>> I can confirm we don't have any 32-bit builds.
>>>
>>>
>>>> The structure pointer "union mon_data_bits priv;" is created in stack
>>>> and passed to create mondata directory. We are reading it later again in
>>>> rdtgroup_mondata_show.
>>>>
>>>> How is this pointer valid again?  Shouldn't we use static pointer or
>>>> allocate memory for the pointer?
>>>
>>> The union is copied by value into the pointer-sized field, hence the
>>> need for pointers to be large enough to hold this value.
>>
>> Couldn't we allocate the memory for a structure to hold the values we want, then use the
>> pointer as a pointer?
> 
> Yes, there are a couple of different ways to solve this that remains friendly
> to 32-bit. My goal with this thread was to gauge the sentiment surrounding
> continuing support for 32-bit resctrl.
> 
>> I suspect whether 32bit builds are important depends on if anyone is using it, which we
>> can't really know. Debian has 32bit builds, and while its unlikely anyone runs that on a
>> server part, whatever an "Intel Celeron J3455" is supports RDT too. I'd be keen not to
>> break it!
> 
> You are right. We can't really know. My question did not yet receive a request to
> keep 32-bit support so this will remain uncertain but I am starting to get a sense that
> folks may not be using these builds. I do not think that the issue that Rex reported 
> warrants such a direction change so I am ok to delay considering moving to 64-bit only
> and try to keep 32-bit in mind in future work. I have not been testing 32-bit builds though.
> 
> (btw ... "Intel Celeron J3455" details can be seen at [1]. It is a great (64-bit)
> platform that I worked with for a while and it supports cache pseudo-locking well.)
> 
>> As for these eye-sore-ids ... I'm in two minds as to whether we should clean them up in
>> the kernel. It would be fairly straightforward to scan the PPTT to find them all and map
>> them to 0,1,2,. But this loses the values provided by the vendor.
>> x86 and arm64:device-tree systems generate them, so its not clear that user-space needs a
>> value provided by the vendor here.


> Another alternative: if I counted right it seems that Arm would need 24bits for these
> IDs? That still leaves 8 bits for the resource ID (current max 4) and event ID (current max 3).
> How many resources and events are on the horizon for Arm?

No where near that many! - but its impossible for me to know. MPAM is a bag of bits, its
up to Arm's partners what they build with it. MPAM does have 8 bit counter id,s but there
are only two defined so far. The other fields depend on what people build.

The problem here is the folk who generate the ACPI tables have seen a 32bit field, and
generated a value based on the topology so they know its always unique - instead of taking
a description of the two or three caches that need an ID.

I'll have a go at compressing them into a smaller range in the kernel. As no other
platform preserves a hardware ID here, I don't think its a major loss to re-create these.
It's certainly going to be less ugly to user-space!


Thanks,

James