Re: [PATCH v10 15/24] x86/resctrl: Implement resctrl_arch_config_cntr() to assign a counter with ABMC

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

Hi Reinette,

On 12/20/2024 5:47 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 12/20/24 2:28 PM, Moger, Babu wrote:
>> On 12/20/2024 3:41 PM, Reinette Chatre wrote:
>>> On 12/20/24 11:22 AM, Moger, Babu wrote:
>>>> On 12/19/2024 5:04 PM, Reinette Chatre wrote:
> 
>>>>>> @@ -1686,6 +1686,34 @@ unsigned int mon_event_config_index_get(u32 evtid)
>>>>>>         }
>>>>>>     }
>>>>>>     +struct cntr_config {
>>>>>> +    struct rdt_resource *r;
>>>>>> +    struct rdt_mon_domain *d;
>>>>>> +    enum resctrl_event_id evtid;
>>>>>> +    u32 rmid;
>>>>>> +    u32 closid;
>>>>>> +    u32 cntr_id;
>>>>>> +    u32 val;
>>>>>> +    bool assign;
>>>>>> +};
>>>>>
>>>>> I think I am missing something because it is not clear to me why this
>>>>> new struct is needed. Why not just use union l3_qos_abmc_cfg?
>>>>
>>>> New struct is needed because we need to call resctrl_arch_reset_rmid() inside IPI. It requires all these parameters.
>>>
>>> Could you please answer my question?
>>
>> May be I did not understand your question here.
>>
>> We need to do couple of things here in the IPI.
>>
>> 1. Configure the counter. This requires the cntr_id, rmid, event config value and assign(or unassign). This is to populate  l3_qos_abmc_cfg and write the MSR.
>>
>> 2. Reset RMID. This requires rdt_resource, rdt_mon_domain, RMID, CLOSID and event.
>>
>> So, I packed all these in a new structure and sent to IPI handler so that both these actions can be done in IPI.
>>
>> Can this be simplified?
> 
> This is all architecture specific code so I think l3_qos_abmc_cfg can be
> initialized once and then passed around. Bouncing the individual members of
> l3_qos_abmc_cfg through struct cntr_config seems unnecessary to me. More specifically,
> would it not make things simpler to make l3_qos_abmc_cfg a member of cntr_config?

Yes. It can be done.

> 
>>>>>> @@ -1869,6 +1897,36 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
>>>>>>         return ret ?: nbytes;
>>>>>>     }
>>>>>>     +/*
>>>>>> + * Send an IPI to the domain to assign the counter to RMID, event pair.
>>>>>> + */
>>>>>> +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)
>>>>>> +{
>>>>>> +    struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>>>>>> +    struct cntr_config config = { 0 };
>>>>>
>>>>> Please see 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")
>>>>
>>>> This may not apply here.
>>>>
>>>> x86/resctrl: Slightly clean-up mbm_config_show()
>>>>
>>>> "mon_info' is already zeroed in the list_for_each_entry() loop below. There  is no need to explicitly initialize it here. It just wastes some space and cycles.
>>>>
>>>> In our case we are not doing memset again.
>>>
>>> No, but every member is explicitly initialized instead. It may be needed if
>>> union l3_qos_abmc_cfg is used as I asked about earlier where it will be important
>>> to ensure reserve bits are initialized.
>>
>> I missed your comment on reserve bits(Searched in this series). General rule is reserve bits should be written as zeros.
> 
> 
> I do not think I am being clear.
> 
> Back to original comment: resctrl_arch_config_cntr() zeroes the entire struct and then
> initializes every member. I do not think it is necessary to zero the struct if
> every member is initialized. If you want to be explicit about the zero initialization
> you can do so while initializing the struct only once where it is defined.
> See for example, rdtgroup_kn_set_ugid()

Yes. I got it. It was not required as we are initializing all the 
members of config here.

With adding l3_qos_abmc_cfg inside cntr_config,  we may still have to 
keep it.

Thanks
Babu