linux-kernel - Re: [PATCH v5 17/40] x86/resctrl: Rewrite and move the for_each_*_rdt

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: <ZwR2D4ISzIrZRTHi@agluck-desk3.sc.intel.com>
Date: Mon, 7 Oct 2024 17:00:15 -0700
From: Tony Luck <tony.luck@...el.com>
To: James Morse <james.morse@....com>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
	Fenghua Yu <fenghua.yu@...el.com>,
	Reinette Chatre <reinette.chatre@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	H Peter Anvin <hpa@...or.com>, Babu Moger <Babu.Moger@....com>,
	shameerali.kolothum.thodi@...wei.com,
	D Scott Phillips OS <scott@...amperecomputing.com>,
	carl@...amperecomputing.com, lcherian@...vell.com,
	bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
	baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
	Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
	dfustini@...libre.com, amitsinght@...vell.com,
	David Hildenbrand <david@...hat.com>,
	Rex Nie <rex.nie@...uarmicro.com>,
	Dave Martin <dave.martin@....com>,
	Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH v5 17/40] x86/resctrl: Rewrite and move the
 for_each_*_rdt_resource() walkers

On Fri, Oct 04, 2024 at 06:03:24PM +0000, James Morse wrote:
> The for_each_*_rdt_resource() helpers walk the architecture's array
> of structures, using the resctrl visible part as an iterator. These
> became over-complex when the structures were split into a
> filesystem and architecture-specific struct. This approach avoided
> the need to touch every call site, and was done before there was a
> helper to retrieve a resource by rid.
> 
> Once the filesystem parts of resctrl are moved to /fs/, both the
> architecture's resource array, and the definition of those structures
> is no longer accessible. To support resctrl, each architecture would
> have to provide equally complex macros.
> 
> Rewrite the macro to make use of resctrl_arch_get_resource(), and
> move these to the core header so existing x86 arch code continues
> to use them.

Apologies if this comment was suggested against earlier versions
of this series.

Did you consider replacing rdt_resources_all[] a list (in the filesystem
code) instead of an array (in the architecture code)?

List would start empty. Architecture init code would enumerate features
and add entries to the list for those that exist and are to be enabled.

The "for_each" macros then walk the list (variants for all entries,
for "alloc_capable" and for "mon_capable"). Note that only enabled
entries appear on the lists.

There are currently a bunch of places in filesystem code that
do:
	r = resctrl_arch_get_resource(RDT_RESOURCE_MBA);
or
	r = resctrl_arch_get_resource(RDT_RESOURCE_L3);

those could become:

	r = resctrl_arch_get_mba_resource();

	r = resctrl_arch_get_l3_resource();

Then the whole "enum resctrl_res_level" and ->rid field in
struct rdt_resource could go away? Remaining uses look like
distinguishing MBA from SMBA. Perhaps better done with a
flags word?

Advantage of doing this would be to avoid the generic
enum resctrl_res_level having to be a superset of all
features across all architectures. E.g. ARM might want
to add L4/L5 resources, X86 may have some that ARM will
never need. RiscV may also follow some divergent path.

If this v5 series is close to being applied then I don't
want to derail with a re-write at this late stage.
All of this could be done as a cleanup after this series
has been applied.

-Tony