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

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

Hi James,

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@amd.com> wrote:
>>> On 3/14/2024 10:25 AM, Reinette Chatre wrote:
>>>> +x86 maintainers, Tony, Babu, Peter
>>>>
>>>> Hi Everybody,
>>>>
>>>> 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?

Reinette


[1] https://ark.intel.com/content/www/us/en/ark/products/95594/intel-celeron-processor-j3455-2m-cache-up-to-2-3-ghz.html