Re: [PATCH v5 3/8] x86/resctrl: Split the rdt_domain structure

From: Reinette Chatre
Date: Mon Sep 25 2023 - 19:25:32 EST

Hi Tony,

Subject:

x86/resctrl: Split the rdt_domain and rdt_hw_domain structures


On 8/29/2023 4:44 PM, Tony Luck wrote:
> The same rdt_domain structure is used for both control an monitor

"control an monitor" -> "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.
>
> Create a new rdt_mondomain structure tailored explicitly for use in
> monitor parts of the core. Slim down the rdt_domain structure by
> removing the unused monitor fields.
>

Similar to the previous patch I think it will make the code
easier to understand if the naming is clear for both
monitoring and control structured. Why not rdt_mondomain
and rdt_ctrldomain instead?


> Similar breakout of struct rdt_hw_mondomain from struct rdt_hw_domain.

rdt_hw_mondomain and rdt_hw_ctrldomain?

>
> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
> include/linux/resctrl.h | 46 +++++++++++++++--------
> arch/x86/kernel/cpu/resctrl/internal.h | 38 +++++++++++++------
> arch/x86/kernel/cpu/resctrl/core.c | 18 ++++-----
> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 4 +-
> arch/x86/kernel/cpu/resctrl/monitor.c | 40 ++++++++++----------
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 24 ++++++------
> 6 files changed, 101 insertions(+), 69 deletions(-)
>
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 33856943a787..08382548571e 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -53,7 +53,29 @@ struct resctrl_staged_config {
> };
>
> /**
> - * struct rdt_domain - group of CPUs sharing a resctrl resource
> + * struct rdt_domain - group of CPUs sharing a resctrl control resource
> + * @list: all instances of this resource
> + * @id: unique id for this instance
> + * @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_domain {
> + // First three fields must match struct rdt_mondomain below.

Please avoid comments within declarations. Even so, could you please
elaborate what the above means? Why do the first three fields have to
match? I understand there is common code, for example, __rdt_find_domain()
that operated on the same members of the two structs but does that
require the members be in the same position in the struct?
I understand that a comment may be required if position in the struct
is important but I cannot see that it is.

> + struct list_head list;
> + int id;
> + struct cpumask cpu_mask;
> +
> + struct pseudo_lock_region *plr;
> + struct resctrl_staged_config staged_config[CDP_NUM_TYPES];
> + u32 *mbps_val;
> +};
> +
> +/**
> + * struct rdt_mondomain - group of CPUs sharing a resctrl monitor resource
> * @list: all instances of this resource
> * @id: unique id for this instance
> * @cpu_mask: which CPUs share this resource
> @@ -64,16 +86,13 @@ struct resctrl_staged_config {
> * @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_mondomain {
> + // First three fields must match struct rdt_domain above.

Same comment.

> struct list_head list;
> int id;
> struct cpumask cpu_mask;
> +
> unsigned long *rmid_busy_llc;
> struct mbm_state *mbm_total;
> struct mbm_state *mbm_local;
> @@ -81,9 +100,6 @@ struct rdt_domain {
> 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;
> };
>
> /**

...

> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 468c1815edfd..5167ac9cbe98 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -521,7 +521,7 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
> }
>
> void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> - struct rdt_domain *d, struct rdtgroup *rdtgrp,
> + struct rdt_mondomain *d, struct rdtgroup *rdtgrp,
> int evtid, int first)
> {
> /*
> @@ -544,7 +544,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
> struct rdtgroup *rdtgrp;
> struct rdt_resource *r;
> union mon_data_bits md;
> - struct rdt_domain *d;
> + struct rdt_mondomain *d;

Reverse fir order.

> struct rmid_read rr;
> int ret = 0;
>
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 66beca785535..42262d59ef9b 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -170,7 +170,7 @@ static int __rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *val)
> return 0;
> }
>
> -static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_domain *hw_dom,
> +static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_mondomain *hw_dom,
> u32 rmid,
> enum resctrl_event_id eventid)
> {
> @@ -189,10 +189,10 @@ static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_domain *hw_dom,
> return NULL;
> }
>
> -void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_domain *d,
> +void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mondomain *d,
> u32 rmid, enum resctrl_event_id eventid)
> {
> - struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
> + struct rdt_hw_mondomain *hw_dom = resctrl_to_arch_mondom(d);
> struct arch_mbm_state *am;
>
> am = get_arch_mbm_state(hw_dom, rmid, eventid);
> @@ -208,9 +208,9 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_domain *d,
> * Assumes that hardware counters are also reset and thus that there is
> * no need to record initial non-zero counts.
> */
> -void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_domain *d)
> +void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mondomain *d)
> {
> - struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
> + struct rdt_hw_mondomain *hw_dom = resctrl_to_arch_mondom(d);
>
> if (is_mbm_total_enabled())
> memset(hw_dom->arch_mbm_total, 0,
> @@ -229,11 +229,11 @@ static u64 mbm_overflow_count(u64 prev_msr, u64 cur_msr, unsigned int width)
> return chunks >> shift;
> }
>
> -int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
> +int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mondomain *d,
> u32 rmid, enum resctrl_event_id eventid, u64 *val)
> {
> struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> - struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
> + struct rdt_hw_mondomain *hw_mondom = resctrl_to_arch_mondom(d);

Reverse fir.

> struct arch_mbm_state *am;
> u64 msr_val, chunks;
> int ret;
> @@ -245,7 +245,7 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
> if (ret)
> return ret;
>
> - am = get_arch_mbm_state(hw_dom, rmid, eventid);
> + am = get_arch_mbm_state(hw_mondom, rmid, eventid);
> if (am) {
> am->chunks += mbm_overflow_count(am->prev_msr, msr_val,
> hw_res->mbm_width);
> @@ -266,7 +266,7 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
> * decrement the count. If the busy count gets to zero on an RMID, we
> * free the RMID
> */
> -void __check_limbo(struct rdt_domain *d, bool force_free)
> +void __check_limbo(struct rdt_mondomain *d, bool force_free)
> {
> struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> struct rmid_entry *entry;
> @@ -305,7 +305,7 @@ void __check_limbo(struct rdt_domain *d, bool force_free)
> }
> }
>
> -bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d)
> +bool has_busy_rmid(struct rdt_resource *r, struct rdt_mondomain *d)
> {
> return find_first_bit(d->rmid_busy_llc, r->num_rmid) != r->num_rmid;
> }
> @@ -334,7 +334,7 @@ int alloc_rmid(void)
> static void add_rmid_to_limbo(struct rmid_entry *entry)
> {
> struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> - struct rdt_domain *d;
> + struct rdt_mondomain *d;
> int cpu, err;
> u64 val = 0;
>
> @@ -383,7 +383,7 @@ void free_rmid(u32 rmid)
> list_add_tail(&entry->list, &rmid_free_lru);
> }
>
> -static struct mbm_state *get_mbm_state(struct rdt_domain *d, u32 rmid,
> +static struct mbm_state *get_mbm_state(struct rdt_mondomain *d, u32 rmid,
> enum resctrl_event_id evtid)
> {
> switch (evtid) {
> @@ -516,7 +516,7 @@ void mon_event_count(void *info)
> * throttle MSRs already have low percentage values. To avoid
> * unnecessarily restricting such rdtgroups, we also increase the bandwidth.
> */
> -static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm)
> +static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mondomain *dom_mbm)
> {
> u32 closid, rmid, cur_msr_val, new_msr_val;
> struct mbm_state *pmbm_data, *cmbm_data;
> @@ -600,7 +600,7 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm)
> }
> }
>
> -static void mbm_update(struct rdt_resource *r, struct rdt_domain *d, int rmid)
> +static void mbm_update(struct rdt_resource *r, struct rdt_mondomain *d, int rmid)
> {
> struct rmid_read rr;
>
> @@ -641,12 +641,12 @@ void cqm_handle_limbo(struct work_struct *work)
> unsigned long delay = msecs_to_jiffies(CQM_LIMBOCHECK_INTERVAL);
> int cpu = smp_processor_id();
> struct rdt_resource *r;
> - struct rdt_domain *d;
> + struct rdt_mondomain *d;

Reverse fir (Please check all code).

Reinette