Re: [PATCH v3 11/19] x86/resctrl: Allow arch to allocate memory needed in resctrl_arch_rmid_read()

Reinette Chatre
2023-04-27T23:40:12Z

Hi James,

On 4/27/2023 7:19 AM, James Morse wrote:
> On 01/04/2023 00:27, Reinette Chatre wrote:
>> On 3/20/2023 10:26 AM, James Morse wrote:

...

>>> #include <linux/module.h>
>>> #include <linux/sizes.h>
>>> #include <linux/slab.h>
>>> @@ -271,7 +272,7 @@ static void smp_call_rmid_read(void *_arg)
>>>
>>> int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
>>> u32 closid, u32 rmid, enum resctrl_event_id eventid,
>>> - u64 *val)
>>> + u64 *val, int ignored)
>>> {
>>> struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>> struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
>>> @@ -317,9 +318,14 @@ void __check_limbo(struct rdt_domain *d, bool force_free)
>>> u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>>> struct rmid_entry *entry;
>>> u32 idx, cur_idx = 1;
>>> + int arch_mon_ctx;
>>> bool rmid_dirty;
>>> u64 val = 0;
>>>
>>> + arch_mon_ctx = resctrl_arch_mon_ctx_alloc(r, QOS_L3_OCCUP_EVENT_ID);
>>> + if (arch_mon_ctx < 0)
>>> + return;
>
>> The vision for this is not clear to me. When I read that context needs to be allocated
>> I expect it to return a pointer to some new context, not an int. What would the
>> "context" consist of?
>
> It might just need a different name.
>
> For MPAM, this is allocating a monitor, which is the hardware that does the counting in
> the cache or the memory controller. The number of monitors is an implementation choice,
> and may not match the number of CLOSID/RMID that are in use. There aren't guaranteed to be
> enough to allocate one for every control or monitor group up front.
>
> The int being returned is the allocated monitor's index. It identifies which monitor needs
> programming to read the provided CLOSID/RMID, and the counter register to read with the value.

I see.

>
> I can allocate memory for an int if you think that is clearer.
> (I was hoping to leave that for whoever needs something bigger than a pointer)

I'd rather not complicate it in this way.

>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>> index ff7452f644e4..03e4f41cd336 100644
>>> --- a/include/linux/resctrl.h
>>> +++ b/include/linux/resctrl.h
>>> @@ -233,6 +233,7 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d);
>>> * @rmid: rmid of the counter to read.
>>> * @eventid: eventid to read, e.g. L3 occupancy.
>>> * @val: result of the counter read in bytes.
>>> + * @arch_mon_ctx: An allocated context from resctrl_arch_mon_ctx_alloc().
>>> *
>
>> Could this description be expanded to indicate what this context is used for?
>
> Sure,
> "An architecture specific value from resctrl_arch_mon_ctx_alloc(), for MPAM this
> identifies the hardware monitor allocated for this read request".

This helps. Thank you.

Reinette