On Mon, Sep 25, 2023 at 04:25:15PM -0700, Reinette Chatre wrote:
> Hi Tony,
>
> Subject:
>
> x86/resctrl: Split the rdt_domain and rdt_hw_domain structures
Fixed subject as suggested.
>
>
> 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"
Fixed.
>
> > 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?
Done.
>
>
> > Similar breakout of struct rdt_hw_mondomain from struct rdt_hw_domain.
>
> rdt_hw_mondomain and rdt_hw_ctrldomain?
Yes. rdt_hw_domain is split two ways and each gets an appropriate name.
I added an extra "_" to match shape of other names. So these are now
"rdt_hw_ctrl_domain" and "rdt_hw_mon_domain"
> >
> > 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.
Discussed in other e-mail thread. Comments go away. The TWO (not three)
fields that must be common are now in an embedded "rdt_domain_hdr"
structure.
>
> > + 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.
Same solution.
>
> > 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.
Fixed.
>
> > 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.
Fixed.
>
> > 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).
Fixed. (I wrote a simple awk script to find these (as I'm
obviously bad at noticing them). Included at end of this
message).
>
> Reinette
#!/usr/bin/awk -f
BEGIN {
keyw["bool"] = 1
keyw["char"] = 1
keyw["enum"] = 1
keyw["int"] = 1
keyw["long"] = 1
keyw["short"] = 1
keyw["static"] = 1
keyw["struct"] = 1
keyw["u16"] = 1
keyw["u32"] = 1
keyw["u64"] = 1
keyw["u8"] = 1
keyw["unsigned"] = 1
keyw["void"] = 1
}
{
source[NR] = $0
if ($0 == "{") {
infunction = NR
skip
}
if ($0 == "}") {
infunction = 0
skip
}
if (infunction && ($1 in keyw)) {
inlocals++
if (inlocals > 1 && length(source[NR - 1]) < length)
badfir = 1
skip
}
if (inlocals && NF == 0) {
if (badfir) {
print "==== BAD FIR ===="
for (i = infunction - 2; i < NR; i++)
printf("%4d: %s\n", i, source[i])
}
inlocals = 0
badfir = 0
}
}
