[PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag

Babu Moger posted 13 patches 2 years, 7 months ago
There is a newer version of this series
[PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Babu Moger 2 years, 7 months ago
Newer AMD processors support the new feature Bandwidth Monitoring Event
Configuration (BMEC).

The feature support is identified via CPUID Fn8000_0020_EBX_x0 (ECX=0).
Bits    Field Name    Description
3       EVT_CFG       Bandwidth Monitoring Event Configuration (BMEC)

The bandwidth monitoring events mbm_total_bytes and mbm_local_bytes are
set to count all the total and local reads/writes respectively. With the
introduction of slow memory, the two counters are not enough to count
all the different types of memory events. With the feature BMEC, the
users have the option to configure mbm_total_bytes and mbm_local_bytes
to count the specific type of events.

Each BMEC event has a configuration MSR, which contains one field for
each bandwidth type that can be used to configure the bandwidth event
to track any combination of supported bandwidth types. The event will
count requests from every bandwidth type bit that is set in the
corresponding configuration register.

Following are the types of events supported:

====    ========================================================
Bits    Description
====    ========================================================
6       Dirty Victims from the QOS domain to all types of memory
5       Reads to slow memory in the non-local NUMA domain
4       Reads to slow memory in the local NUMA domain
3       Non-temporal writes to non-local NUMA domain
2       Non-temporal writes to local NUMA domain
1       Reads to memory in the non-local NUMA domain
0       Reads to memory in the local NUMA domain
====    ========================================================

By default, the mbm_total_bytes configuration is set to 0x7F to count
all the event types and the mbm_local_bytes configuration is set to
0x15 to count all the local memory events.

Feature description is available in the specification, "AMD64
Technology Platform Quality of Service Extensions, Revision: 1.03
Publication".

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Link: https://www.amd.com/en/support/tech-docs/amd64-technology-platform-quality-service-extensions
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/cpuid-deps.c   | 2 ++
 arch/x86/kernel/cpu/scattered.c    | 1 +
 3 files changed, 4 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 00045123f418..db5287c06b65 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -308,6 +308,7 @@
 #define X86_FEATURE_CALL_DEPTH		(11*32+19) /* "" Call depth tracking for RSB stuffing */
 #define X86_FEATURE_MSR_TSX_CTRL	(11*32+20) /* "" MSR IA32_TSX_CTRL (Intel) implemented */
 #define X86_FEATURE_SMBA		(11*32+21) /* Slow Memory Bandwidth Allocation */
+#define X86_FEATURE_BMEC		(11*32+22) /* Bandwidth Monitoring Event Configuration */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */
 #define X86_FEATURE_AVX_VNNI		(12*32+ 4) /* AVX VNNI instructions */
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index d95221117129..f6748c8bd647 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -68,6 +68,8 @@ static const struct cpuid_dep cpuid_deps[] = {
 	{ X86_FEATURE_CQM_OCCUP_LLC,		X86_FEATURE_CQM_LLC   },
 	{ X86_FEATURE_CQM_MBM_TOTAL,		X86_FEATURE_CQM_LLC   },
 	{ X86_FEATURE_CQM_MBM_LOCAL,		X86_FEATURE_CQM_LLC   },
+	{ X86_FEATURE_BMEC,			X86_FEATURE_CQM_MBM_TOTAL   },
+	{ X86_FEATURE_BMEC,			X86_FEATURE_CQM_MBM_LOCAL   },
 	{ X86_FEATURE_AVX512_BF16,		X86_FEATURE_AVX512VL  },
 	{ X86_FEATURE_AVX512_FP16,		X86_FEATURE_AVX512BW  },
 	{ X86_FEATURE_ENQCMD,			X86_FEATURE_XSAVES    },
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index d925753084fb..0dad49a09b7a 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -46,6 +46,7 @@ static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_PROC_FEEDBACK,    CPUID_EDX, 11, 0x80000007, 0 },
 	{ X86_FEATURE_MBA,		CPUID_EBX,  6, 0x80000008, 0 },
 	{ X86_FEATURE_SMBA,		CPUID_EBX,  2, 0x80000020, 0 },
+	{ X86_FEATURE_BMEC,		CPUID_EBX,  3, 0x80000020, 0 },
 	{ X86_FEATURE_PERFMON_V2,	CPUID_EAX,  0, 0x80000022, 0 },
 	{ X86_FEATURE_AMD_LBR_V2,	CPUID_EAX,  1, 0x80000022, 0 },
 	{ 0, 0, 0, 0, 0 }
-- 
2.34.1
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Borislav Petkov 2 years, 7 months ago
On Mon, Jan 09, 2023 at 10:43:56AM -0600, Babu Moger wrote:
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 00045123f418..db5287c06b65 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -308,6 +308,7 @@
>  #define X86_FEATURE_CALL_DEPTH		(11*32+19) /* "" Call depth tracking for RSB stuffing */
>  #define X86_FEATURE_MSR_TSX_CTRL	(11*32+20) /* "" MSR IA32_TSX_CTRL (Intel) implemented */
>  #define X86_FEATURE_SMBA		(11*32+21) /* Slow Memory Bandwidth Allocation */
> +#define X86_FEATURE_BMEC		(11*32+22) /* Bandwidth Monitoring Event Configuration */

If those flags are meant only for internal kernel code use - it looks like it -
and userspace doesn't care, pls put "" in front of both in the comment like the
others above them do.

This way they won't be visible in /proc/cpuinfo.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Moger, Babu 2 years, 7 months ago
Hi Boris,

On 1/9/23 12:58, Borislav Petkov wrote:
> On Mon, Jan 09, 2023 at 10:43:56AM -0600, Babu Moger wrote:
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index 00045123f418..db5287c06b65 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -308,6 +308,7 @@
>>  #define X86_FEATURE_CALL_DEPTH		(11*32+19) /* "" Call depth tracking for RSB stuffing */
>>  #define X86_FEATURE_MSR_TSX_CTRL	(11*32+20) /* "" MSR IA32_TSX_CTRL (Intel) implemented */
>>  #define X86_FEATURE_SMBA		(11*32+21) /* Slow Memory Bandwidth Allocation */
>> +#define X86_FEATURE_BMEC		(11*32+22) /* Bandwidth Monitoring Event Configuration */
> If those flags are meant only for internal kernel code use - it looks like it -
> and userspace doesn't care, pls put "" in front of both in the comment like the
> others above them do.

All the QoS(or RDT) features are visible so far. If we make them visible,
users can easily figure out if this specific feature is supported or not.

There is some benefit if we make it visible. What do you think?

Thanks

Babu
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Borislav Petkov 2 years, 7 months ago
On Mon, Jan 09, 2023 at 01:49:09PM -0600, Moger, Babu wrote:
> All the QoS(or RDT) features are visible so far. If we make them visible,
> users can easily figure out if this specific feature is supported or not.

What would be the actual, real-life use case where the presence of those flags
in /proc/cpuinfo is really needed?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
RE: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Luck, Tony 2 years, 7 months ago
>> All the QoS(or RDT) features are visible so far. If we make them visible,
>> users can easily figure out if this specific feature is supported or not.
>
> What would be the actual, real-life use case where the presence of those flags
> in /proc/cpuinfo is really needed?

It feels like the old "rule" was "make it visible in /proc/cpuid" unless there was some
good reason NOT to do it.  But that has resulted in the "flags" line getting ridiculously
long and hard for humans to read (141 fields with 926 bytes on my Skylake, more on
more modern CPUs).

For RDT I don't see a lot of value in knowing that a feature is present ... all of them
have parameters on how many things they can control/monitor ... so you have to
either go parse the CPUID leaves, or just mount /sys/fs/resctrl and look in the "info"
directory to get the extra information you need to do anything with RDT.

I don't know if we'd break anything if we dropped:

  cat_l3 cdp_l3 mba cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local

from /proc/cpuinfo.

Perhaps the "rule" should be written in Documentation/{somewhere}?

-Tony
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Moger, Babu 2 years, 7 months ago
On 1/9/23 15:25, Luck, Tony wrote:
>>> All the QoS(or RDT) features are visible so far. If we make them visible,
>>> users can easily figure out if this specific feature is supported or not.
>> What would be the actual, real-life use case where the presence of those flags
>> in /proc/cpuinfo is really needed?
> It feels like the old "rule" was "make it visible in /proc/cpuid" unless there was some
> good reason NOT to do it.  But that has resulted in the "flags" line getting ridiculously
> long and hard for humans to read (141 fields with 926 bytes on my Skylake, more on
> more modern CPUs).
>
> For RDT I don't see a lot of value in knowing that a feature is present ... all of them
> have parameters on how many things they can control/monitor ... so you have to
> either go parse the CPUID leaves, or just mount /sys/fs/resctrl and look in the "info"
> directory to get the extra information you need to do anything with RDT.
>
> I don't know if we'd break anything if we dropped:
>
>   cat_l3 cdp_l3 mba cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local
>
> from /proc/cpuinfo.
>
> Perhaps the "rule" should be written in Documentation/{somewhere}?

Actually, these feature bits are referred in Documentation/x86/resctrl.rst

This feature is enabled by the CONFIG_X86_CPU_RESCTRL and the x86
/proc/cpuinfo
flag bits:

===============================================
================================
RDT (Resource Director Technology) Allocation   "rdt_a"
CAT (Cache Allocation Technology)               "cat_l3", "cat_l2"
CDP (Code and Data Prioritization)              "cdp_l3", "cdp_l2"
CQM (Cache QoS Monitoring)                      "cqm_llc", "cqm_occup_llc"
MBM (Memory Bandwidth Monitoring)               "cqm_mbm_total",
"cqm_mbm_local"
MBA (Memory Bandwidth Allocation)               "mba"
SMBA (Slow Memory Bandwidth Allocation)         "smba"
BMEC (Bandwidth Monitoring Event Configuration) "bmec"
===============================================
================================

So, if we remove them, we need to update here also.

Thanks

Babu


Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Borislav Petkov 2 years, 7 months ago
On Mon, Jan 09, 2023 at 03:44:30PM -0600, Moger, Babu wrote:
> So, if we remove them, we need to update here also.

We can "reroute" that documentation to /sys/fs/resctrl's info...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
RE: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Moger, Babu 2 years, 7 months ago
[AMD Official Use Only - General]



> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Monday, January 9, 2023 3:52 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: Luck, Tony <tony.luck@intel.com>; corbet@lwn.net; Chatre, Reinette
> <reinette.chatre@intel.com>; tglx@linutronix.de; mingo@redhat.com; Yu,
> Fenghua <fenghua.yu@intel.com>; dave.hansen@linux.intel.com;
> x86@kernel.org; hpa@zytor.com; paulmck@kernel.org; akpm@linux-
> foundation.org; quic_neeraju@quicinc.com; rdunlap@infradead.org;
> damien.lemoal@opensource.wdc.com; songmuchun@bytedance.com;
> peterz@infradead.org; jpoimboe@kernel.org; pbonzini@redhat.com; Bae,
> Chang Seok <chang.seok.bae@intel.com>;
> pawan.kumar.gupta@linux.intel.com; jmattson@google.com;
> daniel.sneddon@linux.intel.com; Das1, Sandipan <Sandipan.Das@amd.com>;
> james.morse@arm.com; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; bagasdotme@gmail.com; Eranian, Stephane
> <eranian@google.com>; christophe.leroy@csgroup.eu; jarkko@kernel.org;
> Hunter, Adrian <adrian.hunter@intel.com>; quic_jiles@quicinc.com;
> peternewman@google.com
> Subject: Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring
> Event Configuration feature flag
> 
> On Mon, Jan 09, 2023 at 03:44:30PM -0600, Moger, Babu wrote:
> > So, if we remove them, we need to update here also.
> 
> We can "reroute" that documentation to /sys/fs/resctrl's info...

Yes. We could. But at this point we don't have all the features listed in /sys/fs/resctrl/info directory. We need to add all the resctrl feature bits in info directory.
How about we take this as separate task and I can send separate series to address it?
Thanks
Babu
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Borislav Petkov 2 years, 7 months ago
On Mon, Jan 09, 2023 at 11:10:40PM +0000, Moger, Babu wrote:
> Yes. We could.
>
> But at this point we don't have all the features listed in /sys/fs/resctrl/info
> directory. We need to add all the resctrl feature bits in info directory. How
> about we take this as separate task and I can send separate series to address
> it?

See my reply to Reinette from just now and lemme know if something's not clear
yet.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Moger, Babu 2 years, 7 months ago
On 1/24/2023 5:34 AM, Borislav Petkov wrote:
> On Mon, Jan 09, 2023 at 11:10:40PM +0000, Moger, Babu wrote:
>> Yes. We could.
>>
>> But at this point we don't have all the features listed in /sys/fs/resctrl/info
>> directory. We need to add all the resctrl feature bits in info directory. How
>> about we take this as separate task and I can send separate series to address
>> it?
> See my reply to Reinette from just now and lemme know if something's not clear
> yet.

Understood. I am planning to add resctrl feature list inside 
/sys/fs/resctrl/info/ in my next series.

Thanks

Babu
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Borislav Petkov 2 years, 7 months ago
On Tue, Jan 24, 2023 at 08:11:21AM -0600, Moger, Babu wrote:
> Understood. I am planning to add resctrl feature list inside
> /sys/fs/resctrl/info/ in my next series.

Maybe I wasn't as clear as I hoped for:

so you have a couple of flags in /proc/cpuinfo which are actively being used by
tools.

Why would you want to move the flags somewhere else and do the extra work for no
apparent reason?

> We need to add all the resctrl feature bits in info directory.

What for?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Moger, Babu 2 years, 7 months ago
On 1/24/23 09:10, Borislav Petkov wrote:
> On Tue, Jan 24, 2023 at 08:11:21AM -0600, Moger, Babu wrote:
>> Understood. I am planning to add resctrl feature list inside
>> /sys/fs/resctrl/info/ in my next series.
> Maybe I wasn't as clear as I hoped for:
>
> so you have a couple of flags in /proc/cpuinfo which are actively being used by
> tools.
Those flags will be there. Not planning to remove them.
>
> Why would you want to move the flags somewhere else and do the extra work for no
> apparent reason?

With this series(v12) we have added two new cpuid features(SMBA and BMEC).

But these features are not visible in /proc/cpuinfo. Planning to add them
in /sys/fs/resctrl/info.

So, users can see them here. 

>
>> We need to add all the resctrl feature bits in info directory.
> What for?

Same reason as above.

Thanks

Babu


Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Reinette Chatre 2 years, 7 months ago
Hi Babu,

On 1/24/2023 8:06 AM, Moger, Babu wrote:
> 
> On 1/24/23 09:10, Borislav Petkov wrote:
>> On Tue, Jan 24, 2023 at 08:11:21AM -0600, Moger, Babu wrote:
>>> Understood. I am planning to add resctrl feature list inside
>>> /sys/fs/resctrl/info/ in my next series.
>> Maybe I wasn't as clear as I hoped for:
>>
>> so you have a couple of flags in /proc/cpuinfo which are actively being used by
>> tools.
> Those flags will be there. Not planning to remove them.
>>
>> Why would you want to move the flags somewhere else and do the extra work for no
>> apparent reason?
> 
> With this series(v12) we have added two new cpuid features(SMBA and BMEC).
> 
> But these features are not visible in /proc/cpuinfo. Planning to add them
> in /sys/fs/resctrl/info.
> 
> So, users can see them here. 

Could you please elaborate what you are planning to do? 

Existence and support for SMBA and BMEC is already visible to user space
in your current series:
* On a system that supports SMBA with the needed kernel support users will
  find the /sys/fs/resctrl/info/SMBA directory with enumerated properties
  as well as SMBA within the schemata file.
* On a system that supports BMEC with the needed kernel support users will
  find the relevant files listed within /sys/fs/resctrl/info/L3_MON/mon_features.

Reinette
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Moger, Babu 2 years, 7 months ago
On 1/24/23 10:59, Reinette Chatre wrote:
> Hi Babu,
>
> On 1/24/2023 8:06 AM, Moger, Babu wrote:
>> On 1/24/23 09:10, Borislav Petkov wrote:
>>> On Tue, Jan 24, 2023 at 08:11:21AM -0600, Moger, Babu wrote:
>>>> Understood. I am planning to add resctrl feature list inside
>>>> /sys/fs/resctrl/info/ in my next series.
>>> Maybe I wasn't as clear as I hoped for:
>>>
>>> so you have a couple of flags in /proc/cpuinfo which are actively being used by
>>> tools.
>> Those flags will be there. Not planning to remove them.
>>> Why would you want to move the flags somewhere else and do the extra work for no
>>> apparent reason?
>> With this series(v12) we have added two new cpuid features(SMBA and BMEC).
>>
>> But these features are not visible in /proc/cpuinfo. Planning to add them
>> in /sys/fs/resctrl/info.
>>
>> So, users can see them here. 
> Could you please elaborate what you are planning to do? 

Yes. It is sort of available. But, I wanted to add them explicit using the
already available function rdt_cpu_has().

Something like.

#cat /sys/fs/resctrl/info/features

 cmt, mbmtotal, mbmlocal, l3cat, mba, smba, bmec


Some of these features can be disabled using boot parameter options. So,
this will show only the features which enabled. 

Thanks

Babu

>
> Existence and support for SMBA and BMEC is already visible to user space
> in your current series:
> * On a system that supports SMBA with the needed kernel support users will
>   find the /sys/fs/resctrl/info/SMBA directory with enumerated properties
>   as well as SMBA within the schemata file.
> * On a system that supports BMEC with the needed kernel support users will
>   find the relevant files listed within /sys/fs/resctrl/info/L3_MON/mon_features.
>
> Reinette

-- 
Thanks
Babu Moger

Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Reinette Chatre 2 years, 7 months ago
Hi Babu,

On 1/24/2023 9:30 AM, Moger, Babu wrote:
> 
> On 1/24/23 10:59, Reinette Chatre wrote:
>> On 1/24/2023 8:06 AM, Moger, Babu wrote:
>>> On 1/24/23 09:10, Borislav Petkov wrote:
>>>> On Tue, Jan 24, 2023 at 08:11:21AM -0600, Moger, Babu wrote:
>>>>> Understood. I am planning to add resctrl feature list inside
>>>>> /sys/fs/resctrl/info/ in my next series.
>>>> Maybe I wasn't as clear as I hoped for:
>>>>
>>>> so you have a couple of flags in /proc/cpuinfo which are actively being used by
>>>> tools.
>>> Those flags will be there. Not planning to remove them.
>>>> Why would you want to move the flags somewhere else and do the extra work for no
>>>> apparent reason?
>>> With this series(v12) we have added two new cpuid features(SMBA and BMEC).
>>>
>>> But these features are not visible in /proc/cpuinfo. Planning to add them
>>> in /sys/fs/resctrl/info.
>>>
>>> So, users can see them here. 
>> Could you please elaborate what you are planning to do? 
> 
> Yes. It is sort of available. But, I wanted to add them explicit using the
> already available function rdt_cpu_has().
> 
> Something like.
> 
> #cat /sys/fs/resctrl/info/features
> 
>  cmt, mbmtotal, mbmlocal, l3cat, mba, smba, bmec
> 
> 
> Some of these features can be disabled using boot parameter options. So,
> this will show only the features which enabled. 
> 

From what I understand the only feature that needs additional help is CDP
and it appears in /proc/cpuinfo. For all other features
/sys/fs/resctrl/info already provides information when they are enabled, no?

Reinette

Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Moger, Babu 2 years, 7 months ago
On 1/24/23 11:47, Reinette Chatre wrote:
> Hi Babu,
>
> On 1/24/2023 9:30 AM, Moger, Babu wrote:
>> On 1/24/23 10:59, Reinette Chatre wrote:
>>> On 1/24/2023 8:06 AM, Moger, Babu wrote:
>>>> On 1/24/23 09:10, Borislav Petkov wrote:
>>>>> On Tue, Jan 24, 2023 at 08:11:21AM -0600, Moger, Babu wrote:
>>>>>> Understood. I am planning to add resctrl feature list inside
>>>>>> /sys/fs/resctrl/info/ in my next series.
>>>>> Maybe I wasn't as clear as I hoped for:
>>>>>
>>>>> so you have a couple of flags in /proc/cpuinfo which are actively being used by
>>>>> tools.
>>>> Those flags will be there. Not planning to remove them.
>>>>> Why would you want to move the flags somewhere else and do the extra work for no
>>>>> apparent reason?
>>>> With this series(v12) we have added two new cpuid features(SMBA and BMEC).
>>>>
>>>> But these features are not visible in /proc/cpuinfo. Planning to add them
>>>> in /sys/fs/resctrl/info.
>>>>
>>>> So, users can see them here. 
>>> Could you please elaborate what you are planning to do? 
>> Yes. It is sort of available. But, I wanted to add them explicit using the
>> already available function rdt_cpu_has().
>>
>> Something like.
>>
>> #cat /sys/fs/resctrl/info/features
>>
>>  cmt, mbmtotal, mbmlocal, l3cat, mba, smba, bmec
>>
>>
>> Some of these features can be disabled using boot parameter options. So,
>> this will show only the features which enabled. 
>>
> From what I understand the only feature that needs additional help is CDP
> and it appears in /proc/cpuinfo. For all other features
> /sys/fs/resctrl/info already provides information when they are enabled, no?

Yes. It is available.  But, the feature BMEC is not explicitly available.
I was thinking making all of them explicit. But we can live with that for
now. We can think about this in the future. Thanks for the comments.

Thanks

Babu

> Reinette
>
-- 
Thanks
Babu Moger

Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Reinette Chatre 2 years, 7 months ago
Hi Babu,

On 1/24/2023 11:03 AM, Moger, Babu wrote:
> 
> Yes. It is available.  But, the feature BMEC is not explicitly available.

I think your addition [1] to the resctrl documentation explains
well how user space can determine which parts of BMEC are available:

		If the system supports Bandwidth Monitoring Event
		Configuration (BMEC), then the bandwidth events will
		be configurable. The output will be::

			# cat /sys/fs/resctrl/info/L3_MON/mon_features
			llc_occupancy
			mbm_total_bytes
			mbm_total_bytes_config
			mbm_local_bytes
			mbm_local_bytes_config


Reinette

[1] https://lore.kernel.org/lkml/20230113152039.770054-14-babu.moger@amd.com/

Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Moger, Babu 2 years, 7 months ago
Hi Reinette,

On 1/24/23 13:23, Reinette Chatre wrote:
> Hi Babu,
>
> On 1/24/2023 11:03 AM, Moger, Babu wrote:
>> Yes. It is available.  But, the feature BMEC is not explicitly available.
> I think your addition [1] to the resctrl documentation explains
> well how user space can determine which parts of BMEC are available:
>
> 		If the system supports Bandwidth Monitoring Event
> 		Configuration (BMEC), then the bandwidth events will
> 		be configurable. The output will be::
>
> 			# cat /sys/fs/resctrl/info/L3_MON/mon_features
> 			llc_occupancy
> 			mbm_total_bytes
> 			mbm_total_bytes_config
> 			mbm_local_bytes
> 			mbm_local_bytes_config
>
Yes. Sure. That works.

Thank you.

Babu

Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Borislav Petkov 2 years, 7 months ago
On Tue, Jan 24, 2023 at 01:03:32PM -0600, Moger, Babu wrote:
> Yes. It is available.  But, the feature BMEC is not explicitly available.
> I was thinking making all of them explicit. But we can live with that for
> now. We can think about this in the future.

Yes, you can always think about a solution when the requirement presents itself.
What I find the wrong approach is thinking that someone *might* need it and then
devising some solution. Wait for the actual use case first and then think of a
proper solution.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Borislav Petkov 2 years, 7 months ago
On Mon, Jan 09, 2023 at 09:25:32PM +0000, Luck, Tony wrote:
> It feels like the old "rule" was "make it visible in /proc/cpuid" unless there was some
> good reason NOT to do it.  But that has resulted in the "flags" line getting ridiculously
> long and hard for humans to read (141 fields with 926 bytes on my Skylake,
> more on more modern CPUs).

Yap, imagine every possible CPUID bit were in there...

> I don't know if we'd break anything if we dropped:
> 
>   cat_l3 cdp_l3 mba cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local
> 
> from /proc/cpuinfo.

I wouldn't mind if we remove them from cpuinfo, frankly.

> Perhaps the "rule" should be written in Documentation/{somewhere}?

This started documenting it:

Documentation/x86/cpuinfo.rst

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Reinette Chatre 2 years, 7 months ago
Hi Boris and Tony,

On 1/9/2023 1:39 PM, Borislav Petkov wrote:
> On Mon, Jan 09, 2023 at 09:25:32PM +0000, Luck, Tony wrote:
>> It feels like the old "rule" was "make it visible in /proc/cpuid" unless there was some
>> good reason NOT to do it.  But that has resulted in the "flags" line getting ridiculously
>> long and hard for humans to read (141 fields with 926 bytes on my Skylake,
>> more on more modern CPUs).
> 
> Yap, imagine every possible CPUID bit were in there...
> 
>> I don't know if we'd break anything if we dropped:
>>
>>   cat_l3 cdp_l3 mba cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local
>>
>> from /proc/cpuinfo.
> 
> I wouldn't mind if we remove them from cpuinfo, frankly.

I am afraid that I am not aware of all the resctrl user space apps and
tools being used.

I did take a quick look at intel-cmt-cat that has an active user base and
from what I can tell it uses /proc/cpuinfo to learn some capabilities:

Example of looking for "cqm" (although "cqm" is not in Tony's list):
https://github.com/intel/intel-cmt-cat/blob/master/lib/os_cap.c#L420

Example of looking for "cdp_l3":
https://github.com/intel/intel-cmt-cat/blob/master/lib/os_cap.c#L520

Example of looking for "cdp_l2":
https://github.com/intel/intel-cmt-cat/blob/master/lib/os_cap.c#L564

> 
>> Perhaps the "rule" should be written in Documentation/{somewhere}?
> 
> This started documenting it:
> 
> Documentation/x86/cpuinfo.rst
> 

We could make a rule that no more resctrl related features are added to
cpuinfo but I am hesitant to remove the ones that are already there. 

Reinette
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Borislav Petkov 2 years, 7 months ago
On Mon, Jan 09, 2023 at 03:43:11PM -0800, Reinette Chatre wrote:
> We could make a rule that no more resctrl related features are added to
> cpuinfo but I am hesitant to remove the ones that are already there.

Yes, that makes sense.

And note that we try for /proc/cpuinfo to contain flags where the kernel has
received (substantial) enablement work to support a feature. Shadow stack would
be one good example.

If resctrl needs to use a feature and it cannot use that feature without kernel
enablement, then yes, by all means, it should use /proc/cpuinfo and not the
corresponding CPUID bit. Because the presence of the flag in /proc/cpuinfo says
"yes, you can use the feature and the kernel you're running on has the required
support."

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
RE: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Luck, Tony 2 years, 7 months ago
> This started documenting it:
>
> Documentation/x86/cpuinfo.rst

That does a good job to explain HOW everything works. But doesn't
really cover whether a field should be made visible. The section on ""
just says to use it:

  "if it does not make sense for the feature to be exposed to userspace"

But that allows for the flimsiest of reasons to used to justify making a
flag visible.

-Tony
Re: [PATCH v11 04/13] x86/cpufeatures: Add Bandwidth Monitoring Event Configuration feature flag
Posted by Borislav Petkov 2 years, 7 months ago
On Mon, Jan 09, 2023 at 09:50:20PM +0000, Luck, Tony wrote:
> But that allows for the flimsiest of reasons to used to justify making a
> flag visible.

How's that for starters?

c: The naming override can be "", which means it will not appear in /proc/cpuinfo.
----------------------------------------------------------------------------------

The feature shall be omitted from /proc/cpuinfo if there is no valid use case
for userspace to query this flag and cannot rely on other means for detecting
feature support. For example, toolchains do use CPUID directly instead of
relying on the kernel providing that info.

If unsure, that flag can always be omitted initially and, once a valid use case
presents itself, be shown later. Not the other way around.

Another example is X86_FEATURE_ALWAYS, defined in cpufeatures.h. That flag is an
internal kernel feature used in the alternative runtime patching functionality.
So, its name is overridden with "". Its flag will not appear in /proc/cpuinfo
because it absolutely does not make any sense to appear there.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette