linux-kernel - Re: [PATCH v10 15/24] x86/resctrl: Implement resctrl_arch_config

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eb84c593-3f16-49e7-98e9-75a13b8b5890@amd.com>
Date: Sat, 21 Dec 2024 07:40:57 -0600
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Babu Moger <babu.moger@....com>, corbet@....net, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
 tony.luck@...el.com, peternewman@...gle.com
Cc: fenghua.yu@...el.com, x86@...nel.org, hpa@...or.com, paulmck@...nel.org,
 akpm@...ux-foundation.org, thuth@...hat.com, rostedt@...dmis.org,
 xiongwei.song@...driver.com, pawan.kumar.gupta@...ux.intel.com,
 daniel.sneddon@...ux.intel.com, jpoimboe@...nel.org, perry.yuan@....com,
 sandipan.das@....com, kai.huang@...el.com, xiaoyao.li@...el.com,
 seanjc@...gle.com, xin3.li@...el.com, andrew.cooper3@...rix.com,
 ebiggers@...gle.com, mario.limonciello@....com, james.morse@....com,
 tan.shaopeng@...itsu.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, maciej.wieczor-retman@...el.com,
 eranian@...gle.com
Subject: Re: [PATCH v10 15/24] x86/resctrl: Implement
 resctrl_arch_config_cntr() to assign a counter with ABMC

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