Re: [PATCH v9 2/9] x86/resctrl: Prepare for per-ctrl_mon group mba_MBps control

["Luck, Tony" <tony.luck@intel.com>]

On Tue, Nov 19, 2024 at 05:08:42PM -0800, Reinette Chatre wrote:
> Hi Tony,
> 
> On 11/13/24 4:17 PM, Tony Luck wrote:
> > Resctrl uses local memory bandwidth event as input to the feedback
> > loop when the mba_MBps mount option is used. This means that this
> > mount option cannot be used on systems that only support monitoring
> > of total bandwidth.
> > 
> > Prepare to allow users to choose the input event independently for
> > each ctrl_mon group.
> 
Hi Reinette,

> The lack of detail on design and implementation leaves a lot for the
> reader to decipher. For example,
> * the change appears to create a contract that rdtgroup.mba_mbps_event
>   is only valid if mba_sc is enabled, this is "documented" in the
>   structure member comment but not connected to the rest of implementation, not
>   here nor later in series.

I'll add text documenting this to the commit comment here, and also a
comment in the code that defines mba_mbps_default_event.

> * the patch uses *three* different checks to manage new variables:
>   is_mbm_local_enabled(), is_mba_sc(), and supports_mba_mbps(). Reader is
>   left to decipher that all checks are built on is_mbm_local_enabled()
>   and thus it is ok to use these checks before using the value that is only
>   assigned when is_mbm_local_enabled().

With some refactoring I've got that down to just one new additon of
"is_mba_sc()" (protecting the assignment of rdtgrp->mba_mbps_event
in rdtgroup_mkdir_ctrl_mon().

> * clearly mba_mbps_default_event cannot always have a value making reader wonder
>   if enum resctrl_event_id needs a "0", takes some deciphering to get confidence
>   that its assignment when is_mbm_local_enabled() fits under the contract
>   that values are only value when is_mba_sc() and thus any code following contract by
>   first checking for mba_sc should never encounter a 0.

Documented that mba_mbps_default_event remaining "0" is not an issue in
the comment at the definition of mba_mbps_default_event.

> * based on premise of this work reader may consider what happens if
>   system does not support local MBM. more deciphering needed to get confidence
>   that while mba_mbps_default_event will not be set, since is_mba_sc() still
>   depends on local MBM this still fits under contract that mba_mbps_default_event
>   cannot be used in this case.

I think I've cleared up this mystery with commit and code comments.

> Of course, it may just me that needs more help to understand what a patch is doing 
> while having little insight into what it intends to do. I thought by sharing some of
> the questions I felt needed to investigated may give some insight into the difficulty
> a cryptic changelog creates. Review could be helped significantly if the changelog
> provides insight into the design decisions. 

Good point. I've also re-worked the cover letter to provide more
information on the problem and solution implemented by this series.

> ...
> 
> > @@ -3611,6 +3613,8 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
> >  			rdt_last_cmd_puts("kernfs subdir error\n");
> >  			goto out_del_list;
> >  		}
> > +		if (is_mba_sc(NULL))
> > +			rdtgrp->mba_mbps_event = mba_mbps_default_event;
> >  	}
> >  
> >  	goto out_unlock;
> > @@ -3970,6 +3974,8 @@ static void __init rdtgroup_setup_default(void)
> >  	rdtgroup_default.closid = RESCTRL_RESERVED_CLOSID;
> >  	rdtgroup_default.mon.rmid = RESCTRL_RESERVED_RMID;
> >  	rdtgroup_default.type = RDTCTRL_GROUP;
> > +	if (supports_mba_mbps())
> > +		rdtgroup_default.mba_mbps_event = mba_mbps_default_event;
> >  	INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
> >  
> >  	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
> 
> I do not see the default resource group's mba_mbps_event ever being reset. This means
> that if the user mounts resctrl, changes mba_mbps_event, umount resctrl, remount
> resctrl, then the default resource group will not have the default mba_mbps_event
> but whatever was set on previous mount. Is this intended? No mention of this behavior in
> changelog.

Good catch. You are correct that a changed event value in the default
CTRL_MON group is preserved across unmount/remount. This was not
intentional. Moving the initialization of rdtgroup_default.mba_mbps_event
into set_mba_sc() fixes this (with the added benefit of removing the
supports_mba_mbps() check).

> Reinette

-Tony