Re: [PATCH v8 5/7] x86/resctrl: Add "mba_MBps_event" file to ctrl_mon directories

[Reinette Chatre]

Hi Tony,

On 11/12/24 3:42 PM, Luck, Tony wrote:
>>> +static int set_mba_sc(bool mba_sc)
>>> +{
>>> +   struct rftype *rft;
>>> +
>>> +   rft = rdtgroup_get_rftype_by_name("mba_MBps_event");
>>> +   if (rft)
>>> +           rft->fflags = enable ? RFTYPE_CTRL_BASE : 0;
>>
>> I think this sets this file to be created for all CTRL groups, even when not supporting
>> monitoring?
> 
> No. The calling sequence is:
> 
> rdt_get_tree()
>     rdt_enable_ctx()
> 
>         if (ctx->enable_mba_mbps) {
>                 ret = set_mba_sc(true);
>                 if (ret)
>                         goto out_cdpl3;
>         }
> 
> So set_mba_sc() is only called when the mba_MBps mount option has been used. So
> mba_mbps_event_init() isn't called.
> 
> Note that on unmount of the resctrl file system rdt_kill_sb() calls rdt_disable_ctx()
> which calls set_mba_sc(false) which will clear rft->fflags on systems which support
> mba_MBps.

It sounds as though you are saying that setting the wrong flags are ok as long as it is
done in some specific calling sequence. Is this correct? Relying on calling sequence
instead of correct flags requires more in-depth knowledge of code flows and makes code
harder to maintain.
Why not just make this "RFTYPE_CTRL_BASE | RFTYPE_MON_BASE" to make it clear that the file
applies to CTRL_MON groups? What am I missing?

Reinette