linux-kernel - Re: [PATCH v8 5/7] x86/resctrl: Add "mba_MBps_event" file to ctrl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1f7938a9-d0ac-44a5-a2d0-db9739ef6de9@intel.com>
Date: Tue, 12 Nov 2024 18:54:19 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Luck, Tony" <tony.luck@...el.com>, "Yu, Fenghua" <fenghua.yu@...el.com>,
	Peter Newman <peternewman@...gle.com>, Jonathan Corbet <corbet@....net>,
	Shuah Khan <skhan@...uxfoundation.org>, "x86@...nel.org" <x86@...nel.org>
CC: James Morse <james.morse@....com>, Jamie Iles <quic_jiles@...cinc.com>,
	Babu Moger <babu.moger@....com>, Randy Dunlap <rdunlap@...radead.org>,
	"Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"patches@...ts.linux.dev" <patches@...ts.linux.dev>
Subject: Re: [PATCH v8 5/7] x86/resctrl: Add "mba_MBps_event" file to ctrl_mon
 directories

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