Messages in this thread | | | Subject | Re: [PATCH v2 18/23] x86/resctrl: Pass the required parameters into resctrl_arch_rmid_read() | From | Reinette Chatre <> | Date | Fri, 15 Oct 2021 15:30:09 -0700 |
| |
Hi James,
On 10/1/2021 9:02 AM, James Morse wrote: > resctrl_arch_rmid_read() is intended as the function that an > architecture agnostic resctrl filesystem driver can use to > read a value in bytes from a hardware register. Currently the function > returns the mbm values in chunks directly from hardware. > > To convert this to bytes, some correction and overflow calculations > are needed. These depend on the resource and domain structures. > Overflow detection requires the old chunks value. None of this > is available to resctrl_arch_rmid_read(). MPAM requires the > resource and domain structures to find the MMIO device that holds > the registers. > > Pass the resource and domain to resctrl_arch_rmid_read(). This make > rmid_dirty() to big, instead merge it with its only caller, the name is
to big -> too big
> kept as a local variable. > > Signed-off-by: James Morse <james.morse@arm.com> > --- > This is all a little noisy for __mon_event_count(), as the switch > statement work is now before the resctrl_arch_rmid_read() call. > --- > arch/x86/kernel/cpu/resctrl/monitor.c | 31 +++++++++++++++------------ > include/linux/resctrl.h | 15 ++++++++++++- > 2 files changed, 31 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index cf35eaf01042..f833bc01aeac 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -167,10 +167,14 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_domain *d, > memset(am, 0, sizeof(*am)); > } > > -int resctrl_arch_rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *val) > +int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
Please do not use tabs in the function parameters.
> + u32 rmid, enum resctrl_event_id eventid, u64 *val) > { > u64 msr_val; > > + if (!cpumask_test_cpu(smp_processor_id(), &d->cpu_mask)) > + return -EINVAL; > + > /* > * As per the SDM, when IA32_QM_EVTSEL.EvtID (bits 7:0) is configured > * with a valid event code for supported resource type and the bits > @@ -192,16 +196,6 @@ int resctrl_arch_rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *val) > return 0; > } > > -static bool rmid_dirty(struct rmid_entry *entry) > -{ > - u64 val = 0; > - > - if (resctrl_arch_rmid_read(entry->rmid, QOS_L3_OCCUP_EVENT_ID, &val)) > - return true; > - > - return val >= resctrl_cqm_threshold; > -} > - > /* > * Check the RMIDs that are marked as busy for this domain. If the > * reported LLC occupancy is below the threshold clear the busy bit and > @@ -213,6 +207,8 @@ void __check_limbo(struct rdt_domain *d, bool force_free) > struct rmid_entry *entry; > struct rdt_resource *r; > u32 crmid = 1, nrmid; > + bool rmid_dirty; > + u64 val = 0; > > r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; > > @@ -228,7 +224,14 @@ void __check_limbo(struct rdt_domain *d, bool force_free) > break; > > entry = __rmid_entry(nrmid); > - if (force_free || !rmid_dirty(entry)) { > + > + if (resctrl_arch_rmid_read(r, d, entry->rmid, > + QOS_L3_OCCUP_EVENT_ID, &val)) > + rmid_dirty = true; > + else > + rmid_dirty = (val >= resctrl_cqm_threshold); > + > + if (force_free || !rmid_dirty) { > clear_bit(entry->rmid, d->rmid_busy_llc); > if (!--entry->busy) { > rmid_limbo_count--; > @@ -278,7 +281,7 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) > cpu = get_cpu(); > list_for_each_entry(d, &r->domains, list) { > if (cpumask_test_cpu(cpu, &d->cpu_mask)) { > - err = resctrl_arch_rmid_read(entry->rmid, > + err = resctrl_arch_rmid_read(r, d, entry->rmid, > QOS_L3_OCCUP_EVENT_ID, > &val); > if (err || val <= resctrl_cqm_threshold) > @@ -336,7 +339,7 @@ static u64 __mon_event_count(u32 rmid, struct rmid_read *rr) > if (rr->first) > resctrl_arch_reset_rmid(rr->r, rr->d, rmid, rr->evtid); > > - rr->err = resctrl_arch_rmid_read(rmid, rr->evtid, &tval); > + rr->err = resctrl_arch_rmid_read(rr->r, rr->d, rmid, rr->evtid, &tval); > if (rr->err) > return rr->err; > > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h > index 01bdd8be590b..4215a0564206 100644 > --- a/include/linux/resctrl.h > +++ b/include/linux/resctrl.h > @@ -218,7 +218,20 @@ u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_domain *d, > u32 closid, enum resctrl_conf_type type); > int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d); > void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d); > -int resctrl_arch_rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *res); > + > +/** > + * resctrl_arch_rmid_read() - Read the eventid counter correpsonding to rmid
correpsonding -> corresponding
> + * for this resource and domain. > + * @r: The resource that the counter should be read from. > + * @d: The domain that the counter should be read from. > + * @rmid: The rmid of the counter to read. > + * @eventid: The eventid to read, e.g. L3 occupancy. > + * @val: The result of the counter read in chunks. > + *
"The" prefix can be removed from all descriptions to match style of other descriptions, also in description of resctrl_arch_rmid_read() below.
> + * Returns 0 on success, or -EIO, -EINVAL etc on error.
I do not think this is valid kernel doc for the return section.
> + */ > +int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, > + u32 rmid, enum resctrl_event_id eventid, u64 *val); > > /** > * resctrl_arch_reset_rmid() - Reset any private state associated with rmid >
Reinette
| |