linux-kernel - RE: [PATCH v7 1/4] x86/resctrl: Make input event for MBA Software Controller configurable

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: <SJ1PR11MB608328D2D772314AF62A2A7AFC4F2@SJ1PR11MB6083.namprd11.prod.outlook.com>
Date: Fri, 25 Oct 2024 20:42:06 +0000
From: "Luck, Tony" <tony.luck@...el.com>
To: "Chatre, Reinette" <reinette.chatre@...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: Shaopeng Tan <tan.shaopeng@...itsu.com>, James Morse
	<james.morse@....com>, Jamie Iles <quic_jiles@...cinc.com>, Babu Moger
	<babu.moger@....com>, Randy Dunlap <rdunlap@...radead.org>,
	"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 v7 1/4] x86/resctrl: Make input event for MBA Software
 Controller configurable

Plucking out just the big, direction change, comment for discussion (which may make
several of the code comments moot).

> I needed to refresh my understanding of this work by re-reading the previous discussions.
> You mentioned in [2]:
>       I tried out some code to make the event runtime selectable via a r/w file in the
>       resctrl/info directories. But that got complicated because of the amount of state
>       that needs to be updated when switching events.
>
> Could you please clarify which state you referred to? I wonder if it may be the
> struct mbm_state state maintained by mbm_bw_count()? mbm_bw_count() is lightweight
> and I see no problem with it being called for all supported MBM events when
> the software controller is enabled. With state for all supported events always available
> it seems simpler to runtime switch between which events guide the software controller?
>
> Thinking about it more, it seems possible for the user to use different
> MBM events to guide the software controller for different resource groups.
>
> If it is possible to do runtime switching in this way I do think it will simplify this
> implementation while not requiring the user to remount resctrl to make changes. You
> mentioned [3] that "a separate patch series" may be coming to address this but doing this
> now seems simpler while avoiding any future work as well as confusing duplicate ABI
> ... unless you were referring to other issues that needs to be addressed separately?

Yes, the state maintained by mbm_bw_count() was the piece that worried me. After
a user switch to a different event there would be no bandwidth data until two updates
passed by (one to get a baseline, second to compute bandwidth). So update_mba_bw()
would need to be aware of this liminal period to avoid making updates with no data to
back them up.

Your solution is elegant. The cost to maintain bandwidth data for each event is indeed
very low. So there are no weird transition cases. update_mba_bw() can immediately 
compare bandwidth for the new event against the target bandwidth and make appropriate
adjustments.

This requires a new file in each CTRL_MON directory when mba_sc is enabled so
the user can make their selection.

Note that technically it would be possible to make a different selection for each domain.
But that seems like an option without an obvious use case and would just complicate
the syntax of the new file.

Maybe name this new file "mba_sc_event"[1] with contents that match the names of
the mbm_monitor events as listed in /sys/fs/resctrl/info/L3_MON/mon_features?

So default state when resctrl is mounted with the software controller enabled would
have:

$ cat /sys/fs/resctrl/mba_sc_event
mbm_local_bytes

User could switch to total with

# echo mbm_total_bytes > /sys/fs/resctrl/mba_sc_event

On systems where mbm_local_bytes is not supported default would be mbm_total_bytes.

New CTRL_MON directories would also default to mbm_local_bytes if it is supported.

-Tony

[1] Other suggestions welcome.