Re: [PATCH v2 01/16] fs/resctrl: Add kernel mode (kmode) data structures and arch hook

[Babu Moger]

Hi Reinette,

On 3/24/26 17:51, Reinette Chatre wrote:
> Hi Babu,
>
> On 3/12/26 1:36 PM, Babu Moger wrote:
> > Add resctrl_kmode, resctrl_kmode_cfg, kernel mode bit defines, and
> > resctrl_arch_get_kmode_cfg() for resctrl kernel mode (e.g. PLZA) support.
> We should not have to start every series from scratch.
> Documentation/process/maintainer-tip.rst. Always.

Sure. Yea. I did not focus on that aspect of patch submission in this 
series. Will do next revision.


> > ---
> >   include/linux/resctrl.h       | 10 ++++++++++
> >   include/linux/resctrl_types.h | 30 ++++++++++++++++++++++++++++++
> >   2 files changed, 40 insertions(+)
> >
> > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> > index 006e57fd7ca5..2c36d1ac392f 100644
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -699,6 +699,16 @@ int resctrl_arch_io_alloc_enable(struct rdt_resource *r, bool enable);
> >    */
> >   bool resctrl_arch_get_io_alloc_enabled(struct rdt_resource *r);
> >   
> > +/**
> > + * resctrl_arch_get_kmode_cfg() - Get resctrl kernel mode configuration
> > + * @kcfg:	Filled with current kernel mode config (kmode, kmode_cur, k_rdtgrp).
> > + *
> > + * Used by the arch (e.g. x86) to report which kernel mode is active and,
> > + * when a global assign mode is in use, which rdtgroup is assigned to
> > + * kernel work.
> > + */
> > +void resctrl_arch_get_kmode_cfg(struct resctrl_kmode_cfg *kcfg);
> This interface does not look right. Would it not be resctrl fs that determines
> which resource group is assigned? This cannot be set by arch. Why does arch decide
> which mode is active? Is this not also resctrl fs? Should arch not just tell
> resctrl fs what it supports?
Yes. Sure. Let the arch tell what is supported.  Let fs decide what is 
default.
> > +
> >   extern unsigned int resctrl_rmid_realloc_threshold;
> >   extern unsigned int resctrl_rmid_realloc_limit;
> >   
> > diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> > index a5f56faa18d2..6b78b08eab29 100644
> > --- a/include/linux/resctrl_types.h
> > +++ b/include/linux/resctrl_types.h
> > @@ -65,7 +65,37 @@ enum resctrl_event_id {
> >   	QOS_NUM_EVENTS,
> >   };
> >   
> > +/**
> > + * struct resctrl_kmode - Resctrl kernel mode descriptor
> > + * @name:	Human-readable name of the kernel mode.
> > + * @val:	Bitmask value for the kernel mode (e.g. INHERIT_CTRL_AND_MON).
> > + */
> > +struct resctrl_kmode {
> > +	char    name[32];
> > +	u32     val;
> > +};
> There is no reason why this needs to be in a central header exposed to archs. Could
> this not be a static within the only function that uses it? Something like
> rdt_mode_str[]?
Yes. I think so.
> > +
> > +/**
> > + * struct resctrl_kmode_cfg - Resctrl kernel mode configuration
> > + * @kmode:	Requested kernel mode.
> > + * @kmode_cur:	Currently active kernel mode.
> > + * @k_rdtgrp:	Resource control structure in use, or NULL otherwise.
> > + */
> > +struct resctrl_kmode_cfg {
> > +	u32 kmode;
> > +	u32 kmode_cur;
> > +	struct rdtgroup *k_rdtgrp;
> > +};
> > +
> >   #define QOS_NUM_L3_MBM_EVENTS	(QOS_L3_MBM_LOCAL_EVENT_ID - QOS_L3_MBM_TOTAL_EVENT_ID + 1)
> >   #define MBM_STATE_IDX(evt)	((evt) - QOS_L3_MBM_TOTAL_EVENT_ID)
> >   
> > +/* Resctrl kernel mode bits (e.g. for PLZA). */
> > +#define INHERIT_CTRL_AND_MON		BIT(0)	/* Kernel uses same CLOSID/RMID as user. */
> > +/* One CLOSID for all kernel work; RMID inherited from user. */
> > +#define GLOBAL_ASSIGN_CTRL_INHERIT_MON	BIT(1)
> > +/* One resource group (CLOSID+RMID) for all kernel work. */
> > +#define GLOBAL_ASSIGN_CTRL_ASSIGN_MON	BIT(2)
> > +#define RESCTRL_KERNEL_MODES_NUM	3
> I think it will make the code much easier to understand if the different modes are described by an
> enum. For example,
Yes. Sure.
> 	enum resctrl_kernel_modes {
> 		INHERIT_CTRL_AND_MON,
> 		GLOBAL_ASSIGN_CTRL_INHERIT_MON,
> 		GLOBAL_ASSIGN_CTRL_ASSIGN_MON,
> 		RESCTRL_KMODE_LAST = GLOBAL_ASSIGN_CTRL_ASSIGN_MON
> 	};
> 	#define RESCTRL_NUM_KERNEL_MODES (RESCTRL_KMODE_LAST + 1)
>
> The supported kernel modes can still be managed as a bitmap with intuitive API using the
> enum that will make the code easier to read. For example, __set_bit(INHERIT_CTRL_AND_MON, ...)
> or BIT(INHERIT_CTRL_AND_MON). The naming is awkward at the moment though, we should improve here.
> 		

Sure.  Yes. We need to think about naming.. Let me think about it.

Thanks

Babu