Re: [PATCH v9 18/26] x86/resctrl: Add the interface to assign/update counter assignment

["Moger, Babu" <bmoger@amd.com>]

Hi Reinette,

On 11/22/2024 4:07 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 11/22/24 1:04 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 11/21/2024 2:50 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 11/20/24 10:05 AM, Moger, Babu wrote:
>>>> Hi Reinette,
>>>>
>>>> On 11/15/24 18:57, Reinette Chatre wrote:
>>>>> Hi Babu,
>>>>>
>>>>> On 10/29/24 4:21 PM, Babu Moger wrote:
>>>>>> The mbm_cntr_assign mode offers several hardware counters that can be
>>>>>> assigned to an RMID, event pair and monitor the bandwidth as long as it
>>>>>> is assigned.
>>>>>>
>>>>>> Counters are managed at two levels. The global assignment is tracked
>>>>>> using the mbm_cntr_free_map field in the struct resctrl_mon, while
>>>>>> domain-specific assignments are tracked using the mbm_cntr_map field
>>>>>> in the struct rdt_mon_domain. Allocation begins at the global level
>>>>>> and is then applied individually to each domain.
>>>>>>
>>>>>> Introduce an interface to allocate these counters and update the
>>>>>> corresponding domains accordingly.
>>>>>>
>>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>>>> ---
>>>>>
>>>>> ...
>>>>>
>>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> index 00f7bf60e16a..cb496bd97007 100644
>>>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> @@ -717,6 +717,8 @@ unsigned int mon_event_config_index_get(u32 evtid);
>>>>>>    int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>>>>                     enum resctrl_event_id evtid, u32 rmid, u32 closid,
>>>>>>                     u32 cntr_id, bool assign);
>>>>>> +int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>>>>>> +                   struct rdt_mon_domain *d, enum resctrl_event_id evtid);
>>>>>>    void rdt_staged_configs_clear(void);
>>>>>>    bool closid_allocated(unsigned int closid);
>>>>>>    int resctrl_find_cleanest_closid(void);
>>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>>> index 1b5529c212f5..bc3752967c44 100644
>>>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>>> @@ -1924,6 +1924,93 @@ int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>>>>        return 0;
>>>>>>    }
>>>>>>    +/*
>>>>>> + * Configure the counter for the event, RMID pair for the domain.
>>>>>> + * Update the bitmap and reset the architectural state.
>>>>>> + */
>>>>>> +static int resctrl_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>>>> +                   enum resctrl_event_id evtid, u32 rmid, u32 closid,
>>>>>> +                   u32 cntr_id, bool assign)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = resctrl_arch_config_cntr(r, d, evtid, rmid, closid, cntr_id, assign);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    if (assign)
>>>>>> +        __set_bit(cntr_id, d->mbm_cntr_map);
>>>>>> +    else
>>>>>> +        __clear_bit(cntr_id, d->mbm_cntr_map);
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Reset the architectural state so that reading of hardware
>>>>>> +     * counter is not considered as an overflow in next update.
>>>>>> +     */
>>>>>> +    resctrl_arch_reset_rmid(r, d, closid, rmid, evtid);
>>>>>
>>>>> resctrl_arch_reset_rmid() expects to be run on a CPU that is in the domain
>>>>> @d ... note that after the architectural state is reset it initializes the
>>>>> state by reading the event on the current CPU. By running it here it is
>>>>> run on a random CPU that may not be in the right domain.
>>>>
>>>> Yes. That is correct.  We can move this part to our earlier
>>>> implementation. We dont need to read the RMID.  We just have to reset the
>>>> counter.
>>>>
>>>> https://lore.kernel.org/lkml/16d88cc4091cef1999b7ec329364e12dd0dc748d.1728495588.git.babu.moger@amd.com/
>>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> index 9fe419d0c536..bc3654ec3a08 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> @@ -2371,6 +2371,13 @@ int resctrl_arch_config_cntr(struct rdt_resource
>>>> *r, struct rdt_mon_domain *d,
>>>>           smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd,
>>>>                                 &abmc_cfg, 1);
>>>>
>>>> +       /*
>>>> +        * Reset the architectural state so that reading of hardware
>>>> +        * counter is not considered as an overflow in next update.
>>>> +        */
>>>> +       if (arch_mbm)
>>>> +               memset(arch_mbm, 0, sizeof(struct arch_mbm_state));
>>>> +
>>>>           return 0;
>>>>    }
>>>>
>>>>
>>>
>>> I am not sure what you envision here. One motivation for the move out of
>>> resctrl_arch_config_cntr() was to avoid architectural state being reset twice. For reference,
>>> mbm_config_write_domain()->resctrl_arch_reset_rmid_all(). Will architectural state
>>> be reset twice again?
>>
>> That is good point. We don't have to do it twice.
>>
>> We can move the whole reset(arch_mbm) in  resctrl_arch_config_cntr().
> 
> This is not clear to me. The architectural state needs to be reset on MBM config write even
> when assignable mode is not supported and/or enabled. Moving it to resctrl_arch_config_cntr()
> will break this, no?

Yes. The architectural state needs to be reset if ABMC is enabled or not 
enabled on MBM config write.

> 
> I wonder if it may not simplify things to call resctrl_arch_reset_rmid() from
> resctrl_abmc_config_one_amd()?

Yes. That is is an option. I can try.

> 
>>> One thing that I did not notice before is that the non-architectural MBM state is not
>>> reset. Care should be taken to reset this also when considering that there is a plan
>>> to use that MBM state to build a generic rate event for all platforms:
>>> https://lore.kernel.org/all/CALPaoCgFRFgQqG00Uc0GhMHK47bsbtFw6Bxy5O9A_HeYmGa5sA@mail.gmail.com/
>>
>> Did you mean we should add the following code in resctrl_arch_config_cntr()?
>>
>> m = get_mbm_state(d, closid, rmid, evtid);
>> if (m)
>>       memset(m, 0, sizeof(struct mbm_state));
> 
> This is not arch code but instead resctrl fs, so resctrl_config_cntr() may be more appropriate?

Sure. We can do that.

> 
> Reinette
> 
> 

-- 
- Babu Moger