Re: [PATCH v5 06/16] fs/resctrl: Introduce kernel mode states for resctrl

[Babu Moger]

Hi Reinette,

On 9/16/26 00:28, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/26/26 12:32 PM, Babu Moger wrote:
>
> > ---
> >   fs/resctrl/internal.h | 51 +++++++++++++++++++++++++++++++++++++++++++
> >   fs/resctrl/rdtgroup.c | 33 ++++++++++++++++++++++++++++
> >   2 files changed, 84 insertions(+)
> >
> > diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> > index e62a277dee85..4087bf44a06d 100644
> > --- a/fs/resctrl/internal.h
> > +++ b/fs/resctrl/internal.h
> > @@ -314,6 +314,57 @@ struct mbm_state {
> >   	u32	prev_bw;
> >   };
> >   
> > +/**
> > + * enum kmode_state - Control or monitoring state for a kernel mode
> > + * @KMODE_INHERIT: Inherit from the user space task.
> > + * @KMODE_ASSIGN: Use a global assignment for kernel mode.
> > + */
> > +enum kmode_state {
> > +	KMODE_INHERIT,
> > +	KMODE_ASSIGN
> > +};
> > +
> > +/**
> > + * struct resctrl_kmode_caps - Static kernel mode capabilities
> > + * @kmode_sup:	Bitmap of supported kernel modes. Empty when neither
> > + *		@ctrl_en nor @mon_en is set and kernel mode policy is
> > + *		unavailable on this system.
> > + * @ctrl_en:	Whether kernel mode may use global assignment for control.
> > + * @mon_en:	Whether kernel mode may use global assignment for monitoring.
>
> Why is ctrl_en and mon_en needed? It seems to just store the output of
> whether system supports allocation and monitoring. I only see these used
> when user interacts reads or writes the kernel mode so not a "hot path" that
> needs to be optimized. Can these just be dropped and just use resctrl_arch_alloc_capable()
> and resctrl_arch_mon_capable() directly? Please note they are in process of
> being changed/renamed:

Sure. I'll remove it.

That also means we can consolidate everything into a single 
resctrl_kmode_cfg structure.


> https://lore.kernel.org/lkml/20260831174421.13921-7-tony.luck@xxxxxxxxx/
>
> > + */
> > +struct resctrl_kmode_caps {
> > +	DECLARE_BITMAP(kmode_sup, RESCTRL_NUM_KERNEL_MODES);
> > +	bool				ctrl_en;
> > +	bool				mon_en;
> > +};
> > +
> > +/**
> > + * struct resctrl_kmode_active - Runtime kernel mode state
> > + * @kmode_cur:	Currently selected kernel mode.
> > + * @ctrl_mode:	Control assignment state when kernel mode is active.
> > + * @mon_mode:	Monitoring assignment state when kernel mode is active.
> > + * @k_rdtgrp:	Resource group backing global assignment mode.
> > + *
> > + * When @kmode_cur is %RESCTRL_INHERIT_USER, assignment state is ignored and
> > + * @k_rdtgrp is %NULL.
>
> This implies that this is only the state for RESCTRL_ASSIGN_GLOBAL_ENABLE_PER_CPU.
> If it is made specifically so there is no need to pretend it is generic and
> add all these caveats.

ok.

> > + */
> > +struct resctrl_kmode_active {
> > +	enum resctrl_kernel_mode	kmode_cur;
> > +	enum kmode_state		ctrl_mode;
> > +	enum kmode_state		mon_mode;
>
> Is "mode" accurate? This is all about kernel "mode" and now control and
> monitoring have other modes?

It can be control and monitor.

> > +	struct rdtgroup			*k_rdtgrp;
>
> Could naming be consistent? Consider, for example, kmode_rdtgrp? Although
> if this struct can be specific to the global per-CPU kernel mode then it can
> just be "rdtgrp".

Sure.

> > +};
> > +
> > +/**
> > + * struct resctrl_kmode_cfg - Global kernel mode state
> > + * @caps:	Supported modes and assignment capabilities.
> > + * @active:	Active mode, assignment state, and assigned group.
>
> Please do not list the struct members as part of its description elsewhere since
> that will be difficult to keep accurate. Just describe what the struct represents.

Will remove it.
> > + */
> > +struct resctrl_kmode_cfg {
> > +	struct resctrl_kmode_caps	caps;
> > +	struct resctrl_kmode_active	active;
> > +};
> > +
> >   extern struct mutex rdtgroup_mutex;
> >   
> >   static inline const char *rdt_kn_name(const struct kernfs_node *kn)
> > diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> > index 5dcbb0a964e8..3c53f3f74e5a 100644
> > --- a/fs/resctrl/rdtgroup.c
> > +++ b/fs/resctrl/rdtgroup.c
> > @@ -76,6 +76,13 @@ static void rdtgroup_destroy_root(void);
> >   
> >   struct dentry *debugfs_resctrl;
> >   
> > +/*
> > + * Global kernel mode policy state: supported modes, active mode, assignment
> > + * capabilities, assignment state, and the resource group selected for a global
> > + * assignment.
>
> Same here - please do not just provide a list of the struct's members. A high level
> description instead.

ok.

> This code is really strange. This whole series is difficult to read. I have not seen
> these styles used before and surprised that it comes from you.

ack.

> > + */
> > +static struct resctrl_kmode_cfg resctrl_kcfg;
>
> I think the code will be easier to read if "resctrl_kcfg" -> "resctrl_kmode".

Sure.

> > +
> >   /*
> >    * Memory bandwidth monitoring event to use for the default CTRL_MON group
> >    * and each new CTRL_MON group created by the user.  Only relevant when
> > @@ -2297,6 +2304,30 @@ static void io_alloc_init(void)
> >   	}
> >   }
> >   
> > +/*
> > + * Initialize kernel mode policy defaults from architecture capabilities.
> > + *
> > + * When ctrl_en or mon_en is set, RESCTRL_INHERIT_USER is supported and
> > + * selected as the initial active mode. When neither is set, kmode_sup is
> > + * left empty, kernel mode policy is unavailable, and kmode_cur remains at
> > + * its zero-initialized default (RESCTRL_INHERIT_USER) but is unused.
>
> Above just verbatim describes the code. Please provide higher level why the
> code does what it does.

ok.

Thanks
Babu