Re: [PATCH v1 29/31] fs/resctrl: Add boiler plate for external resctrl code

[Dave Martin]

On Thu, Apr 11, 2024 at 10:42:48AM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/11/2024 7:27 AM, Dave Martin wrote:
> > On Mon, Apr 08, 2024 at 08:41:04PM -0700, Reinette Chatre wrote:
> >> Hi James,
> >>
> >> On 3/21/2024 9:51 AM, James Morse wrote:
> >>> Add Makefile and Kconfig for fs/resctrl. Add ARCH_HAS_CPU_RESCTRL
> >>> for the common parts of the resctrl interface and make X86_CPU_RESCTRL
> >>> depend on this.
> >>>
> >>> Signed-off-by: James Morse <james.morse@xxxxxxx>
> >>> ---
> >>>  MAINTAINERS              |  1 +
> >>>  arch/Kconfig             |  8 ++++++++
> >>>  arch/x86/Kconfig         | 10 +++-------
> >>>  fs/Kconfig               |  1 +
> >>>  fs/Makefile              |  1 +
> >>>  fs/resctrl/Kconfig       | 23 +++++++++++++++++++++++
> >>>  fs/resctrl/Makefile      |  3 +++
> >>>  fs/resctrl/ctrlmondata.c |  0
> >>>  fs/resctrl/internal.h    |  0
> >>>  fs/resctrl/monitor.c     |  0
> >>>  fs/resctrl/psuedo_lock.c |  0
> >>>  fs/resctrl/rdtgroup.c    |  0
> >>>  include/linux/resctrl.h  |  4 ++++
> >>>  13 files changed, 44 insertions(+), 7 deletions(-)
> >>>  create mode 100644 fs/resctrl/Kconfig
> >>>  create mode 100644 fs/resctrl/Makefile
> >>>  create mode 100644 fs/resctrl/ctrlmondata.c
> >>>  create mode 100644 fs/resctrl/internal.h
> >>>  create mode 100644 fs/resctrl/monitor.c
> >>>  create mode 100644 fs/resctrl/psuedo_lock.c
> >>>  create mode 100644 fs/resctrl/rdtgroup.c
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index 5621dd823e79..c49090e9c777 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -18543,6 +18543,7 @@ S:	Supported
> >>>  F:	Documentation/arch/x86/resctrl*
> >>>  F:	arch/x86/include/asm/resctrl.h
> >>>  F:	arch/x86/kernel/cpu/resctrl/
> >>> +F:	fs/resctrl/
> >>>  F:	include/linux/resctrl*.h
> >>>  F:	tools/testing/selftests/resctrl/
> >>>  
> >>> diff --git a/arch/Kconfig b/arch/Kconfig
> >>> index fd18b7db2c77..131d874d6738 100644
> >>> --- a/arch/Kconfig
> >>> +++ b/arch/Kconfig
> >>> @@ -1406,6 +1406,14 @@ config STRICT_MODULE_RWX
> >>>  config ARCH_HAS_PHYS_TO_DMA
> >>>  	bool
> >>>  
> >>> +config ARCH_HAS_CPU_RESCTRL
> >>> +	bool
> >>> +	help
> >>> +	  The 'resctrl' filesystem allows CPU controls of shared resources
> >>> +	  such as caches and memory bandwidth to be configured. An architecture
> >>> +	  selects this if it provides the arch-specific hooks for the filesystem
> >>> +	  and needs the per-task CLOSID/RMID properties.
> >>
> >> Should it mention monitoring capabilities?
> > 
> > Probably, although I wonder whether it is better to describe this just
> > once, under RESCTRL_FS.  Does it makes sense to have something
> > like this here?
> > 
> > 	  An architecture selects this option to indicate that the necessary
> > 	  hooks are provided to support the common memory system usage
> > 	  monitoring and control interfaces provided by the 'resctrl'
> > 	  filesystem (see RESCTRL_FS).
> 
> This looks good to me.

Noted, thanks.

> 
> > 
> > If so, I can propose this.
> > 
> > (Details on what gets added to task_struct is maybe unnecessarily low-
> > level to bother with here...)
> > 
> >>> +
> >>>  config HAVE_ARCH_COMPILER_H
> >>>  	bool
> >>>  	help
> >>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >>> index e071e564452e..cb043543f088 100644
> >>> --- a/arch/x86/Kconfig
> >>> +++ b/arch/x86/Kconfig
> >>> @@ -479,8 +479,10 @@ config GOLDFISH
> >>>  config X86_CPU_RESCTRL
> >>>  	bool "x86 CPU resource control support"
> >>>  	depends on X86 && (CPU_SUP_INTEL || CPU_SUP_AMD)
> >>> +	depends on MISC_FILESYSTEMS
> >>>  	select KERNFS
> >>
> >> Do both X86_CPU_RESCTRL and RESCTRL_FS need to select KERNFS?
> > 
> > Hmmm, hopefully the arch backend doesn't need to re-depend on KERNFS.
> > I'll note that for review.
> > 
> > (If not, we can probably drop filesystem-related #includes from the
> > remaining x86 arch code too...)
> > 
> > [...]

(I still need to look at this.)

> > 
> >>> diff --git a/fs/Kconfig b/fs/Kconfig
> >>> index a46b0cbc4d8f..d8a36383b6dc 100644
> >>> --- a/fs/Kconfig
> >>> +++ b/fs/Kconfig
> >>> @@ -331,6 +331,7 @@ source "fs/omfs/Kconfig"
> >>>  source "fs/hpfs/Kconfig"
> >>>  source "fs/qnx4/Kconfig"
> >>>  source "fs/qnx6/Kconfig"
> >>> +source "fs/resctrl/Kconfig"
> >>>  source "fs/romfs/Kconfig"
> >>>  source "fs/pstore/Kconfig"
> >>>  source "fs/sysv/Kconfig"
> >>> diff --git a/fs/Makefile b/fs/Makefile
> >>> index 6ecc9b0a53f2..da6e2d028722 100644
> >>> --- a/fs/Makefile
> >>> +++ b/fs/Makefile
> >>> @@ -129,3 +129,4 @@ obj-$(CONFIG_EFIVAR_FS)		+= efivarfs/
> >>>  obj-$(CONFIG_EROFS_FS)		+= erofs/
> >>>  obj-$(CONFIG_VBOXSF_FS)		+= vboxsf/
> >>>  obj-$(CONFIG_ZONEFS_FS)		+= zonefs/
> >>> +obj-$(CONFIG_RESCTRL_FS)	+= resctrl/
> >>> diff --git a/fs/resctrl/Kconfig b/fs/resctrl/Kconfig
> >>> new file mode 100644
> >>> index 000000000000..36a1ddbe6c21
> >>> --- /dev/null
> >>> +++ b/fs/resctrl/Kconfig
> >>
> >> Could you please review the contents of this file for
> >> appropriate line length and consistent tab usage?
> > 
> > Noted.
> > 
> >>> @@ -0,0 +1,23 @@
> >>> +config RESCTRL_FS
> >>> +	bool "CPU Resource Control Filesystem (resctrl)"
> >>> +	depends on ARCH_HAS_CPU_RESCTRL
> >>> +	select KERNFS
> >>> +	select PROC_CPU_RESCTRL               if PROC_FS
> >>> +	help
> >>> +	  Resctrl is a filesystem interface
> >>> +	  to control allocation and
> >>> +	  monitoring of system resources
> >>> +	  used by the CPUs.
> > 
> > (Not quite a haiku, but I don't know how many syllables "resctrl"
> > counts as...)
> > 
> > Since this is the Kconfig user's primary knob for enabling resctrl,
> > maybe flesh this out and make it a bit more generic and newbie-friendly?
> > Something like:
> > 
> > 	  Some architectures provide hardware facilities to group tasks and
> > 	  monitor and control their usage of memory system resources such as
> > 	  caches and memory bandwidth.  Examples of such facilities include
> > 	  Intel's Resource Director Technology (Intel(R) RDT) and AMD's
> > 	  Platform Quality of Service (AMD QoS).
> 
> Nit: We should double check with AMD how they want to refer to their
> feature. Their contribution to the resctrl docs used the term you provide
> but their spec uses PQOS.
> 
> Do you expect this snippet to be updated when MPAM full support lands or
> does resctrl have enough support at this point to include a mention of MPAM?

Probably yes, even if only to make the point that this is generic and
relevant for more than one architecture.

For this series, MPAM is not known to the kernel, so I didn't want to
give the impression that there was any support for it (yet).

> > 
> > 	  If your system has the necessary support and you want to be able to
> > 	  assign tasks to groups and manipulate the associated resource
> > 	  monitors and controls from userspace, say Y here to get a mountable
> > 	  'resctrl' filesystem that lets you do just that.
> > 
> > 	  If nothing mounts or prods the 'resctrl' filesystem, resource
> > 	  controls and monitors are left in a quiescent, permissive state.
> 
> Well written, thank you.

Thanks for the once-over; I'll propose this to James for inclusion.

> 
> > 
> > 	  If unsure, it is safe to say Y.
> > 
> > 	  See <file:Documentation/arch/x86/resctrl.rst> for more information.
> > 
> > I'm assuming that just enabling this option doesn't introduce
> > significant overheads.  For MPAM I'm pretty sure it doesn't,
> > but if this is a concern that we could go for "If unsure, say N."
> 
> I would vote for N for when folks are not sure.

OK, fair enough.  Noted.

Cheers
---Dave