Re: [PATCH v3 16/21] x86/resctrl: Pass the required parameters into resctrl_arch_rmid_read()

[Rob Herring <robh@kernel.org>]

On Thu, Feb 17, 2022 at 06:21:05PM +0000, 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

s/make/makes/

> rmid_dirty() too big, instead merge it with its only caller, the name is
> kept as a local variable.

... big. Instead, merge it with its only caller, and the name...

> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> Changes since v2:
>  * Typos.
>  * Kerneldoc fixes.
> 
> 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               | 16 +++++++++++++-
>  2 files changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index b6ad290fda8d..277c22f8c976 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,
> +			   u32 rmid, enum resctrl_event_id eventid, u64 *val)
>  {
>  	u64 msr_val;
>  
> +	if (!cpumask_test_cpu(smp_processor_id(), &d->cpu_mask))

We already tested this and disabled preemption. (At least from some 
caller AFAICT from this patch.) I'd assume we'd want the fs code to 
handle preemption disable and checking cpumask. In any case, it should 
be clear what guarantees resctrl_arch_rmid_read() has.

> @@ -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)