| 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
| ||
|
Message-ID: <2e774d31-d63c-4bc8-b88a-1cee74533a5d@amd.com>
Date: Fri, 22 Nov 2024 15:04:28 -0600
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, babu.moger@....com,
corbet@....net, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com
Cc: fenghua.yu@...el.com, x86@...nel.org, hpa@...or.com, thuth@...hat.com,
paulmck@...nel.org, rostedt@...dmis.org, akpm@...ux-foundation.org,
xiongwei.song@...driver.com, pawan.kumar.gupta@...ux.intel.com,
daniel.sneddon@...ux.intel.com, perry.yuan@....com, sandipan.das@....com,
kai.huang@...el.com, xiaoyao.li@...el.com, seanjc@...gle.com,
jithu.joseph@...el.com, brijesh.singh@....com, xin3.li@...el.com,
ebiggers@...gle.com, andrew.cooper3@...rix.com, mario.limonciello@....com,
james.morse@....com, tan.shaopeng@...itsu.com, tony.luck@...el.com,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
peternewman@...gle.com, maciej.wieczor-retman@...el.com, eranian@...gle.com,
jpoimboe@...nel.org, thomas.lendacky@....com
Subject: Re: [PATCH v9 18/26] x86/resctrl: Add the interface to assign/update
counter assignment
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@....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().
> 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));
- Babu Moger
Powered by blists - more mailing lists