Hi Tony,
On 11/12/24 4:53 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?
>
> I think I misunderstood you. I though you said the these mba_MBps_event files
> would be created even if monitoring is not supported,
>
> But now I wonder what you mean by "all CTRL groups".
>
>>> 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?
>
> The directories which need this new file are the same ones that get a "schemata" file.
Only support for control is required for "schemata" to be created. Monitoring support is not
required for "schemata" to be created. This new file requires both monitoring and control support.
>
> The initialization of fflags for the schemata file just uses RFTYPE_CTRL_BASE:
>
> {
> .name = "schemata",
> .mode = 0644,
> .kf_ops = &rdtgroup_kf_single_ops,
> .write = rdtgroup_schemata_write,
> .seq_show = rdtgroup_schemata_show,
> .fflags = RFTYPE_CTRL_BASE,
> },
>
> I don't see any files using .fflags = "RFTYPE_CTRL_BASE | RFTYPE_MON_BASE"
>
I do not think there is a precedent for this case where a file requires both control and
monitoring support.
Reinette
