Hi Babu,
On 6/19/25 11:41 AM, Moger, Babu wrote:
> On 6/17/25 22:59, Reinette Chatre wrote:
>> On 6/11/25 2:23 PM, Babu Moger wrote:
...
>>> + */
>>> +static int resctrl_io_alloc_closid_get(struct rdt_resource *r)
>>> +{
>>> + int num_closids = closids_supported();
>>> +
>>> + if (resctrl_arch_get_cdp_enabled(r->rid))
>>> + num_closids *= 2;
>>> +
>>> + if (num_closids != resctrl_arch_get_num_closid(r))
>>> + return -ENOSPC;
>>> +
>>> + return closids_supported() - 1;
>>> +}
>>
>> resctrl_io_alloc_closid_get() seems to be trying to do two things:
>> - determine what the io_alloc_closid is
>> - make sure the io_alloc_closid is supported
>>
>> I think this should be split into two functions. Once the
>> io_alloc_closid is determined to be supported and io_alloc
>> enabled then there is no reason to keep checking if it is
>> supported whenever the io_alloc_closid is queried.
>>
>> How about simplifying this to:
>>
>> /*
>> * note how this returns u32 that will eliminate
>> * unnecessary error checking in usages where io_alloc_closid
>> * needs to be determined after an resctrl_arch_get_io_alloc_enabled(r)
>> * already confirmed io_alloc is enabled
>> * function comment could note that this returns the CLOSID
>> * required by io_alloc but not whether the CLOSID can
>> * be supported, for this resctrl_io_alloc_closid_supported() should
>> * be used.
>> * Can also note that returned value will always be valid if
>> * resctrl_arch_get_io_alloc_enabled(r) is true.
>> */
>> u32 resctrl_io_alloc_closid(struct rdt_resource *r) {
>> if (resctrl_arch_get_cdp_enabled(r->rid))
>> return resctrl_arch_get_num_closid(r)/2 - 1
>> else
>> return resctrl_arch_get_num_closid(r) -1
>> }
>>
>> /*
>> * note how below already makes resctrl's io_alloc implementation
>> * more generic
>> */
>> resctrl_io_alloc_closid_supported(u32 io_alloc_closid) {
>> return io_alloc_closid < closids_supported()
>> }
>>
>
> Sure.
> Changed the check to
>
> return io_alloc_closid == (closids_supported() -1)
>
resctrl_io_alloc_closid_supported() is not intended to reflect what the
value is but just check if provided value is supported. By changing the
check to above resctrl_io_alloc_closid_supported() does two things again
(what the move to new functions aimed to avoid): checking that the CLOSID
is supported while requiring that it is the highest supported CLOSID.
What issue(s) do you see with using "io_alloc_closid < closids_supported()"
as the check?
Reinette
