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

[James Morse <james.morse@arm.com>]

Hi Fenghua,

On 07/06/2022 22:07, Fenghua Yu wrote:
> On Tue, Apr 12, 2022 at 12:44:14PM +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 makes
>> rmid_dirty() too big.

> rmid_dirty() is not called by resctrl_arch_rmid_read(). Why does parring r and d
> make rmid_dirty() too big?

But rmid_dirty() does call resctrl_arch_rmid_read(). To add the resource and the domain,
which the arch-specific side of this needs to get its work done, changes the prototype from:
| static bool rmid_dirty(struct rmid_entry *entry)
to:
| static bool rmid_dirty(struct rdt_resource *r, struct rdt_domain *d,
| 			 enum resctrl_event_id eventid, struct rmid_entry *entry)

Not to mention the closid, which MPAM will need in here too.

rmid_dirty() only has one caller, and other than this function call - its only doing a
single comparison.

Instead of bloating it - I merged it with its sole caller.


>> Instead merge it with its only caller, and the
>> name is kept as a local variable.

>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 20c54cbadc0c..81cc7587b598 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)
> 
> Can this be a concise interface by just adding one more parameter "rr"?
> 
> int resctrl_arch_rmid_read(struct rmid_read *rr, u32 rmid, u64 *val);
>>  {
>>  	u64 msr_val;
> 
> Then inside the API, add:
> 	struct rdt_resource *r = rr->r;
> 	struct rdt_domain *d = rr->d;
> 	struct rdt_resource r = rr->r;
> 	enum resctrl_event_id evtid = rr->evtid;

Isn't this less concise for all the callers? A subsequent patch has to add the CLOSID too
as MPAM needs CLOSID/RMID as a pair.

I really don't like APIs that do this, it allows an uninitialised value to be passed by
accident. Putting this in the prototype lets the compiler check all the arguments are
supplied.

(It does make sense if there are unions, or not all the arguments are required. That
doesn't apply here)

I don't think 5 or 6 arguments to a function are a problem.


Thanks,

James