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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e800c0d1-fad7-4388-93ce-0a7a58c4d340@amd.com>
Date: Fri, 20 Dec 2024 16:28:22 -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 3:41 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 12/20/24 11:22 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 12/19/2024 5:04 PM, Reinette Chatre wrote:
>>> (andipan.das@....com -> sandipan.das@....com to stop sending undeliverable emails)
>>
>> Yes.
>>
>>>
>>> Hi Babu,
>>>
>>> On 12/12/24 12:15 PM, Babu Moger wrote:
>>>> The ABMC feature provides an option to the user to assign a hardware
>>>> counter to an RMID, event pair and monitor the bandwidth as long as it is
>>>> assigned. The assigned RMID will be tracked by the hardware until the user
>>>> unassigns it manually.
>>>>
>>>> Configure the counters by writing to the L3_QOS_ABMC_CFG MSR and specifying
>>>> the counter ID, bandwidth source (RMID), and bandwidth event configuration.
>>>>
>>>> Provide the interface to assign the counter ids to RMID.
>>>
>>> Until now in this series many patches "introduced interface X" and every
>>> time it was some new resctrl file that user space interacts with. This
>>> changelog starts with a context about "user to assign a hardware counter"
>>> and ends with "Provide the interface", but there is no new user interface
>>> in this patch. Can this be more specific about what this patch does?
>>
>> Yes. This should be about resctrl_arch_config_cntr(). How about this?
>>
>> The ABMC feature provides an option to the user to assign a hardware
>> counter to an RMID, event pair and monitor the bandwidth as long as it is assigned. The assigned RMID will be tracked by the hardware until the user unassigns it manually.
>>
>> Provide the architecture specific handler to to assign/unassign the counter. Counters are configured by writing to the L3_QOS_ABMC_CFG MSR and specifying the counter ID, bandwidth source (RMID), and bandwidth event configuration.
> 
> Again just one sentence appended. The "to to" demonstrates it is another
> example of something typed quickly to see if it sticks.

My bad. Will rewrite this.

> 
> 
>>>> @@ -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?


> 
>>>> @@ -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.

Thanks
Babu
> 
> Reinette
>