Re: [PATCH v2 3/6] fs/resctrl: Make 'event_filter' files read only if they're not configurable

[Ben Horgan]

Hi Reinette,

On 3/16/26 17:29, Reinette Chatre wrote:
> Hi Ben,
> 
> On 3/16/26 10:02 AM, Ben Horgan wrote:
>> Hi Tony,
>>
>> On 3/16/26 16:27, Luck, Tony wrote:
>>>>> Instead of making the file writable, and then fixing the mode. Maybe
>>>>> patch the mode in res_common_files[] before calling rdtgroup_add_files():
>>>>
>>>>
>>>> I initially rejected this idea because res_common_files[] is global and the
>>>> read/write choice is per event type but we can safely save restore the mode as
>>>> we're holding the rdtgroup mutex. How about this?
>>>
>>> That seems more complicated. Is there an expectation that event_filter
>>> needs to be writable on the "total" event but not on the "local" one?
>>
>> No, at least not currently. For mpam only the total event is supported.
>>
>> Another way of doing it could be to patch the mode in res_common_files[] from
>> resctrl_mkdir_event_configs() for both cases, 0444 and 0644, and change the
>> starting value to something invalid, e.g. 0.
> 
> "event_filter"'s mode could be initialized similar to its fflags. Please see
> resctrl_l3_mon_resource_init().

Ok, this works if we shift to the rdt_resource::resctrl_mon::mbm_cntr_configurable
property you suggest below as that is scoped to all mbm events.

> 
> Stepping back, based on the v1 discussion I understand that BMEC does not
> apply to MPAM so patching the BMEC specific resctrl_arch_is_evt_configurable() 
> in patch #1 to support MPAM does not look right. From what I can tell we need a
> new "configurable" property that is specific to assignable counters. I understand
> naming is not ideal here but forcing the two independent features together like this
> just because resctrl_arch_is_evt_configurable() has a generically relevant name is
> not right.

Sure, we can consider them separately.

> 
> Consider, for example, a new rdt_resource::resctrl_mon::mbm_cntr_configurable
> property that arch can set. From resctrl fs side it would be a property that is
> only considered if rdt_resource::resctrl_mon::mbm_cntr_assignable == true.
> > res_common_files[] can initialize the default mode of "event_filter" to be
> read-only and resctrl_l3_mon_resource_init() can be expanded to to make
> "event_filter" writable if both rdt_resource::resctrl_mon::mbm_cntr_assignable == true
> AND rdt_resource::resctrl_mon::mbm_cntr_configurable == true.

Seems reasonable. I'll give this a go.

> 
> Looking at this we do seem to have options between resctrl fs properties
> and arch callbacks to do this and considering your earlier concern with
> callbacks I wonder if resctrl fs properties would be better?
> How about another new property, rdt_resource::resctrl_mon::mbm_cntr_assign_fixed
> to replace resctrl_arch_mbm_cntr_assign_fixed()?

As we've already got mbm_cntr_assignable and mbm_assign_on_mkdir in resctrl_mon
it won't seem out of place. I'll update this too.

> 
> Reinette

Thanks,

Ben