LinuxLists.cc - [PATCH v11 00/13] x86/resctrl: Support for AMD QoS new features

[permalink] [raw]

Subject: [PATCH v11 02/13] x86/cpufeatures: Add Slow Memory Bandwidth Allocation feature flag

Add the new AMD feature X86_FEATURE_SMBA. With this feature, the QOS
enforcement policies can be applied to external slow memory connected
to the host. QOS enforcement is accomplished by assigning a Class Of
Service (COS) to a processor and specifying allocations or limits for
that COS for each resource to be allocated.

This feature is identified by the CPUID Function 8000_0020_EBX_x0.

CPUID Fn8000_0020_EBX_x0 AMD Bandwidth Enforcement Feature Identifiers
(ECX=0)

Bits Field Name Description
2 L3SBE L3 external slow memory bandwidth enforcement

CXL.memory is the only supported "slow" memory device. With the support
of SMBA feature, the hardware enables bandwidth allocation on the slow
memory devices. If there are multiple slow memory devices in the system,
then the throttling logic groups all the slow sources together and
applies the limit on them as a whole.

The presence of the SMBA feature(with CXL.memory) is independent of
whether slow memory device is actually present in the system. If there
is no slow memory in the system, then setting a SMBA limit will have no
impact on the performance of the system.

Presence of CXL memory can be identified by numactl command.

$numactl -H
available: 2 nodes (0-1)
node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
node 0 size: 63678 MB node 0 free: 59542 MB
node 1 cpus:
node 1 size: 16122 MB
node 1 free: 15627 MB
node distances:
node 0 1
0: 10 50
1: 50 10

CPU list for CXL memory will be empty. The cpu-cxl node distance is
greater than cpu-to-cpu distances. Node 1 has the CXL memory in this
case. CXL memory can also be identified using ACPI SRAT table and
memory maps.

Feature description is available in the specification, "AMD64
Technology Platform Quality of Service Extensions, Revision: 1.03
Publication # 56375 Revision: 1.03 Issue Date: February 2022".

Reviewed-by: Reinette Chatre <[email protected]>
Signed-off-by: Babu Moger <[email protected]>
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/scattered.c | 1 +
2 files changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 61012476d66e..00045123f418 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -307,6 +307,7 @@
#define X86_FEATURE_SGX_EDECCSSA (11*32+18) /* "" SGX EDECCSSA user leaf function */
#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 */

/* 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/scattered.c b/arch/x86/kernel/cpu/scattered.c
index f53944fb8f7f..d925753084fb 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -45,6 +45,7 @@ static const struct cpuid_bit cpuid_bits[] = {
{ X86_FEATURE_CPB, CPUID_EDX, 9, 0x80000007, 0 },
{ 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_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

2023-01-09 17:12:06

[permalink] [raw]

Subject: [PATCH v11 06/13] x86/resctrl: Detect and configure Slow Memory Bandwidth Allocation

The QoS slow memory configuration details are available via
CPUID_Fn80000020_EDX_x02. Detect the available details and
initialize the rest to defaults.

Reviewed-by: Reinette Chatre <[email protected]>
Signed-off-by: Babu Moger <[email protected]>
---
arch/x86/include/asm/msr-index.h | 1 +
arch/x86/kernel/cpu/resctrl/core.c | 36 +++++++++++++++++++++--
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 2 +-
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 8 +++--
4 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 37ff47552bcb..e0a40027aa62 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -1061,6 +1061,7 @@

/* - AMD: */
#define MSR_IA32_MBA_BW_BASE 0xc0000200
+#define MSR_IA32_SMBA_BW_BASE 0xc0000280

/* MSR_IA32_VMX_MISC bits */
#define MSR_IA32_VMX_MISC_INTEL_PT (1ULL << 14)
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 10a8c9d96f32..b4fc851f6489 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -162,6 +162,13 @@ bool is_mba_sc(struct rdt_resource *r)
if (!r)
return rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.mba_sc;

+ /*
+ * The software controller support is only applicable to MBA resource.
+ * Make sure to check for resource type.
+ */
+ if (r->rid != RDT_RESOURCE_MBA)
+ return false;
+
return r->membw.mba_sc;
}

@@ -225,9 +232,15 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r)
struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
union cpuid_0x10_3_eax eax;
union cpuid_0x10_x_edx edx;
- u32 ebx, ecx;
+ u32 ebx, ecx, subleaf;

- cpuid_count(0x80000020, 1, &eax.full, &ebx, &ecx, &edx.full);
+ /*
+ * Query CPUID_Fn80000020_EDX_x01 for MBA and
+ * CPUID_Fn80000020_EDX_x02 for SMBA
+ */
+ subleaf = (r->rid == RDT_RESOURCE_SMBA) ? 2 : 1;
+
+ cpuid_count(0x80000020, subleaf, &eax.full, &ebx, &ecx, &edx.full);
hw_res->num_closid = edx.split.cos_max + 1;
r->default_ctrl = MAX_MBA_BW_AMD;

@@ -750,6 +763,19 @@ static __init bool get_mem_config(void)
return false;
}

+static __init bool get_slow_mem_config(void)
+{
+ struct rdt_hw_resource *hw_res = &rdt_resources_all[RDT_RESOURCE_SMBA];
+
+ if (!rdt_cpu_has(X86_FEATURE_SMBA))
+ return false;
+
+ if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
+ return __rdt_get_mem_config_amd(&hw_res->r_resctrl);
+
+ return false;
+}
+
static __init bool get_rdt_alloc_resources(void)
{
struct rdt_resource *r;
@@ -780,6 +806,9 @@ static __init bool get_rdt_alloc_resources(void)
if (get_mem_config())
ret = true;

+ if (get_slow_mem_config())
+ ret = true;
+
return ret;
}

@@ -869,6 +898,9 @@ static __init void rdt_init_res_defs_amd(void)
} else if (r->rid == RDT_RESOURCE_MBA) {
hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
hw_res->msr_update = mba_wrmsr_amd;
+ } else if (r->rid == RDT_RESOURCE_SMBA) {
+ hw_res->msr_base = MSR_IA32_SMBA_BW_BASE;
+ hw_res->msr_update = mba_wrmsr_amd;
}
}
}
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 7eece3d2d0c3..eb07d4435391 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -209,7 +209,7 @@ static int parse_line(char *line, struct resctrl_schema *s,
unsigned long dom_id;

if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
- r->rid == RDT_RESOURCE_MBA) {
+ (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA)) {
rdt_last_cmd_puts("Cannot pseudo-lock MBA resource\n");
return -EINVAL;
}
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e4e6cdc1ee62..aa469d708991 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1208,7 +1208,7 @@ static bool rdtgroup_mode_test_exclusive(struct rdtgroup *rdtgrp)

list_for_each_entry(s, &resctrl_schema_all, list) {
r = s->res;
- if (r->rid == RDT_RESOURCE_MBA)
+ if (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA)
continue;
has_cache = true;
list_for_each_entry(d, &r->domains, list) {
@@ -1397,7 +1397,8 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
ctrl = resctrl_arch_get_config(r, d,
closid,
type);
- if (r->rid == RDT_RESOURCE_MBA)
+ if (r->rid == RDT_RESOURCE_MBA ||
+ r->rid == RDT_RESOURCE_SMBA)
size = ctrl;
else
size = rdtgroup_cbm_to_size(r, d, ctrl);
@@ -2832,7 +2833,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)

list_for_each_entry(s, &resctrl_schema_all, list) {
r = s->res;
- if (r->rid == RDT_RESOURCE_MBA) {
+ if (r->rid == RDT_RESOURCE_MBA ||
+ r->rid == RDT_RESOURCE_SMBA) {
rdtgroup_init_mba(r, rdtgrp->closid);
if (is_mba_sc(r))
continue;
--
2.34.1

2023-01-09 17:12:51

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Babu Moger <[email protected]>
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

2023-01-09 17:14:32

[permalink] [raw]

Subject: [PATCH v11 08/13] x86/resctrl: Support monitor configuration

Add a new field in struct mon_evt to support Bandwidth Monitoring Event
Configuration(BMEC) and also update the "mon_features" display.

The resctrl file "mon_features" will display the supported events
and files that can be used to configure those events if monitor
configuration is supported.

Before the change.
$cat /sys/fs/resctrl/info/L3_MON/mon_features
llc_occupancy
mbm_total_bytes
mbm_local_bytes

After the change when BMEC is supported.
$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

Reviewed-by: Reinette Chatre <[email protected]>
Signed-off-by: Babu Moger <[email protected]>
---
arch/x86/kernel/cpu/resctrl/internal.h | 2 ++
arch/x86/kernel/cpu/resctrl/monitor.c | 7 +++++++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 5 ++++-
3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index f16b8bc5448c..0605b04f1b7a 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -52,11 +52,13 @@ DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
* struct mon_evt - Entry in the event list of a resource
* @evtid: event id
* @name: name of the event
+ * @configurable: true if the event is configurable
* @list: entry in &rdt_resource->evt_list
*/
struct mon_evt {
enum resctrl_event_id evtid;
char *name;
+ bool configurable;
struct list_head list;
};

diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index e33e8d8bd796..b39e0eca1879 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -783,6 +783,13 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
if (ret)
return ret;

+ if (rdt_cpu_has(X86_FEATURE_BMEC)) {
+ if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL))
+ mbm_total_event.configurable = true;
+ if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL))
+ mbm_local_event.configurable = true;
+ }
+
l3_mon_evt_init(r);

r->mon_capable = true;
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index aa469d708991..f34c70c7a791 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -996,8 +996,11 @@ static int rdt_mon_features_show(struct kernfs_open_file *of,
struct rdt_resource *r = of->kn->parent->priv;
struct mon_evt *mevt;

- list_for_each_entry(mevt, &r->evt_list, list)
+ list_for_each_entry(mevt, &r->evt_list, list) {
seq_printf(seq, "%s\n", mevt->name);
+ if (mevt->configurable)
+ seq_printf(seq, "%s_config\n", mevt->name);
+ }

return 0;
}
--
2.34.1

2023-01-09 19:12:10

[permalink] [raw]

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

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

2023-01-09 20:33:45

[permalink] [raw]

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

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

2023-01-09 21:31:58

[permalink] [raw]

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

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

2023-01-09 21:52:27

by Luck, Tony

[permalink] [raw]

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

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

2023-01-09 22:06:24

by Luck, Tony

[permalink] [raw]

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

> 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

2023-01-09 22:11:31

[permalink] [raw]

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

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

2023-01-09 22:24:24

[permalink] [raw]

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

--
Regards/Gruss,
Boris.

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

2023-01-09 22:42:01

[permalink] [raw]

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

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

2023-01-09 23:54:18

[permalink] [raw]

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

[AMD Official Use Only - General]

> -----Original Message-----
> From: Borislav Petkov <[email protected]>
> Sent: Monday, January 9, 2023 3:52 PM
> To: Moger, Babu <[email protected]>
> Cc: Luck, Tony <[email protected]>; [email protected]; Chatre, Reinette
> <[email protected]>; [email protected]; [email protected]; Yu,
> Fenghua <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; akpm@linux-
> foundation.org; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Bae,
> Chang Seok <[email protected]>;
> [email protected]; [email protected];
> [email protected]; Das1, Sandipan <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Eranian, Stephane
> <[email protected]>; [email protected]; [email protected];
> Hunter, Adrian <[email protected]>; [email protected];
> [email protected]
> 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

2023-01-10 00:20:38

[permalink] [raw]

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

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

2023-01-24 11:29:01

[permalink] [raw]

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

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

2023-01-24 11:32:33

[permalink] [raw]

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

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

2023-01-24 11:35:11

[permalink] [raw]

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

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

2023-01-24 14:11:40

[permalink] [raw]

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

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

2023-01-24 15:11:14

[permalink] [raw]

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

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

2023-01-24 16:06:12

[permalink] [raw]

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

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

2023-01-24 17:00:13

[permalink] [raw]

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

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

2023-01-24 17:30:46

[permalink] [raw]

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

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

2023-01-24 17:48:24

[permalink] [raw]

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

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

2023-01-24 19:03:57

[permalink] [raw]

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

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

2023-01-24 19:11:53

[permalink] [raw]

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

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

2023-01-24 19:23:50

[permalink] [raw]

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

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/[email protected]/

2023-01-24 20:12:38