| 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: <Z__vIEObB27Rq7Le@agluck-desk3>
Date: Wed, 16 Apr 2025 10:55:44 -0700
From: "Luck, Tony" <tony.luck@...el.com>
To: "Moger, Babu" <babu.moger@....com>
Cc: Reinette Chatre <reinette.chatre@...el.com>, peternewman@...gle.com,
corbet@....net, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
paulmck@...nel.org, akpm@...ux-foundation.org, thuth@...hat.com,
rostedt@...dmis.org, ardb@...nel.org, gregkh@...uxfoundation.org,
daniel.sneddon@...ux.intel.com, jpoimboe@...nel.org,
alexandre.chartre@...cle.com, pawan.kumar.gupta@...ux.intel.com,
thomas.lendacky@....com, perry.yuan@....com, seanjc@...gle.com,
kai.huang@...el.com, xiaoyao.li@...el.com,
kan.liang@...ux.intel.com, xin3.li@...el.com, ebiggers@...gle.com,
xin@...or.com, sohil.mehta@...el.com, andrew.cooper3@...rix.com,
mario.limonciello@....com, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, maciej.wieczor-retman@...el.com,
eranian@...gle.com
Subject: Re: [PATCH v12 14/26] x86/resctrl: Add the functionality to assign
MBM events
On Wed, Apr 16, 2025 at 12:09:52PM -0500, Moger, Babu wrote:
> Hi Reinette,
>
> On 4/15/25 11:53, Reinette Chatre wrote:
> > Hi Babu,
> >
> > On 4/15/25 7:20 AM, Moger, Babu wrote:
> >> Hi Reinette,
> >>
> >> On 4/11/25 16:04, Reinette Chatre wrote:
> >>> Hi Babu,
> >>>
> >>> On 4/3/25 5:18 PM, Babu Moger wrote:
> >>>> The mbm_cntr_assign mode offers "num_mbm_cntrs" number of counters that
> >>>> can be assigned to an RMID, event pair and monitor the bandwidth as long
> >>>> as it is assigned.
> >>>
> >>> Above makes it sound as though multiple counters can be assigned to
> >>> an RMID, event pair.
> >>>
> >>
> >> Yes. Multiple counter-ids can be assigned to RMID, event pair.
> >
> > oh, are you referring to the assignments of different counters across multiple
> > domains?
>
> May be I am confusing you here. This is what I meant.
>
> Here is one example.
>
> In a same group,
> Configure cntr_id 0, to count reads only (This maps to total event).
> Configure cntr_id 1, to count write only (This maps to local event).
> Configure cntr_id 2, to count dirty victims.
> so on..
> so on..
> Configure cntr_id 31, to count remote read only.
>
> We have 32 counter ids in a domain. Basically, we can configure all the
> counters in a domain to just one group if you want to.
>
> We cannot do that right now because our data structures cannot do that.
> We can only configure 2 events(local and total) right now.
Not just data structures, but also user visible files in
mon_data/mon_L3*/*
You'd need to create a new file for each counter.
My patch for making it easier to add more counters:
https://lore.kernel.org/all/20250407234032.241215-3-tony.luck@intel.com/
may help ... though you have to pick the number of simultaneous counters
at compile time to size the arrays in the domain structures:
struct mbm_state *mbm_states[QOS_NUM_MBM_EVENTS];
and if you are dynamically adding/removing events using the
configuration files, need to alloc/free the memory that those
arrays of pointers reference ... as well as adding/removing files
from the appropriate mon_data/mon_L3* directory.
> My understanding it is same with MPAM also.
>
> >
> >>
> >>>>
> >>>> Add the functionality to allocate and assign the counters to RMID, event
> >>>> pair in the domain.
> >>>
> >>> "assign *a* counter to an RMID, event pair"?
> >>
> >> Sure.
> >>
> >>>
> >>>>
> >>>> If all the counters are in use, the kernel will log the error message
> >>>> "Unable to allocate counter in domain" in /sys/fs/resctrl/info/
> >>>> last_cmd_status when a new assignment is requested. Exit on the first
> >>>> failure when assigning counters across all the domains.
> >>>>
> >>>> Signed-off-by: Babu Moger <babu.moger@....com>
> >>>> ---
> >>>
> >>> ...
> >>>
> >>>> ---
> >>>> arch/x86/kernel/cpu/resctrl/internal.h | 2 +
> >>>> arch/x86/kernel/cpu/resctrl/monitor.c | 124 +++++++++++++++++++++++++
> >>>> 2 files changed, 126 insertions(+)
> >>>>
> >>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> >>>> index 0b73ec451d2c..1a8ac511241a 100644
> >>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> >>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> >>>> @@ -574,6 +574,8 @@ bool closid_allocated(unsigned int closid);
> >>>> int resctrl_find_cleanest_closid(void);
> >>>> void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
> >>>> unsigned int mon_event_config_index_get(u32 evtid);
> >>>> +int resctrl_assign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
> >>>> + struct rdtgroup *rdtgrp, enum resctrl_event_id evtid, u32 evt_cfg);
> >>>
> >>> This is internal to resctrl fs. Why is it needed to provide both the event id
> >>> and the event configuration? Event configuration can be determined from event ID?
> >>
> >> Yes. It can be done. Then I have to export the functions like
> >> mbm_get_assign_config() into monitor.c. To avoid that I passed it from
> >> here which I felt much more cleaner.
> >
> >>From what I can tell, for example by looking at patch #22, callers of
> > resctrl_assign_cntr_event() now need to call mbm_get_assign_config()
> > every time before calling resctrl_assign_cntr_event(). Calling
> > mbm_get_assign_config() from within resctrl_assign_cntr_event() seems
> > simpler to me and that may result in mbm_get_assign_config() moving to
> > monitor.c as an extra benefit.
>
> Sure.
>
> >
> > ...
> >
> >>>> +static int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
> >>>> + struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
> >>>> +{
> >>>> + int cntr_id;
> >>>> +
> >>>> + for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
> >>>> + if (d->cntr_cfg[cntr_id].rdtgrp == rdtgrp &&
> >>>> + d->cntr_cfg[cntr_id].evtid == evtid)
> >>>> + return cntr_id;
> >>>> + }
> >>>> +
> >>>> + return -ENOENT;
> >>>> +}
> >>>> +
> >>>> +static int mbm_cntr_alloc(struct rdt_resource *r, struct rdt_mon_domain *d,
> >>>> + struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
> >>>> +{
> >>>> + int cntr_id;
> >>>> +
> >>>> + for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
> >>>> + if (!d->cntr_cfg[cntr_id].rdtgrp) {
> >>>> + d->cntr_cfg[cntr_id].rdtgrp = rdtgrp;
> >>>> + d->cntr_cfg[cntr_id].evtid = evtid;
> >>>> + return cntr_id;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + return -ENOSPC;
> >>>> +}
> >>>> +
> >>>> +static void mbm_cntr_free(struct rdt_mon_domain *d, int cntr_id)
> >>>> +{
> >>>> + memset(&d->cntr_cfg[cntr_id], 0, sizeof(struct mbm_cntr_cfg));
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * Allocate a fresh counter and configure the event if not assigned already.
> >>>> + */
> >>>> +static int resctrl_alloc_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> >>>> + struct rdtgroup *rdtgrp, enum resctrl_event_id evtid,
> >>>> + u32 evt_cfg)
> >>>
> >>> Same here, why are both evtid and evt_cfg provided as arguments?
> >>
> >> Yes. It can be done. Then I have to export the functions like
> >> mbm_get_assign_config() into monitor.c. To avoid that I passed it from
> >> here which I felt much more cleaner.
> >
> > Maybe even resctrl_assign_cntr_event() does not need to call mbm_get_assign_config()
> > but only resctrl_alloc_config_cntr() needs to call mbm_get_assign_config(). Doing so
> > may avoid more burden on callers while reducing parameters needed throughout.
> >
>
> ok. Sure. Will do.
>
> --
> Thanks
> Babu Moger
-Tony
Powered by blists - more mailing lists