Re: [PATCH v6 5/8] fs/resctrl: Add user interface to enable/disable io_alloc feature

[Moger, Babu]

Hi Reinette,

On 6/20/2025 10:53 AM, Reinette Chatre wrote:
> 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?

I don't see any issue. It should be fine. Will test and verify it.

thanks
Babu