Re: [PATCH v10 4/8] x86/resctrl: Split the rdt_domain and rdt_hw_domain structures

[Tony Luck]

On Mon, Nov 06, 2023 at 04:32:56PM -0800, Reinette Chatre wrote:
> Hi Tony,
> 
> On 10/31/2023 2:17 PM, Tony Luck wrote:
> > The same rdt_domain structure is used for both control and monitor
> > functions. But this results in wasted memory as some of the fields are
> > only used by control functions, while most are only used for monitor
> > functions.
> > 
> > Split into separate rdt_ctrl_domain and rdt_mon_domain structures with
> > just the fields required for control and monitoring respectively.
> > 
> > Similar split of the rdt_hw_domain structure into rdt_hw_ctrl_domain
> > and rdt_hw_mon_domain.
> > 
> > Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> > ---
> > Changes since v9
> > Comment against patch 4, but now fixed in patch #2. cpu_mask
> > is included in common header.
> > 
> >  include/linux/resctrl.h                   | 50 +++++++------
> >  arch/x86/kernel/cpu/resctrl/internal.h    | 60 ++++++++++------
> >  arch/x86/kernel/cpu/resctrl/core.c        | 87 ++++++++++++-----------
> >  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 32 ++++-----
> >  arch/x86/kernel/cpu/resctrl/monitor.c     | 40 +++++------
> >  arch/x86/kernel/cpu/resctrl/pseudo_lock.c |  6 +-
> >  arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 62 ++++++++--------
> >  7 files changed, 184 insertions(+), 153 deletions(-)
> > 
> > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> > index 35e700edc6e6..36503e8870cd 100644
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -72,7 +72,25 @@ struct rdt_domain_hdr {
> >  };
> >  
> >  /**
> > - * struct rdt_domain - group of CPUs sharing a resctrl resource
> > + * struct rdt_ctrl_domain - group of CPUs sharing a resctrl control resource
> > + * @hdr:		common header for different domain types
> > + * @cpu_mask:		which CPUs share this resource
> > + * @plr:		pseudo-locked region (if any) associated with domain
> > + * @staged_config:	parsed configuration to be applied
> > + * @mbps_val:		When mba_sc is enabled, this holds the array of user
> > + *			specified control values for mba_sc in MBps, indexed
> > + *			by closid
> > + */
> > +struct rdt_ctrl_domain {
> > +	struct rdt_domain_hdr		hdr;
> > +	struct cpumask			cpu_mask;
> 
> This patch did not change what it said it changed.

Reinette,

I'm not sure of the problem. The commit said the patch is splitting the
rdt_domain structure into rdt_ctrl_domain and rdt_mon_domain.

The piece of the patch where you gave this comment changed like this:

------- Before -------
/**
 * struct rdt_domain - group of CPUs sharing a resctrl resource
 * @hdr:		common header for different domain types
 * @rmid_busy_llc:	bitmap of which limbo RMIDs are above threshold
 * @mbm_total:		saved state for MBM total bandwidth
 * @mbm_local:		saved state for MBM local bandwidth
 * @mbm_over:		worker to periodically read MBM h/w counters
 * @cqm_limbo:		worker to periodically read CQM h/w counters
 * @mbm_work_cpu:	worker CPU for MBM h/w counters
 * @cqm_work_cpu:	worker CPU for CQM h/w counters
 * @plr:		pseudo-locked region (if any) associated with domain
 * @staged_config:	parsed configuration to be applied
 * @mbps_val:		When mba_sc is enabled, this holds the array of user
 *			specified control values for mba_sc in MBps, indexed
 *			by closid
 */
struct rdt_domain {
	struct rdt_domain_hdr		hdr;
	unsigned long			*rmid_busy_llc;
	struct mbm_state		*mbm_total;
	struct mbm_state		*mbm_local;
	struct delayed_work		mbm_over;
	struct delayed_work		cqm_limbo;
	int				mbm_work_cpu;
	int				cqm_work_cpu;
	struct pseudo_lock_region	*plr;
	struct resctrl_staged_config	staged_config[CDP_NUM_TYPES];
	u32				*mbps_val;
};
------- After -------
/**
 * struct rdt_ctrl_domain - group of CPUs sharing a resctrl control resource
 * @hdr:		common header for different domain types
 * @cpu_mask:		which CPUs share this resource
 * @plr:		pseudo-locked region (if any) associated with domain
 * @staged_config:	parsed configuration to be applied
 * @mbps_val:		When mba_sc is enabled, this holds the array of user
 *			specified control values for mba_sc in MBps, indexed
 *			by closid
 */
struct rdt_ctrl_domain {
	struct rdt_domain_hdr		hdr;
	struct cpumask			cpu_mask;
	struct pseudo_lock_region	*plr;
	struct resctrl_staged_config	staged_config[CDP_NUM_TYPES];
	u32				*mbps_val;
};

/**
 * struct rdt_mon_domain - group of CPUs sharing a resctrl control resource
 * @hdr:		common header for different domain types
 * @rmid_busy_llc:	bitmap of which limbo RMIDs are above threshold
 * @mbm_total:		saved state for MBM total bandwidth
 * @mbm_local:		saved state for MBM local bandwidth
 * @mbm_over:		worker to periodically read MBM h/w counters
 * @cqm_limbo:		worker to periodically read CQM h/w counters
 * @mbm_work_cpu:	worker CPU for MBM h/w counters
 * @cqm_work_cpu:	worker CPU for CQM h/w counters
 */
struct rdt_mon_domain {
	struct rdt_domain_hdr		hdr;
	unsigned long			*rmid_busy_llc;
	struct mbm_state		*mbm_total;
	struct mbm_state		*mbm_local;
	struct delayed_work		mbm_over;
	struct delayed_work		cqm_limbo;
	int				mbm_work_cpu;
	int				cqm_work_cpu;
};
-----

Which to my eyes looks like exactly what the commit comment says I
was going to do the in this patch. The old rdt_domain structure has
been replaced with two structures, with names as described in the commit
comment. The fields of the orginal structure have been distributed
betweeen the two new structures based on whether they are used for
control or monitor functions.


What am I missing? (Apart from a silly cut & paste error in the comments
that I just noticed and will fix now).

-Tony