[PATCH V3 0/4] x86/resctrl: Enable user to view and select thread throttling mode

[PATCH V3 0/4] x86/resctrl: Enable user to view and select thread throttling mode

[Reinette Chatre]

V2 upstream submission available from:
https://lore.kernel.org/lkml/cover.1586801373.git.reinette.chatre@intel.com

Changes since V2:
- Rebase on top of recently merged series "x86/resctrl: Support wider
  MBM counters". Small change needed to take into account
  asm/resctrl_sched.h -> asm/resctrl.h name change.
- Fix rST formatting of documentation (resctrl_ui.rst) describing
  new "thread_throttle_mode" resctrl file.
- Use boot_cpu_has() instead of static_cpu_has() when determining what
  to display to user (slow path).

V1 upstream submission available from:
https://lore.kernel.org/lkml/cover.1585765499.git.reinette.chatre@intel.com

A notable change since V1 is the inclusion of two additional patches from
Fenghua Yu that introduce the new per-thread MBA feature. These changes are
added to this series because they are small and closely related to the
original submission. The per-thread MBA feature is a hardware advancement
that requires no software interface changes. The patches added just enumerate
the feature and expose it to userspace by showing "per-thread" in the new
resctrl file "thread_throttle_mode" to help user applications fine tune
performance.

There are currently a few resctrl changes outstanding for upstream inclusion.
To support their consideration all outstanding resctrl patches can be
viewed at https://github.com/rchatre/linux.git (branch resctrl/next)

Changes since V1 (also documented within patches to which they apply):
- Rebased on top of James Morse's CDP fix
(https://lore.kernel.org/lkml/20200221162105.154163-1-james.morse@arm.com)
- Remove RF_UNINITIALIZED (having uninitialized be represented with ones
  creates too much confusion), replace with an explicit check of rft->fflags
  in rdtgroup_add_files() (Fenghua Yu)
- Rename MBA_THREAD_THROTTLE_MODE to MBA_THROTTLE_MODE_MASK to clarify its
  use as a mask (Tony Luck)
- Introduce explicit MBA_THROTTLE_MODE_MAX instead of implying it is the
  opposite of min and use these values (min and max) explicitly whenever
  testing/setting the throttle mode value (Tony Luck)
- Add __init attribute to thread_throttle_mode_init_intel_rw() and
  thread_throttle_mode_init_intel_ro() since they are only needed during
  initialization (Fenghua Yu)
- Remove MBA_CFG MSR reads and error checking so that the patch is simpler
  and easier to review (Fenghua Yu)
- Ensure CPU hotplug lock is taken when writing register on multiple CPUs (Fenghua Yu)
- Use CPU mask already maintained as part of domains to determine which
  CPUs to update MBA register on (Fenghua Yu)
- Maintain MBA configuration register contents to support use case when not
  all CPUs of a package are online when configuration is set from user
  space
- Use seq_puts() instead of seq_printf() when simple strings are printed
- Set MBA configuration to default when resctrl is unmounted
- Complete rewrite of "thread_throttle_mode" documentation (Tony Luck)
- Remove unnecessary checks on user input (Andy Shevchenko)
- Change code style surrounding usage of sysfs_match_string() (Andy Shevchenko)

From V1 submission:

The first patch in this series introduces a new resctrl file,
"thread_throttle_mode", on Intel systems that exposes to the
user how per-thread values are allocated to a core. This is added in
support of newer Intel systems that can be configured to allocate
either maximum or minimum throttling of the per-thread CLOS values
to the core.

Details about the feature can be found in the commit description and
in Chapter 9 of the most recent Intel ISE available from
https://software.intel.com/sites/default/files/managed/c5/15/architecture-instruction-set-extensions-programming-reference.pdf

The first patch parses user input with the appropriate sysfs API that has
not previously been used in resctrl. The second (later in the fourth) patch is
added as a subsequent cleanup that switches existing resctrl string parsing
code to also use this appropriate API.

Fenghua Yu (2):
  x86/resctrl: Enumerate per-thread MBA
  x86/resctrl: Enable per-thread MBA

Reinette Chatre (2):
  x86/resctrl: Enable user to view and select thread throttling mode
  x86/resctrl: Use appropriate API for strings terminated by newline

 Documentation/x86/resctrl_ui.rst       |  22 ++-
 arch/x86/include/asm/cpufeatures.h     |   1 +
 arch/x86/kernel/cpu/cpuid-deps.c       |   1 +
 arch/x86/kernel/cpu/resctrl/core.c     |  32 ++++
 arch/x86/kernel/cpu/resctrl/internal.h |  13 ++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 231 +++++++++++++++++++++++--
 arch/x86/kernel/cpu/scattered.c        |   1 +
 7 files changed, 283 insertions(+), 18 deletions(-)

-- 
2.21.0

[PATCH V3 1/4] x86/resctrl: Enable user to view and select thread throttling mode

[Reinette Chatre]

Intel Memory Bandwidth Allocation (MBA) control is provided per
processor core. At the same time different CLOS, configured with different
bandwidth percentages, can be assigned to the hardware threads
sharing a core. In the original implementation of MBA the maximum throttling
of the per-thread CLOS is allocated to the core. Specifically, the lower
bandwidth percentage is allocated to the core.

Newer systems can be configured to allocate either maximum or
minimum throttling of the per-thread CLOS values to the core.

Introduce a new resctrl file, "thread_throttle_mode", on Intel systems
that exposes to the user how per-thread values are allocated to
a core. On systems that support the original MBA implementation the
file will always display "max". On systems that can be configured
the possible values are "min" or "max" that the user can modify by
writing these same words to the file.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
Changes since V2:
- Rebase on top of recently merged series "x86/resctrl: Support wider
  MBM counters". Small change needed to take into account
  asm/resctrl_sched.h -> asm/resctrl.h name change.
- Fix rST formatting of documentation (resctrl_ui.rst) describing
  new "thread_throttle_mode" resctrl file.

 Documentation/x86/resctrl_ui.rst       |  19 ++-
 arch/x86/kernel/cpu/resctrl/core.c     |  32 +++++
 arch/x86/kernel/cpu/resctrl/internal.h |  13 ++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 188 ++++++++++++++++++++++++-
 4 files changed, 249 insertions(+), 3 deletions(-)

diff --git a/Documentation/x86/resctrl_ui.rst b/Documentation/x86/resctrl_ui.rst
index 5368cedfb530..861ee2816470 100644
--- a/Documentation/x86/resctrl_ui.rst
+++ b/Documentation/x86/resctrl_ui.rst
@@ -138,6 +138,19 @@ with respect to allocation:
 		non-linear. This field is purely informational
 		only.
 
+"thread_throttle_mode":
+		Indicator (on some CPU models control) on Intel systems
+		of how tasks running on threads of a physical core are
+		throttled in cases where they request different memory
+		bandwidth percentages:
+
+		"min":
+			the largest percentage is applied
+			to all threads
+		"max":
+			the smallest percentage is applied
+			to all threads
+
 If RDT monitoring is available there will be an "L3_MON" directory
 with the following files:
 
@@ -364,8 +377,10 @@ to the next control step available on the hardware.
 
 The bandwidth throttling is a core specific mechanism on some of Intel
 SKUs. Using a high bandwidth and a low bandwidth setting on two threads
-sharing a core will result in both threads being throttled to use the
-low bandwidth. The fact that Memory bandwidth allocation(MBA) is a core
+sharing a core may result in both threads being throttled to use the
+low bandwidth (see "thread_throttle_mode").
+
+The fact that Memory bandwidth allocation(MBA) may be a core
 specific mechanism where as memory bandwidth monitoring(MBM) is done at
 the package level may lead to confusion when users try to apply control
 via the MBA and then monitor the bandwidth to see if the controls are
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 12f967c6b603..1bc686777069 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -250,6 +250,30 @@ static inline bool rdt_get_mb_table(struct rdt_resource *r)
 	return false;
 }
 
+/*
+ * Model-specific test to determine if platform where memory bandwidth
+ * control is applied to a core can be configured to apply either the
+ * maximum or minimum of the per-thread delay values.
+ * By default, platforms where memory bandwidth control is applied to a
+ * core will select the maximum delay value of the per-thread CLOS.
+ *
+ * NOTE: delay value programmed to hardware is inverse of bandwidth
+ * percentage configured via user interface.
+ */
+bool mba_cfg_supports_min_max_intel(void)
+{
+	switch (boot_cpu_data.x86_model) {
+	case INTEL_FAM6_ATOM_TREMONT_D:
+	case INTEL_FAM6_ICELAKE_X:
+	case INTEL_FAM6_ICELAKE_D:
+		return true;
+	default:
+		return false;
+	}
+
+	return false;
+}
+
 static bool __get_mem_config_intel(struct rdt_resource *r)
 {
 	union cpuid_0x10_3_eax eax;
@@ -270,6 +294,11 @@ static bool __get_mem_config_intel(struct rdt_resource *r)
 	}
 	r->data_width = 3;
 
+	if (mba_cfg_supports_min_max_intel())
+		thread_throttle_mode_init_intel_rw();
+	else
+		thread_throttle_mode_init_intel_ro();
+
 	r->alloc_capable = true;
 	r->alloc_enabled = true;
 
@@ -580,6 +609,9 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
 
 	rdt_domain_reconfigure_cdp(r);
 
+	if (mba_cfg_supports_min_max_intel())
+		wrmsrl(MSR_MBA_CFG, mba_cfg_msr);
+
 	if (r->alloc_capable && domain_setup_ctrlval(r, d)) {
 		kfree(d);
 		return;
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index f20a47d120b1..c5f4cb91009b 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -9,6 +9,7 @@
 
 #define MSR_IA32_L3_QOS_CFG		0xc81
 #define MSR_IA32_L2_QOS_CFG		0xc82
+#define MSR_MBA_CFG			0xc84
 #define MSR_IA32_L3_CBM_BASE		0xc90
 #define MSR_IA32_L2_CBM_BASE		0xd10
 #define MSR_IA32_MBA_THRTL_BASE		0xd50
@@ -21,6 +22,9 @@
 
 #define L2_QOS_CDP_ENABLE		0x01ULL
 
+#define MBA_THROTTLE_MODE_MIN		0x01ULL
+#define MBA_THROTTLE_MODE_MAX		0x00ULL
+
 /*
  * Event IDs are used to program IA32_QM_EVTSEL before reading event
  * counter from IA32_QM_CTR
@@ -38,6 +42,8 @@
 #define MBA_MAX_MBPS			U32_MAX
 #define MAX_MBA_BW_AMD			0x800
 
+#define MBA_THROTTLE_MODE_MASK		BIT_ULL(0)
+
 #define RMID_VAL_ERROR			BIT_ULL(63)
 #define RMID_VAL_UNAVAIL		BIT_ULL(62)
 /*
@@ -47,6 +53,10 @@
  */
 #define MBM_CNTR_WIDTH_OFFSET_MAX (62 - MBM_CNTR_WIDTH_BASE)
 
+/*
+ * MSR_MBA_CFG cache
+ */
+extern u64 mba_cfg_msr;
 
 struct rdt_fs_context {
 	struct kernfs_fs_context	kfc;
@@ -611,5 +621,8 @@ void __check_limbo(struct rdt_domain *d, bool force_free);
 bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
 bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r);
 void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
+bool mba_cfg_supports_min_max_intel(void);
+void thread_throttle_mode_init_intel_rw(void);
+void thread_throttle_mode_init_intel_ro(void);
 
 #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index d7cb5ab0d1f0..6a9408060ac4 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -29,6 +29,7 @@
 
 #include <uapi/linux/magic.h>
 
+#include <asm/intel-family.h>
 #include <asm/resctrl.h>
 #include "internal.h"
 
@@ -38,6 +39,7 @@ DEFINE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
 static struct kernfs_root *rdt_root;
 struct rdtgroup rdtgroup_default;
 LIST_HEAD(rdt_all_groups);
+u64 mba_cfg_msr;
 
 /* Kernel fs node for "info" directory under root */
 static struct kernfs_node *kn_info;
@@ -1017,6 +1019,134 @@ static int max_threshold_occ_show(struct kernfs_open_file *of,
 	return 0;
 }
 
+/*
+ * As documented in the Intel SDM, on systems supporting the original MBA
+ * implementation the delay value allocated to a core is always the maximum
+ * of the delay values assigned to the hardware threads sharing the core.
+ *
+ * Some systems support a model-specific MSR with which this default
+ * behavior can be changed. On these systems the core can be allocated
+ * with either the minimum or maximum delay value assigned to its hardware
+ * threads.
+ *
+ * NOTE: The hardware deals with memory delay values that may be programmed
+ * from zero (implying zero delay, and full bandwidth available) to the
+ * maximum specified in CPUID. The software interface deals with memory
+ * bandwidth percentages that are the inverse of the delay values (100%
+ * memory bandwidth from user perspective is zero MBA delay from hardware
+ * perspective). When maximum throttling is active the core is allocated
+ * with the maximum delay value that from the software interface will be
+ * the minimum of the bandwidth percentages assigned to the hardware threads
+ * sharing the core.
+ */
+static int rdt_thread_throttle_mode_show(struct kernfs_open_file *of,
+					 struct seq_file *seq, void *v)
+{
+	unsigned int throttle_mode = 0;
+
+	if (mba_cfg_supports_min_max_intel())
+		throttle_mode = mba_cfg_msr & MBA_THROTTLE_MODE_MASK;
+
+	seq_puts(seq,
+		 throttle_mode == MBA_THROTTLE_MODE_MIN ? "min\n" : "max\n");
+
+	return 0;
+}
+
+static void update_mba_cfg(void *data)
+{
+	u64 *mba_cfg = data;
+
+	wrmsrl(MSR_MBA_CFG, *mba_cfg);
+}
+
+/*
+ * The model-specific MBA configuration MSR has package scope. Making a
+ * system-wide MBA configuration change thus needs to modify the MSR on one
+ * CPU from each package.
+ */
+static int rdt_system_mba_cfg_set(u64 mba_cfg)
+{
+	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA];
+	cpumask_var_t cpu_mask;
+	struct rdt_domain *d;
+
+	if (list_is_singular(&r->domains)) {
+		wrmsrl(MSR_MBA_CFG, mba_cfg);
+		goto out;
+	}
+
+	if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL)) {
+		rdt_last_cmd_puts("Memory allocation error\n");
+		return -ENOMEM;
+	}
+
+	list_for_each_entry(d, &r->domains, list)
+		cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
+
+	on_each_cpu_mask(cpu_mask, update_mba_cfg, &mba_cfg, 1);
+
+	free_cpumask_var(cpu_mask);
+out:
+	mba_cfg_msr = mba_cfg;
+	return 0;
+}
+
+/*
+ * See NOTE associated with rdt_thread_throttle_mode_show() for
+ * details of the min/max interpretation.
+ */
+static ssize_t rdt_thread_throttle_mode_write(struct kernfs_open_file *of,
+					      char *buf, size_t nbytes,
+					      loff_t off)
+{
+	u64 mba_cfg;
+	int ret = 0;
+
+	if (nbytes == 0)
+		return -EINVAL;
+
+	cpus_read_lock();
+	mutex_lock(&rdtgroup_mutex);
+
+	rdt_last_cmd_clear();
+
+	/*
+	 * Additional check.
+	 * This function should not be associated with the user space file
+	 * on systems that do not support configuration.
+	 */
+	if (!mba_cfg_supports_min_max_intel()) {
+		rdt_last_cmd_puts("Platform does not support mode changes\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	mba_cfg = mba_cfg_msr & MBA_THROTTLE_MODE_MASK;
+
+	if ((sysfs_streq(buf, "min") && mba_cfg == MBA_THROTTLE_MODE_MIN) ||
+	    (sysfs_streq(buf, "max") && mba_cfg == MBA_THROTTLE_MODE_MAX))
+		goto out;
+
+	if (sysfs_streq(buf, "min")) {
+		mba_cfg = MBA_THROTTLE_MODE_MIN;
+	} else if (sysfs_streq(buf, "max")) {
+		mba_cfg = MBA_THROTTLE_MODE_MAX;
+	} else {
+		rdt_last_cmd_puts("Unknown or unsupported mode\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	mba_cfg = (mba_cfg_msr & ~MBA_THROTTLE_MODE_MASK) | mba_cfg;
+	ret = rdt_system_mba_cfg_set(mba_cfg);
+
+out:
+	mutex_unlock(&rdtgroup_mutex);
+	cpus_read_unlock();
+	return ret ?: nbytes;
+}
+
 static ssize_t max_threshold_occ_write(struct kernfs_open_file *of,
 				       char *buf, size_t nbytes, loff_t off)
 {
@@ -1512,6 +1642,16 @@ static struct rftype res_common_files[] = {
 		.seq_show	= rdt_delay_linear_show,
 		.fflags		= RF_CTRL_INFO | RFTYPE_RES_MB,
 	},
+	/*
+	 * Platform specific which (if any) capabilities are provided by
+	 * thread_throttle_mode. Defer some initialization to platform
+	 * discovery.
+	 */
+	{
+		.name		= "thread_throttle_mode",
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdt_thread_throttle_mode_show,
+	},
 	{
 		.name		= "max_threshold_occupancy",
 		.mode		= 0644,
@@ -1571,6 +1711,47 @@ static struct rftype res_common_files[] = {
 
 };
 
+static struct rftype *rdtgroup_rftype_by_name(const char *name)
+{
+	struct rftype *rfts, *rft;
+	int len;
+
+	rfts = res_common_files;
+	len = ARRAY_SIZE(res_common_files);
+
+	for (rft = rfts; rft < rfts + len; rft++) {
+		if (!strcmp(rft->name, name))
+			return rft;
+	}
+
+	return NULL;
+}
+
+void __init thread_throttle_mode_init_intel_rw(void)
+{
+	struct rftype *rft;
+
+	rft = rdtgroup_rftype_by_name("thread_throttle_mode");
+	if (!rft)
+		return;
+
+	rft->mode = 0644;
+	rft->write = rdt_thread_throttle_mode_write;
+	rft->fflags = RF_CTRL_INFO | RFTYPE_RES_MB;
+}
+
+void __init thread_throttle_mode_init_intel_ro(void)
+{
+	struct rftype *rft;
+
+	rft = rdtgroup_rftype_by_name("thread_throttle_mode");
+	if (!rft)
+		return;
+
+	rft->mode = 0444;
+	rft->fflags = RF_CTRL_INFO | RFTYPE_RES_MB;
+}
+
 static int rdtgroup_add_files(struct kernfs_node *kn, unsigned long fflags)
 {
 	struct rftype *rfts, *rft;
@@ -1582,7 +1763,7 @@ static int rdtgroup_add_files(struct kernfs_node *kn, unsigned long fflags)
 	lockdep_assert_held(&rdtgroup_mutex);
 
 	for (rft = rfts; rft < rfts + len; rft++) {
-		if ((fflags & rft->fflags) == rft->fflags) {
+		if (rft->fflags && ((fflags & rft->fflags) == rft->fflags)) {
 			ret = rdtgroup_add_file(kn, rft);
 			if (ret)
 				goto error;
@@ -2239,6 +2420,11 @@ static int reset_all_ctrls(struct rdt_resource *r)
 	smp_call_function_many(cpu_mask, rdt_ctrl_update, &msr_param, 1);
 	put_cpu();
 
+	if (mba_cfg_supports_min_max_intel()) {
+		mba_cfg_msr = 0;
+		on_each_cpu_mask(cpu_mask, update_mba_cfg, &mba_cfg_msr, 1);
+	}
+
 	free_cpumask_var(cpu_mask);
 
 	return 0;
-- 
2.21.0

[PATCH V3 2/4] x86/resctrl: Enumerate per-thread MBA

[Reinette Chatre]

From: Fenghua Yu <fenghua.yu@intel.com>

Some systems support per-thread Memory Bandwidth Allocation (MBA) which
applies a throttling delay value to each hardware thread instead of to
a core. Per-thread MBA is enumerated by CPUID.

No feature flag is shown in /proc/cpuinfo. User applications need to
check a resctrl throttling mode info file to know if the feature is
supported.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/cpuid-deps.c   | 1 +
 arch/x86/kernel/cpu/scattered.c    | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index db189945e9b0..d0ec26ce7f66 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -286,6 +286,7 @@
 #define X86_FEATURE_FENCE_SWAPGS_USER	(11*32+ 4) /* "" LFENCE in user entry SWAPGS path */
 #define X86_FEATURE_FENCE_SWAPGS_KERNEL	(11*32+ 5) /* "" LFENCE in kernel entry SWAPGS path */
 #define X86_FEATURE_SPLIT_LOCK_DETECT	(11*32+ 6) /* #AC for split lock */
+#define X86_FEATURE_PER_THREAD_MBA	(11*32+ 7) /* "" Per-thread Memory Bandwidth Allocation */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */
 #define X86_FEATURE_AVX512_BF16		(12*32+ 5) /* AVX512 BFLOAT16 instructions */
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 3cbe24ca80ab..3e30b26c50ef 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -69,6 +69,7 @@ static const struct cpuid_dep cpuid_deps[] = {
 	{ X86_FEATURE_CQM_MBM_TOTAL,		X86_FEATURE_CQM_LLC   },
 	{ X86_FEATURE_CQM_MBM_LOCAL,		X86_FEATURE_CQM_LLC   },
 	{ X86_FEATURE_AVX512_BF16,		X86_FEATURE_AVX512VL  },
+	{ X86_FEATURE_PER_THREAD_MBA,		X86_FEATURE_MBA       },
 	{}
 };
 
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 62b137c3c97a..bccfc9ff3cc1 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -35,6 +35,7 @@ static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_CDP_L3,		CPUID_ECX,  2, 0x00000010, 1 },
 	{ X86_FEATURE_CDP_L2,		CPUID_ECX,  2, 0x00000010, 2 },
 	{ X86_FEATURE_MBA,		CPUID_EBX,  3, 0x00000010, 0 },
+	{ X86_FEATURE_PER_THREAD_MBA,	CPUID_ECX,  0, 0x00000010, 3 },
 	{ X86_FEATURE_HW_PSTATE,	CPUID_EDX,  7, 0x80000007, 0 },
 	{ X86_FEATURE_CPB,		CPUID_EDX,  9, 0x80000007, 0 },
 	{ X86_FEATURE_PROC_FEEDBACK,    CPUID_EDX, 11, 0x80000007, 0 },
-- 
2.21.0

[PATCH V3 3/4] x86/resctrl: Enable per-thread MBA

[Reinette Chatre]

From: Fenghua Yu <fenghua.yu@intel.com>

Current Memory Bandwidth Allocation (MBA) hardware has a limitation:
all threads on the same core must have the same delay value. If there
are different delay values across threads on one core, the original
MBA implementation allocates the max delay value to the core and an
updated implementation allocates either min or max delay value specified
by a configuration MSR across threads on the core.

Newer systems support per-thread MBA such that each thread is allocated
with its own delay value.

If per-thread MBA is supported, report "per-thread" in resctrl file
"info/MB/thread_throttle_mode" to let user applications know memory
bandwidth is allocated per thread and help them fine tune MBA on thread
level.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
Changes since V2:
- Fix rST formatting of documentation (resctrl_ui.rst) describing
  new "thread_throttle_mode" resctrl file.
- Use boot_cpu_has() instead of static_cpu_has() when determining what
  to display to user (slow path).

 Documentation/x86/resctrl_ui.rst       |  3 +++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/Documentation/x86/resctrl_ui.rst b/Documentation/x86/resctrl_ui.rst
index 861ee2816470..1b066d1aafad 100644
--- a/Documentation/x86/resctrl_ui.rst
+++ b/Documentation/x86/resctrl_ui.rst
@@ -150,6 +150,9 @@ with respect to allocation:
 		"max":
 			the smallest percentage is applied
 			to all threads
+		"per-thread":
+			bandwidth percentages are directly applied to
+			the threads running on the core
 
 If RDT monitoring is available there will be an "L3_MON" directory
 with the following files:
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 6a9408060ac4..c60a3b307f7d 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1038,12 +1038,23 @@ static int max_threshold_occ_show(struct kernfs_open_file *of,
  * with the maximum delay value that from the software interface will be
  * the minimum of the bandwidth percentages assigned to the hardware threads
  * sharing the core.
+ *
+ * Some systems (identified by X86_FEATURE_PER_THREAD_MBA enumerated via CPUID)
+ * support per-thread MBA. On these systems hardware doesn't apply the minimum
+ * or maximum delay value to all threads in a core. Instead, a thread is
+ * allocated with the delay value that is assigned to the thread.
  */
 static int rdt_thread_throttle_mode_show(struct kernfs_open_file *of,
 					 struct seq_file *seq, void *v)
 {
 	unsigned int throttle_mode = 0;
 
+	if (boot_cpu_has(X86_FEATURE_PER_THREAD_MBA)) {
+		seq_puts(seq, "per-thread\n");
+
+		return 0;
+	}
+
 	if (mba_cfg_supports_min_max_intel())
 		throttle_mode = mba_cfg_msr & MBA_THROTTLE_MODE_MASK;
 
-- 
2.21.0

[PATCH V3 4/4] x86/resctrl: Use appropriate API for strings terminated by newline

[Reinette Chatre]

The user input to files in the resctrl filesystem are expected to be
terminated with a newline. Testing the user input includes a test for
the presence of a newline and then replacing the newline with NUL
byte followed by comparison using strcmp().

sysfs_streq() exists to test if strings are equal, treating both NUL and
newline-then-NUL as equivalent string terminations. Even more,
sysfs_match_string() exists to match a given string in an array using
sysfs_streq().

Replace existing strcmp() comparisons of strings that are terminated
with a newline with more appropriate sysfs_streq() via the
sysfs_match_string() API that can perform the match across the different
mode strings that are already maintained in an array.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 32 ++++++++++++++------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index c60a3b307f7d..e70694892c02 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1408,12 +1408,11 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
 {
 	struct rdtgroup *rdtgrp;
 	enum rdtgrp_mode mode;
-	int ret = 0;
+	int user_m;
+	int ret;
 
-	/* Valid input requires a trailing newline */
-	if (nbytes == 0 || buf[nbytes - 1] != '\n')
+	if (nbytes == 0)
 		return -EINVAL;
-	buf[nbytes - 1] = '\0';
 
 	rdtgrp = rdtgroup_kn_lock_live(of->kn);
 	if (!rdtgrp) {
@@ -1425,11 +1424,17 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
 
 	mode = rdtgrp->mode;
 
-	if ((!strcmp(buf, "shareable") && mode == RDT_MODE_SHAREABLE) ||
-	    (!strcmp(buf, "exclusive") && mode == RDT_MODE_EXCLUSIVE) ||
-	    (!strcmp(buf, "pseudo-locksetup") &&
-	     mode == RDT_MODE_PSEUDO_LOCKSETUP) ||
-	    (!strcmp(buf, "pseudo-locked") && mode == RDT_MODE_PSEUDO_LOCKED))
+	ret = sysfs_match_string(rdt_mode_str, buf);
+	if (ret < 0) {
+		rdt_last_cmd_puts("Unknown or unsupported mode\n");
+		goto out;
+	}
+
+	user_m = ret;
+	ret = 0;
+
+	/* Do nothing and return success if user asks for current mode */
+	if (user_m == mode)
 		goto out;
 
 	if (mode == RDT_MODE_PSEUDO_LOCKED) {
@@ -1438,14 +1443,14 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
 		goto out;
 	}
 
-	if (!strcmp(buf, "shareable")) {
+	if (user_m == RDT_MODE_SHAREABLE) {
 		if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
 			ret = rdtgroup_locksetup_exit(rdtgrp);
 			if (ret)
 				goto out;
 		}
 		rdtgrp->mode = RDT_MODE_SHAREABLE;
-	} else if (!strcmp(buf, "exclusive")) {
+	} else if (user_m == RDT_MODE_EXCLUSIVE) {
 		if (!rdtgroup_mode_test_exclusive(rdtgrp)) {
 			ret = -EINVAL;
 			goto out;
@@ -1456,14 +1461,11 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
 				goto out;
 		}
 		rdtgrp->mode = RDT_MODE_EXCLUSIVE;
-	} else if (!strcmp(buf, "pseudo-locksetup")) {
+	} else if (user_m == RDT_MODE_PSEUDO_LOCKSETUP) {
 		ret = rdtgroup_locksetup_enter(rdtgrp);
 		if (ret)
 			goto out;
 		rdtgrp->mode = RDT_MODE_PSEUDO_LOCKSETUP;
-	} else {
-		rdt_last_cmd_puts("Unknown or unsupported mode\n");
-		ret = -EINVAL;
 	}
 
 out:
-- 
2.21.0

RE: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread throttling mode

[Babu Moger]

Hi Reinnette,

The patches did not apply on my tree. I got the latest tree today. You
might want to check again.
Hunk #1 FAILED at 29.
1 out of 7 hunks FAILED -- saving rejects to file
arch/x86/kernel/cpu/resctrl/rdtgroup.c.rej


> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Wednesday, May 6, 2020 6:50 PM
> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
> tony.luck@intel.com
> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>
> Subject: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread
> throttling mode
> 
> Intel Memory Bandwidth Allocation (MBA) control is provided per
> processor core. At the same time different CLOS, configured with different
> bandwidth percentages, can be assigned to the hardware threads
> sharing a core. In the original implementation of MBA the maximum throttling
> of the per-thread CLOS is allocated to the core. Specifically, the lower
> bandwidth percentage is allocated to the core.
> 
> Newer systems can be configured to allocate either maximum or
> minimum throttling of the per-thread CLOS values to the core.
> 
> Introduce a new resctrl file, "thread_throttle_mode", on Intel systems
> that exposes to the user how per-thread values are allocated to
> a core. On systems that support the original MBA implementation the
> file will always display "max". On systems that can be configured
> the possible values are "min" or "max" that the user can modify by
> writing these same words to the file.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Changes since V2:
> - Rebase on top of recently merged series "x86/resctrl: Support wider
>   MBM counters". Small change needed to take into account
>   asm/resctrl_sched.h -> asm/resctrl.h name change.
> - Fix rST formatting of documentation (resctrl_ui.rst) describing
>   new "thread_throttle_mode" resctrl file.
> 
>  Documentation/x86/resctrl_ui.rst       |  19 ++-
>  arch/x86/kernel/cpu/resctrl/core.c     |  32 +++++
>  arch/x86/kernel/cpu/resctrl/internal.h |  13 ++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 188 ++++++++++++++++++++++++-
>  4 files changed, 249 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/x86/resctrl_ui.rst
> b/Documentation/x86/resctrl_ui.rst
> index 5368cedfb530..861ee2816470 100644
> --- a/Documentation/x86/resctrl_ui.rst
> +++ b/Documentation/x86/resctrl_ui.rst
> @@ -138,6 +138,19 @@ with respect to allocation:
>  		non-linear. This field is purely informational
>  		only.
> 
> +"thread_throttle_mode":
> +		Indicator (on some CPU models control) on Intel systems
> +		of how tasks running on threads of a physical core are
> +		throttled in cases where they request different memory
> +		bandwidth percentages:
> +
> +		"min":
> +			the largest percentage is applied
> +			to all threads
> +		"max":
> +			the smallest percentage is applied
> +			to all threads
> +
>  If RDT monitoring is available there will be an "L3_MON" directory
>  with the following files:
> 
> @@ -364,8 +377,10 @@ to the next control step available on the hardware.
> 
>  The bandwidth throttling is a core specific mechanism on some of Intel
>  SKUs. Using a high bandwidth and a low bandwidth setting on two threads
> -sharing a core will result in both threads being throttled to use the
> -low bandwidth. The fact that Memory bandwidth allocation(MBA) is a core
> +sharing a core may result in both threads being throttled to use the
> +low bandwidth (see "thread_throttle_mode").
> +
> +The fact that Memory bandwidth allocation(MBA) may be a core
>  specific mechanism where as memory bandwidth monitoring(MBM) is done at
>  the package level may lead to confusion when users try to apply control
>  via the MBA and then monitor the bandwidth to see if the controls are
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
> b/arch/x86/kernel/cpu/resctrl/core.c
> index 12f967c6b603..1bc686777069 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -250,6 +250,30 @@ static inline bool rdt_get_mb_table(struct rdt_resource
> *r)
>  	return false;
>  }
> 
> +/*
> + * Model-specific test to determine if platform where memory bandwidth
> + * control is applied to a core can be configured to apply either the
> + * maximum or minimum of the per-thread delay values.
> + * By default, platforms where memory bandwidth control is applied to a
> + * core will select the maximum delay value of the per-thread CLOS.
> + *
> + * NOTE: delay value programmed to hardware is inverse of bandwidth
> + * percentage configured via user interface.
> + */
> +bool mba_cfg_supports_min_max_intel(void)
> +{
> +	switch (boot_cpu_data.x86_model) {
> +	case INTEL_FAM6_ATOM_TREMONT_D:
> +	case INTEL_FAM6_ICELAKE_X:
> +	case INTEL_FAM6_ICELAKE_D:
> +		return true;
> +	default:
> +		return false;
> +	}
> +
> +	return false;
> +}

I see that you are calling this function multiple times. Why don't you
make it as a property in rdt_resource. Set it only once during the
init(may be in get_mem_config_intel). Then you can use it wherever
required. This also probably help James to make everything architecture
independent. What do you think?

I assume that this property is probably not part of CPUID.

> +
>  static bool __get_mem_config_intel(struct rdt_resource *r)
>  {
>  	union cpuid_0x10_3_eax eax;
> @@ -270,6 +294,11 @@ static bool __get_mem_config_intel(struct
> rdt_resource *r)
>  	}
>  	r->data_width = 3;
> 
> +	if (mba_cfg_supports_min_max_intel())
> +		thread_throttle_mode_init_intel_rw();
> +	else
> +		thread_throttle_mode_init_intel_ro();
> +
>  	r->alloc_capable = true;
>  	r->alloc_enabled = true;
> 
> @@ -580,6 +609,9 @@ static void domain_add_cpu(int cpu, struct rdt_resource
> *r)
> 
>  	rdt_domain_reconfigure_cdp(r);
> 
> +	if (mba_cfg_supports_min_max_intel())
> +		wrmsrl(MSR_MBA_CFG, mba_cfg_msr);
> +
>  	if (r->alloc_capable && domain_setup_ctrlval(r, d)) {
>  		kfree(d);
>  		return;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> b/arch/x86/kernel/cpu/resctrl/internal.h
> index f20a47d120b1..c5f4cb91009b 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -9,6 +9,7 @@
> 
>  #define MSR_IA32_L3_QOS_CFG		0xc81
>  #define MSR_IA32_L2_QOS_CFG		0xc82
> +#define MSR_MBA_CFG			0xc84
>  #define MSR_IA32_L3_CBM_BASE		0xc90
>  #define MSR_IA32_L2_CBM_BASE		0xd10
>  #define MSR_IA32_MBA_THRTL_BASE		0xd50
> @@ -21,6 +22,9 @@
> 
>  #define L2_QOS_CDP_ENABLE		0x01ULL
> 
> +#define MBA_THROTTLE_MODE_MIN		0x01ULL
> +#define MBA_THROTTLE_MODE_MAX		0x00ULL
> +
>  /*
>   * Event IDs are used to program IA32_QM_EVTSEL before reading event
>   * counter from IA32_QM_CTR
> @@ -38,6 +42,8 @@
>  #define MBA_MAX_MBPS			U32_MAX
>  #define MAX_MBA_BW_AMD			0x800
> 
> +#define MBA_THROTTLE_MODE_MASK		BIT_ULL(0)
> +
>  #define RMID_VAL_ERROR			BIT_ULL(63)
>  #define RMID_VAL_UNAVAIL		BIT_ULL(62)
>  /*
> @@ -47,6 +53,10 @@
>   */
>  #define MBM_CNTR_WIDTH_OFFSET_MAX (62 - MBM_CNTR_WIDTH_BASE)
> 
> +/*
> + * MSR_MBA_CFG cache
> + */
> +extern u64 mba_cfg_msr;
> 
>  struct rdt_fs_context {
>  	struct kernfs_fs_context	kfc;
> @@ -611,5 +621,8 @@ void __check_limbo(struct rdt_domain *d, bool
> force_free);
>  bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
>  bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r);
>  void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
> +bool mba_cfg_supports_min_max_intel(void);
> +void thread_throttle_mode_init_intel_rw(void);
> +void thread_throttle_mode_init_intel_ro(void);
> 
>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index d7cb5ab0d1f0..6a9408060ac4 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -29,6 +29,7 @@
> 
>  #include <uapi/linux/magic.h>
> 
> +#include <asm/intel-family.h>
>  #include <asm/resctrl.h>
>  #include "internal.h"
> 
> @@ -38,6 +39,7 @@ DEFINE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
>  static struct kernfs_root *rdt_root;
>  struct rdtgroup rdtgroup_default;
>  LIST_HEAD(rdt_all_groups);
> +u64 mba_cfg_msr;
> 
>  /* Kernel fs node for "info" directory under root */
>  static struct kernfs_node *kn_info;
> @@ -1017,6 +1019,134 @@ static int max_threshold_occ_show(struct
> kernfs_open_file *of,
>  	return 0;
>  }
> 
> +/*
> + * As documented in the Intel SDM, on systems supporting the original MBA
> + * implementation the delay value allocated to a core is always the maximum
> + * of the delay values assigned to the hardware threads sharing the core.
> + *
> + * Some systems support a model-specific MSR with which this default
> + * behavior can be changed. On these systems the core can be allocated
> + * with either the minimum or maximum delay value assigned to its hardware
> + * threads.
> + *
> + * NOTE: The hardware deals with memory delay values that may be
> programmed
> + * from zero (implying zero delay, and full bandwidth available) to the
> + * maximum specified in CPUID. The software interface deals with memory
> + * bandwidth percentages that are the inverse of the delay values (100%
> + * memory bandwidth from user perspective is zero MBA delay from hardware
> + * perspective). When maximum throttling is active the core is allocated
> + * with the maximum delay value that from the software interface will be
> + * the minimum of the bandwidth percentages assigned to the hardware
> threads
> + * sharing the core.
> + */
> +static int rdt_thread_throttle_mode_show(struct kernfs_open_file *of,
> +					 struct seq_file *seq, void *v)
> +{
> +	unsigned int throttle_mode = 0;
> +
> +	if (mba_cfg_supports_min_max_intel())
> +		throttle_mode = mba_cfg_msr &
> MBA_THROTTLE_MODE_MASK;
> +
> +	seq_puts(seq,
> +		 throttle_mode == MBA_THROTTLE_MODE_MIN ? "min\n" :
> "max\n");
> +
> +	return 0;
> +}
> +
> +static void update_mba_cfg(void *data)
> +{
> +	u64 *mba_cfg = data;
> +
> +	wrmsrl(MSR_MBA_CFG, *mba_cfg);
> +}
> +
> +/*
> + * The model-specific MBA configuration MSR has package scope. Making a
> + * system-wide MBA configuration change thus needs to modify the MSR on
> one
> + * CPU from each package.
> + */
> +static int rdt_system_mba_cfg_set(u64 mba_cfg)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA];
> +	cpumask_var_t cpu_mask;
> +	struct rdt_domain *d;
> +
> +	if (list_is_singular(&r->domains)) {
> +		wrmsrl(MSR_MBA_CFG, mba_cfg);
> +		goto out;
> +	}
> +
> +	if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL)) {
> +		rdt_last_cmd_puts("Memory allocation error\n");
> +		return -ENOMEM;
> +	}
> +
> +	list_for_each_entry(d, &r->domains, list)
> +		cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
> +
> +	on_each_cpu_mask(cpu_mask, update_mba_cfg, &mba_cfg, 1);
> +
> +	free_cpumask_var(cpu_mask);
> +out:
> +	mba_cfg_msr = mba_cfg;
> +	return 0;
> +}
> +
> +/*
> + * See NOTE associated with rdt_thread_throttle_mode_show() for
> + * details of the min/max interpretation.
> + */
> +static ssize_t rdt_thread_throttle_mode_write(struct kernfs_open_file *of,
> +					      char *buf, size_t nbytes,
> +					      loff_t off)
> +{
> +	u64 mba_cfg;
> +	int ret = 0;
> +
> +	if (nbytes == 0)
> +		return -EINVAL;
> +
> +	cpus_read_lock();
> +	mutex_lock(&rdtgroup_mutex);
> +
> +	rdt_last_cmd_clear();
> +
> +	/*
> +	 * Additional check.
> +	 * This function should not be associated with the user space file
> +	 * on systems that do not support configuration.
> +	 */
> +	if (!mba_cfg_supports_min_max_intel()) {
> +		rdt_last_cmd_puts("Platform does not support mode
> changes\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	mba_cfg = mba_cfg_msr & MBA_THROTTLE_MODE_MASK;
> +
> +	if ((sysfs_streq(buf, "min") && mba_cfg ==
> MBA_THROTTLE_MODE_MIN) ||
> +	    (sysfs_streq(buf, "max") && mba_cfg ==
> MBA_THROTTLE_MODE_MAX))
> +		goto out;
> +
> +	if (sysfs_streq(buf, "min")) {
> +		mba_cfg = MBA_THROTTLE_MODE_MIN;
> +	} else if (sysfs_streq(buf, "max")) {
> +		mba_cfg = MBA_THROTTLE_MODE_MAX;
> +	} else {
> +		rdt_last_cmd_puts("Unknown or unsupported mode\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	mba_cfg = (mba_cfg_msr & ~MBA_THROTTLE_MODE_MASK) |
> mba_cfg;
> +	ret = rdt_system_mba_cfg_set(mba_cfg);
> +
> +out:
> +	mutex_unlock(&rdtgroup_mutex);
> +	cpus_read_unlock();
> +	return ret ?: nbytes;
> +}
> +
>  static ssize_t max_threshold_occ_write(struct kernfs_open_file *of,
>  				       char *buf, size_t nbytes, loff_t off)
>  {
> @@ -1512,6 +1642,16 @@ static struct rftype res_common_files[] = {
>  		.seq_show	= rdt_delay_linear_show,
>  		.fflags		= RF_CTRL_INFO | RFTYPE_RES_MB,
>  	},
> +	/*
> +	 * Platform specific which (if any) capabilities are provided by
> +	 * thread_throttle_mode. Defer some initialization to platform
> +	 * discovery.
> +	 */
> +	{
> +		.name		= "thread_throttle_mode",
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= rdt_thread_throttle_mode_show,
> +	},
>  	{
>  		.name		= "max_threshold_occupancy",
>  		.mode		= 0644,
> @@ -1571,6 +1711,47 @@ static struct rftype res_common_files[] = {
> 
>  };
> 
> +static struct rftype *rdtgroup_rftype_by_name(const char *name)
> +{
> +	struct rftype *rfts, *rft;
> +	int len;
> +
> +	rfts = res_common_files;
> +	len = ARRAY_SIZE(res_common_files);
> +
> +	for (rft = rfts; rft < rfts + len; rft++) {
> +		if (!strcmp(rft->name, name))
> +			return rft;
> +	}
> +
> +	return NULL;
> +}
> +
> +void __init thread_throttle_mode_init_intel_rw(void)
> +{
> +	struct rftype *rft;
> +
> +	rft = rdtgroup_rftype_by_name("thread_throttle_mode");
> +	if (!rft)
> +		return;
> +
> +	rft->mode = 0644;
> +	rft->write = rdt_thread_throttle_mode_write;
> +	rft->fflags = RF_CTRL_INFO | RFTYPE_RES_MB;
> +}
> +
> +void __init thread_throttle_mode_init_intel_ro(void)
> +{
> +	struct rftype *rft;
> +
> +	rft = rdtgroup_rftype_by_name("thread_throttle_mode");
> +	if (!rft)
> +		return;
> +
> +	rft->mode = 0444;
> +	rft->fflags = RF_CTRL_INFO | RFTYPE_RES_MB;
> +}
> +
>  static int rdtgroup_add_files(struct kernfs_node *kn, unsigned long fflags)
>  {
>  	struct rftype *rfts, *rft;
> @@ -1582,7 +1763,7 @@ static int rdtgroup_add_files(struct kernfs_node *kn,
> unsigned long fflags)
>  	lockdep_assert_held(&rdtgroup_mutex);
> 
>  	for (rft = rfts; rft < rfts + len; rft++) {
> -		if ((fflags & rft->fflags) == rft->fflags) {
> +		if (rft->fflags && ((fflags & rft->fflags) == rft->fflags)) {
>  			ret = rdtgroup_add_file(kn, rft);
>  			if (ret)
>  				goto error;
> @@ -2239,6 +2420,11 @@ static int reset_all_ctrls(struct rdt_resource *r)
>  	smp_call_function_many(cpu_mask, rdt_ctrl_update, &msr_param, 1);
>  	put_cpu();
> 
> +	if (mba_cfg_supports_min_max_intel()) {
> +		mba_cfg_msr = 0;
> +		on_each_cpu_mask(cpu_mask, update_mba_cfg,
> &mba_cfg_msr, 1);
> +	}
> +
>  	free_cpumask_var(cpu_mask);
> 
>  	return 0;
> --
> 2.21.0

Re: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread throttling mode

[Reinette Chatre]

Hi Babu,

Thank you very much for taking a look.

On 5/14/2020 9:45 AM, Babu Moger wrote:
> Hi Reinnette,
> 
> The patches did not apply on my tree. I got the latest tree today. You

Which tree did you use as baseline? These patches should apply cleanly
on top of the other resctrl patches already queued for inclusion into
v5.8 as found on the x86/cache branch of the tip repo.

> might want to check again.
> Hunk #1 FAILED at 29.
> 1 out of 7 hunks FAILED -- saving rejects to file
> arch/x86/kernel/cpu/resctrl/rdtgroup.c.rej
> 
> 
>> -----Original Message-----
>> From: Reinette Chatre <reinette.chatre@intel.com>
>> Sent: Wednesday, May 6, 2020 6:50 PM
>> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
>> tony.luck@intel.com
>> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
>> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
>> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>
>> Subject: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread
>> throttling mode
>>

...

>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>> b/arch/x86/kernel/cpu/resctrl/core.c
>> index 12f967c6b603..1bc686777069 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -250,6 +250,30 @@ static inline bool rdt_get_mb_table(struct rdt_resource
>> *r)
>>  	return false;
>>  }
>>
>> +/*
>> + * Model-specific test to determine if platform where memory bandwidth
>> + * control is applied to a core can be configured to apply either the
>> + * maximum or minimum of the per-thread delay values.
>> + * By default, platforms where memory bandwidth control is applied to a
>> + * core will select the maximum delay value of the per-thread CLOS.
>> + *
>> + * NOTE: delay value programmed to hardware is inverse of bandwidth
>> + * percentage configured via user interface.
>> + */
>> +bool mba_cfg_supports_min_max_intel(void)
>> +{
>> +	switch (boot_cpu_data.x86_model) {
>> +	case INTEL_FAM6_ATOM_TREMONT_D:
>> +	case INTEL_FAM6_ICELAKE_X:
>> +	case INTEL_FAM6_ICELAKE_D:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +
>> +	return false;
>> +}
> 
> I see that you are calling this function multiple times. Why don't you

It is called:
- once during initialization
- once per package initialization (when first CPU on a package comes online)
- once per read from or write to the new "thread_throttle_mode" file

> make it as a property in rdt_resource. Set it only once during the
> init(may be in get_mem_config_intel). Then you can use it wherever
> required. This also probably help James to make everything architecture
> independent. What do you think?

If I understand correctly this would require understanding how each
architecture behaves in this regard to ensure the property within
rdt_resource is initialized correctly. While it could just be set within
get_mem_config_intel as you suggest, this property would be present in
AMD's resource and thus need a value ... could you please provide
guidance what this property should be on AMD? I looked at the bandwidth
enforcement section of
https://developer.amd.com/wp-content/resources/56375.pdf but it is not
obvious to me where bandwidth is actually enforced and how this
enforcement is affected when threads and/or cores are running tasks with
different CLOS that have different bandwidth limits assigned.

With AMD's properties understood then the new "thread_throttle_mode"
file could be visible on all platforms, not just Intel, and display
accurate values for all and be architecture independent.

Alternatively, the new property could have values: UNDEFINED, MIN, MAX,
or PER_THREAD ... with AMD having UNDEFINED and the
"thread_throttle_mode" continues to be visible on Intel platforms only.

I would appreciate your thoughts on this.

> I assume that this property is probably not part of CPUID.

Correct. This specific property is model specific, but also transient in
that it is replaced by X86_FEATURE_PER_THREAD_MBA (that is part of
CPUID) in future platforms.

Reinette

RE: [PATCH V3 2/4] x86/resctrl: Enumerate per-thread MBA

[Babu Moger]

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Wednesday, May 6, 2020 6:50 PM
> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
> tony.luck@intel.com
> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>
> Subject: [PATCH V3 2/4] x86/resctrl: Enumerate per-thread MBA
> 
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> Some systems support per-thread Memory Bandwidth Allocation (MBA) which
> applies a throttling delay value to each hardware thread instead of to
> a core. Per-thread MBA is enumerated by CPUID.
> 
> No feature flag is shown in /proc/cpuinfo. User applications need to
> check a resctrl throttling mode info file to know if the feature is
> supported.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  arch/x86/include/asm/cpufeatures.h | 1 +
>  arch/x86/kernel/cpu/cpuid-deps.c   | 1 +
>  arch/x86/kernel/cpu/scattered.c    | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h
> b/arch/x86/include/asm/cpufeatures.h
> index db189945e9b0..d0ec26ce7f66 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -286,6 +286,7 @@
>  #define X86_FEATURE_FENCE_SWAPGS_USER	(11*32+ 4) /* "" LFENCE in user
> entry SWAPGS path */
>  #define X86_FEATURE_FENCE_SWAPGS_KERNEL	(11*32+ 5) /* ""
> LFENCE in kernel entry SWAPGS path */
>  #define X86_FEATURE_SPLIT_LOCK_DETECT	(11*32+ 6) /* #AC for split lock
> */
> +#define X86_FEATURE_PER_THREAD_MBA	(11*32+ 7) /* "" Per-thread
> Memory Bandwidth Allocation */
> 
>  /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */
>  #define X86_FEATURE_AVX512_BF16		(12*32+ 5) /* AVX512
> BFLOAT16 instructions */
> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-
> deps.c
> index 3cbe24ca80ab..3e30b26c50ef 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -69,6 +69,7 @@ static const struct cpuid_dep cpuid_deps[] = {
>  	{ X86_FEATURE_CQM_MBM_TOTAL,
> 	X86_FEATURE_CQM_LLC   },
>  	{ X86_FEATURE_CQM_MBM_LOCAL,
> 	X86_FEATURE_CQM_LLC   },
>  	{ X86_FEATURE_AVX512_BF16,		X86_FEATURE_AVX512VL  },
> +	{ X86_FEATURE_PER_THREAD_MBA,		X86_FEATURE_MBA
> },
>  	{}
>  };
> 
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index 62b137c3c97a..bccfc9ff3cc1 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -35,6 +35,7 @@ static const struct cpuid_bit cpuid_bits[] = {
>  	{ X86_FEATURE_CDP_L3,		CPUID_ECX,  2, 0x00000010, 1
> },
>  	{ X86_FEATURE_CDP_L2,		CPUID_ECX,  2, 0x00000010, 2
> },
>  	{ X86_FEATURE_MBA,		CPUID_EBX,  3, 0x00000010, 0 },
> +	{ X86_FEATURE_PER_THREAD_MBA,	CPUID_ECX,  0, 0x00000010, 3
> },

This is a CPUID feature. You can actually detect this feature without
checking vendor model in patch @1. This patch looks good to me.

>  	{ X86_FEATURE_HW_PSTATE,	CPUID_EDX,  7, 0x80000007, 0 },
>  	{ X86_FEATURE_CPB,		CPUID_EDX,  9, 0x80000007, 0 },
>  	{ X86_FEATURE_PROC_FEEDBACK,    CPUID_EDX, 11, 0x80000007, 0 },
> --
> 2.21.0

RE: [PATCH V3 3/4] x86/resctrl: Enable per-thread MBA

[Babu Moger]

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Wednesday, May 6, 2020 6:50 PM
> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
> tony.luck@intel.com
> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>
> Subject: [PATCH V3 3/4] x86/resctrl: Enable per-thread MBA
> 
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> Current Memory Bandwidth Allocation (MBA) hardware has a limitation:
> all threads on the same core must have the same delay value. If there
> are different delay values across threads on one core, the original
> MBA implementation allocates the max delay value to the core and an
> updated implementation allocates either min or max delay value specified
> by a configuration MSR across threads on the core.
> 
> Newer systems support per-thread MBA such that each thread is allocated
> with its own delay value.
> 
> If per-thread MBA is supported, report "per-thread" in resctrl file
> "info/MB/thread_throttle_mode" to let user applications know memory
> bandwidth is allocated per thread and help them fine tune MBA on thread
> level.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Changes since V2:
> - Fix rST formatting of documentation (resctrl_ui.rst) describing
>   new "thread_throttle_mode" resctrl file.
> - Use boot_cpu_has() instead of static_cpu_has() when determining what
>   to display to user (slow path).
> 
>  Documentation/x86/resctrl_ui.rst       |  3 +++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 11 +++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/Documentation/x86/resctrl_ui.rst
> b/Documentation/x86/resctrl_ui.rst
> index 861ee2816470..1b066d1aafad 100644
> --- a/Documentation/x86/resctrl_ui.rst
> +++ b/Documentation/x86/resctrl_ui.rst
> @@ -150,6 +150,9 @@ with respect to allocation:
>  		"max":
>  			the smallest percentage is applied
>  			to all threads
> +		"per-thread":
> +			bandwidth percentages are directly applied to
> +			the threads running on the core
> 
>  If RDT monitoring is available there will be an "L3_MON" directory
>  with the following files:
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 6a9408060ac4..c60a3b307f7d 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1038,12 +1038,23 @@ static int max_threshold_occ_show(struct
> kernfs_open_file *of,
>   * with the maximum delay value that from the software interface will be
>   * the minimum of the bandwidth percentages assigned to the hardware threads
>   * sharing the core.
> + *
> + * Some systems (identified by X86_FEATURE_PER_THREAD_MBA enumerated
> via CPUID)
> + * support per-thread MBA. On these systems hardware doesn't apply the
> minimum
> + * or maximum delay value to all threads in a core. Instead, a thread is
> + * allocated with the delay value that is assigned to the thread.
>   */
>  static int rdt_thread_throttle_mode_show(struct kernfs_open_file *of,
>  					 struct seq_file *seq, void *v)
>  {
>  	unsigned int throttle_mode = 0;
> 
> +	if (boot_cpu_has(X86_FEATURE_PER_THREAD_MBA)) {
> +		seq_puts(seq, "per-thread\n");
> +
You probably don't need an extra line here.

> +		return 0;
> +	}
> +
>  	if (mba_cfg_supports_min_max_intel())
>  		throttle_mode = mba_cfg_msr &
> MBA_THROTTLE_MODE_MASK;
> 
> --
> 2.21.0

RE: [PATCH V3 4/4] x86/resctrl: Use appropriate API for strings terminated by newline

[Babu Moger]

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Wednesday, May 6, 2020 6:50 PM
> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
> tony.luck@intel.com
> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>;
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Subject: [PATCH V3 4/4] x86/resctrl: Use appropriate API for strings terminated
> by newline
> 
> The user input to files in the resctrl filesystem are expected to be
> terminated with a newline. Testing the user input includes a test for
> the presence of a newline and then replacing the newline with NUL
> byte followed by comparison using strcmp().
> 
> sysfs_streq() exists to test if strings are equal, treating both NUL and
> newline-then-NUL as equivalent string terminations. Even more,
> sysfs_match_string() exists to match a given string in an array using
> sysfs_streq().
> 
> Replace existing strcmp() comparisons of strings that are terminated
> with a newline with more appropriate sysfs_streq() via the
> sysfs_match_string() API that can perform the match across the different
> mode strings that are already maintained in an array.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 32 ++++++++++++++------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index c60a3b307f7d..e70694892c02 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1408,12 +1408,11 @@ static ssize_t rdtgroup_mode_write(struct
> kernfs_open_file *of,
>  {
>  	struct rdtgroup *rdtgrp;
>  	enum rdtgrp_mode mode;
> -	int ret = 0;
> +	int user_m;
> +	int ret;

This is bit confusing here. Mode and user_m should have been of same type.
You can define user_m to user_mode to be very clear.

You can completely remove "mode" and directly use rdtgrp->mode instead. It
is left to you.

> > -	/* Valid input requires a trailing newline */
> -	if (nbytes == 0 || buf[nbytes - 1] != '\n')
> +	if (nbytes == 0)
>  		return -EINVAL;
> -	buf[nbytes - 1] = '\0';
> 
>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>  	if (!rdtgrp) {
> @@ -1425,11 +1424,17 @@ static ssize_t rdtgroup_mode_write(struct
> kernfs_open_file *of,
> 
>  	mode = rdtgrp->mode;
> 
> -	if ((!strcmp(buf, "shareable") && mode == RDT_MODE_SHAREABLE) ||
> -	    (!strcmp(buf, "exclusive") && mode == RDT_MODE_EXCLUSIVE) ||
> -	    (!strcmp(buf, "pseudo-locksetup") &&
> -	     mode == RDT_MODE_PSEUDO_LOCKSETUP) ||
> -	    (!strcmp(buf, "pseudo-locked") && mode ==
> RDT_MODE_PSEUDO_LOCKED))
> +	ret = sysfs_match_string(rdt_mode_str, buf);
> +	if (ret < 0) {
> +		rdt_last_cmd_puts("Unknown or unsupported mode\n");
> +		goto out;
> +	}
> +
> +	user_m = ret;
> +	ret = 0;
> +
> +	/* Do nothing and return success if user asks for current mode */
> +	if (user_m == mode)
>  		goto out;
> 
>  	if (mode == RDT_MODE_PSEUDO_LOCKED) {
> @@ -1438,14 +1443,14 @@ static ssize_t rdtgroup_mode_write(struct
> kernfs_open_file *of,
>  		goto out;
>  	}
> 
> -	if (!strcmp(buf, "shareable")) {
> +	if (user_m == RDT_MODE_SHAREABLE) {
>  		if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>  			ret = rdtgroup_locksetup_exit(rdtgrp);
>  			if (ret)
>  				goto out;
>  		}
>  		rdtgrp->mode = RDT_MODE_SHAREABLE;
> -	} else if (!strcmp(buf, "exclusive")) {
> +	} else if (user_m == RDT_MODE_EXCLUSIVE) {
>  		if (!rdtgroup_mode_test_exclusive(rdtgrp)) {
>  			ret = -EINVAL;
>  			goto out;
> @@ -1456,14 +1461,11 @@ static ssize_t rdtgroup_mode_write(struct
> kernfs_open_file *of,
>  				goto out;
>  		}
>  		rdtgrp->mode = RDT_MODE_EXCLUSIVE;
> -	} else if (!strcmp(buf, "pseudo-locksetup")) {
> +	} else if (user_m == RDT_MODE_PSEUDO_LOCKSETUP) {
>  		ret = rdtgroup_locksetup_enter(rdtgrp);
>  		if (ret)
>  			goto out;
>  		rdtgrp->mode = RDT_MODE_PSEUDO_LOCKSETUP;
> -	} else {
> -		rdt_last_cmd_puts("Unknown or unsupported mode\n");
> -		ret = -EINVAL;
>  	}
> 
>  out:
> --
> 2.21.0

Re: [PATCH V3 4/4] x86/resctrl: Use appropriate API for strings terminated by newline

[Andy Shevchenko]

On Thu, May 14, 2020 at 10:08 PM Babu Moger <babu.moger@amd.com> wrote:
> > -----Original Message-----
> > From: Reinette Chatre <reinette.chatre@intel.com>
> > Sent: Wednesday, May 6, 2020 6:50 PM
> > To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
> > tony.luck@intel.com
> > Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
> > Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
> > linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>;
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Subject: [PATCH V3 4/4] x86/resctrl: Use appropriate API for strings terminated
> > by newline
> >
> > The user input to files in the resctrl filesystem are expected to be
> > terminated with a newline. Testing the user input includes a test for
> > the presence of a newline and then replacing the newline with NUL
> > byte followed by comparison using strcmp().
> >
> > sysfs_streq() exists to test if strings are equal, treating both NUL and
> > newline-then-NUL as equivalent string terminations. Even more,
> > sysfs_match_string() exists to match a given string in an array using
> > sysfs_streq().
> >
> > Replace existing strcmp() comparisons of strings that are terminated
> > with a newline with more appropriate sysfs_streq() via the
> > sysfs_match_string() API that can perform the match across the different
> > mode strings that are already maintained in an array.
> >
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 32 ++++++++++++++------------
> >  1 file changed, 17 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index c60a3b307f7d..e70694892c02 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -1408,12 +1408,11 @@ static ssize_t rdtgroup_mode_write(struct
> > kernfs_open_file *of,
> >  {
> >       struct rdtgroup *rdtgrp;
> >       enum rdtgrp_mode mode;
> > -     int ret = 0;
> > +     int user_m;
> > +     int ret;
>
> This is bit confusing here. Mode and user_m should have been of same type.

It can't. If you compare enum to int you will need the explicit (weird) casting.

> You can define user_m to user_mode to be very clear.

> You can completely remove "mode" and directly use rdtgrp->mode instead. It
> is left to you.
>
> > > -   /* Valid input requires a trailing newline */
> > -     if (nbytes == 0 || buf[nbytes - 1] != '\n')
> > +     if (nbytes == 0)
> >               return -EINVAL;
> > -     buf[nbytes - 1] = '\0';
> >
> >       rdtgrp = rdtgroup_kn_lock_live(of->kn);
> >       if (!rdtgrp) {
> > @@ -1425,11 +1424,17 @@ static ssize_t rdtgroup_mode_write(struct
> > kernfs_open_file *of,
> >
> >       mode = rdtgrp->mode;
> >
> > -     if ((!strcmp(buf, "shareable") && mode == RDT_MODE_SHAREABLE) ||
> > -         (!strcmp(buf, "exclusive") && mode == RDT_MODE_EXCLUSIVE) ||
> > -         (!strcmp(buf, "pseudo-locksetup") &&
> > -          mode == RDT_MODE_PSEUDO_LOCKSETUP) ||
> > -         (!strcmp(buf, "pseudo-locked") && mode ==
> > RDT_MODE_PSEUDO_LOCKED))
> > +     ret = sysfs_match_string(rdt_mode_str, buf);
> > +     if (ret < 0) {
> > +             rdt_last_cmd_puts("Unknown or unsupported mode\n");
> > +             goto out;
> > +     }
> > +
> > +     user_m = ret;
> > +     ret = 0;
> > +
> > +     /* Do nothing and return success if user asks for current mode */
> > +     if (user_m == mode)
> >               goto out;
> >
> >       if (mode == RDT_MODE_PSEUDO_LOCKED) {
> > @@ -1438,14 +1443,14 @@ static ssize_t rdtgroup_mode_write(struct
> > kernfs_open_file *of,
> >               goto out;
> >       }
> >
> > -     if (!strcmp(buf, "shareable")) {
> > +     if (user_m == RDT_MODE_SHAREABLE) {
> >               if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> >                       ret = rdtgroup_locksetup_exit(rdtgrp);
> >                       if (ret)
> >                               goto out;
> >               }
> >               rdtgrp->mode = RDT_MODE_SHAREABLE;
> > -     } else if (!strcmp(buf, "exclusive")) {
> > +     } else if (user_m == RDT_MODE_EXCLUSIVE) {
> >               if (!rdtgroup_mode_test_exclusive(rdtgrp)) {
> >                       ret = -EINVAL;
> >                       goto out;
> > @@ -1456,14 +1461,11 @@ static ssize_t rdtgroup_mode_write(struct
> > kernfs_open_file *of,
> >                               goto out;
> >               }
> >               rdtgrp->mode = RDT_MODE_EXCLUSIVE;
> > -     } else if (!strcmp(buf, "pseudo-locksetup")) {
> > +     } else if (user_m == RDT_MODE_PSEUDO_LOCKSETUP) {
> >               ret = rdtgroup_locksetup_enter(rdtgrp);
> >               if (ret)
> >                       goto out;
> >               rdtgrp->mode = RDT_MODE_PSEUDO_LOCKSETUP;
> > -     } else {
> > -             rdt_last_cmd_puts("Unknown or unsupported mode\n");
> > -             ret = -EINVAL;
> >       }
> >
> >  out:
> > --
> > 2.21.0
>


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread throttling mode

[Babu Moger]

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Thursday, May 14, 2020 1:11 PM
> To: Moger, Babu <Babu.Moger@amd.com>; tglx@linutronix.de;
> fenghua.yu@intel.com; bp@alien8.de; tony.luck@intel.com
> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
> hpa@zytor.com; x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread
> throttling mode
> 
> Hi Babu,
> 
> Thank you very much for taking a look.
> 
> On 5/14/2020 9:45 AM, Babu Moger wrote:
> > Hi Reinnette,
> >
> > The patches did not apply on my tree. I got the latest tree today. You
> 
> Which tree did you use as baseline? These patches should apply cleanly
> on top of the other resctrl patches already queued for inclusion into
> v5.8 as found on the x86/cache branch of the tip repo.

Oh. Ok. I was using linux master.  Never mind..

> 
> > might want to check again.
> > Hunk #1 FAILED at 29.
> > 1 out of 7 hunks FAILED -- saving rejects to file
> > arch/x86/kernel/cpu/resctrl/rdtgroup.c.rej
> >
> >
> >> -----Original Message-----
> >> From: Reinette Chatre <reinette.chatre@intel.com>
> >> Sent: Wednesday, May 6, 2020 6:50 PM
> >> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
> >> tony.luck@intel.com
> >> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com;
> mingo@redhat.com;
> >> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
> >> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>
> >> Subject: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread
> >> throttling mode
> >>
> 
> ...
> 
> >> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
> >> b/arch/x86/kernel/cpu/resctrl/core.c
> >> index 12f967c6b603..1bc686777069 100644
> >> --- a/arch/x86/kernel/cpu/resctrl/core.c
> >> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> >> @@ -250,6 +250,30 @@ static inline bool rdt_get_mb_table(struct
> rdt_resource
> >> *r)
> >>  	return false;
> >>  }
> >>
> >> +/*
> >> + * Model-specific test to determine if platform where memory bandwidth
> >> + * control is applied to a core can be configured to apply either the
> >> + * maximum or minimum of the per-thread delay values.
> >> + * By default, platforms where memory bandwidth control is applied to a
> >> + * core will select the maximum delay value of the per-thread CLOS.
> >> + *
> >> + * NOTE: delay value programmed to hardware is inverse of bandwidth
> >> + * percentage configured via user interface.
> >> + */
> >> +bool mba_cfg_supports_min_max_intel(void)
> >> +{
> >> +	switch (boot_cpu_data.x86_model) {
> >> +	case INTEL_FAM6_ATOM_TREMONT_D:
> >> +	case INTEL_FAM6_ICELAKE_X:
> >> +	case INTEL_FAM6_ICELAKE_D:
> >> +		return true;
> >> +	default:
> >> +		return false;
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >
> > I see that you are calling this function multiple times. Why don't you
> 
> It is called:
> - once during initialization
> - once per package initialization (when first CPU on a package comes online)
> - once per read from or write to the new "thread_throttle_mode" file
> 
> > make it as a property in rdt_resource. Set it only once during the
> > init(may be in get_mem_config_intel). Then you can use it wherever
> > required. This also probably help James to make everything architecture
> > independent. What do you think?
> 
> If I understand correctly this would require understanding how each
> architecture behaves in this regard to ensure the property within
> rdt_resource is initialized correctly. While it could just be set within
> get_mem_config_intel as you suggest, this property would be present in
> AMD's resource and thus need a value ... could you please provide
> guidance what this property should be on AMD? I looked at the bandwidth
> enforcement section of
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdevelop
> er.amd.com%2Fwp-
> content%2Fresources%2F56375.pdf&amp;data=02%7C01%7Cbabu.moger%40a
> md.com%7C5e7d546489d54002870808d7f8322934%7C3dd8961fe4884e608e11
> a82d994e183d%7C0%7C0%7C637250766898923893&amp;sdata=hQ6usFmXiUX
> p%2BZCrM2qdVn2Z3Ggx1c2g4nVwrEqrzG4%3D&amp;reserved=0 but it is not
> obvious to me where bandwidth is actually enforced and how this
> enforcement is affected when threads and/or cores are running tasks with
> different CLOS that have different bandwidth limits assigned.
> 
> With AMD's properties understood then the new "thread_throttle_mode"
> file could be visible on all platforms, not just Intel, and display
> accurate values for all and be architecture independent.
> 
> Alternatively, the new property could have values: UNDEFINED, MIN, MAX,
> or PER_THREAD ... with AMD having UNDEFINED and the
> "thread_throttle_mode" continues to be visible on Intel platforms only.
> 
> I would appreciate your thoughts on this.

Here is the text from spec.
"Platform QOS features are implemented on a logical processor basis.
Therefore, multiple hardware threads of a single physical CPU core may
have independent resource monitoring and enforcement configurations."

So, bandwidth allocation is already at thread level. But AMD does not use
memory delay throttle model to control the allocation like Intel does. So,
I would say you can initialize it as UNDEFINED not to cause any confusion.


> 
> > I assume that this property is probably not part of CPUID.
> 
> Correct. This specific property is model specific, but also transient in
> that it is replaced by X86_FEATURE_PER_THREAD_MBA (that is part of
> CPUID) in future platforms.
> 
> Reinette

RE: [PATCH V3 4/4] x86/resctrl: Use appropriate API for strings terminated by newline

[Babu Moger]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Thursday, May 14, 2020 2:11 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: Reinette Chatre <reinette.chatre@intel.com>; tglx@linutronix.de;
> fenghua.yu@intel.com; bp@alien8.de; tony.luck@intel.com; kuo-
> lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
> hpa@zytor.com; x86@kernel.org; linux-kernel@vger.kernel.org; Andy
> Shevchenko <andriy.shevchenko@linux.intel.com>
> Subject: Re: [PATCH V3 4/4] x86/resctrl: Use appropriate API for strings
> terminated by newline
> 
> On Thu, May 14, 2020 at 10:08 PM Babu Moger <babu.moger@amd.com>
> wrote:
> > > -----Original Message-----
> > > From: Reinette Chatre <reinette.chatre@intel.com>
> > > Sent: Wednesday, May 6, 2020 6:50 PM
> > > To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
> > > tony.luck@intel.com
> > > Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com;
> mingo@redhat.com;
> > > Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
> > > linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>;
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Subject: [PATCH V3 4/4] x86/resctrl: Use appropriate API for strings
> terminated
> > > by newline
> > >
> > > The user input to files in the resctrl filesystem are expected to be
> > > terminated with a newline. Testing the user input includes a test for
> > > the presence of a newline and then replacing the newline with NUL
> > > byte followed by comparison using strcmp().
> > >
> > > sysfs_streq() exists to test if strings are equal, treating both NUL and
> > > newline-then-NUL as equivalent string terminations. Even more,
> > > sysfs_match_string() exists to match a given string in an array using
> > > sysfs_streq().
> > >
> > > Replace existing strcmp() comparisons of strings that are terminated
> > > with a newline with more appropriate sysfs_streq() via the
> > > sysfs_match_string() API that can perform the match across the different
> > > mode strings that are already maintained in an array.
> > >
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> > > ---
> > >  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 32 ++++++++++++++------------
> > >  1 file changed, 17 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > > index c60a3b307f7d..e70694892c02 100644
> > > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > > @@ -1408,12 +1408,11 @@ static ssize_t rdtgroup_mode_write(struct
> > > kernfs_open_file *of,
> > >  {
> > >       struct rdtgroup *rdtgrp;
> > >       enum rdtgrp_mode mode;
> > > -     int ret = 0;
> > > +     int user_m;
> > > +     int ret;
> >
> > This is bit confusing here. Mode and user_m should have been of same type.
> 
> It can't. If you compare enum to int you will need the explicit (weird) casting.

Ok. Got it. That is fine then.

> > You can define user_m to user_mode to be very clear.
> 
> > You can completely remove "mode" and directly use rdtgrp->mode instead. It
> > is left to you.
> >
> > > > -   /* Valid input requires a trailing newline */
> > > -     if (nbytes == 0 || buf[nbytes - 1] != '\n')
> > > +     if (nbytes == 0)
> > >               return -EINVAL;
> > > -     buf[nbytes - 1] = '\0';
> > >
> > >       rdtgrp = rdtgroup_kn_lock_live(of->kn);
> > >       if (!rdtgrp) {
> > > @@ -1425,11 +1424,17 @@ static ssize_t rdtgroup_mode_write(struct
> > > kernfs_open_file *of,
> > >
> > >       mode = rdtgrp->mode;
> > >
> > > -     if ((!strcmp(buf, "shareable") && mode == RDT_MODE_SHAREABLE) ||
> > > -         (!strcmp(buf, "exclusive") && mode == RDT_MODE_EXCLUSIVE) ||
> > > -         (!strcmp(buf, "pseudo-locksetup") &&
> > > -          mode == RDT_MODE_PSEUDO_LOCKSETUP) ||
> > > -         (!strcmp(buf, "pseudo-locked") && mode ==
> > > RDT_MODE_PSEUDO_LOCKED))
> > > +     ret = sysfs_match_string(rdt_mode_str, buf);
> > > +     if (ret < 0) {
> > > +             rdt_last_cmd_puts("Unknown or unsupported mode\n");
> > > +             goto out;
> > > +     }
> > > +
> > > +     user_m = ret;
> > > +     ret = 0;
> > > +
> > > +     /* Do nothing and return success if user asks for current mode */
> > > +     if (user_m == mode)
> > >               goto out;
> > >
> > >       if (mode == RDT_MODE_PSEUDO_LOCKED) {
> > > @@ -1438,14 +1443,14 @@ static ssize_t rdtgroup_mode_write(struct
> > > kernfs_open_file *of,
> > >               goto out;
> > >       }
> > >
> > > -     if (!strcmp(buf, "shareable")) {
> > > +     if (user_m == RDT_MODE_SHAREABLE) {
> > >               if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> > >                       ret = rdtgroup_locksetup_exit(rdtgrp);
> > >                       if (ret)
> > >                               goto out;
> > >               }
> > >               rdtgrp->mode = RDT_MODE_SHAREABLE;
> > > -     } else if (!strcmp(buf, "exclusive")) {
> > > +     } else if (user_m == RDT_MODE_EXCLUSIVE) {
> > >               if (!rdtgroup_mode_test_exclusive(rdtgrp)) {
> > >                       ret = -EINVAL;
> > >                       goto out;
> > > @@ -1456,14 +1461,11 @@ static ssize_t rdtgroup_mode_write(struct
> > > kernfs_open_file *of,
> > >                               goto out;
> > >               }
> > >               rdtgrp->mode = RDT_MODE_EXCLUSIVE;
> > > -     } else if (!strcmp(buf, "pseudo-locksetup")) {
> > > +     } else if (user_m == RDT_MODE_PSEUDO_LOCKSETUP) {
> > >               ret = rdtgroup_locksetup_enter(rdtgrp);
> > >               if (ret)
> > >                       goto out;
> > >               rdtgrp->mode = RDT_MODE_PSEUDO_LOCKSETUP;
> > > -     } else {
> > > -             rdt_last_cmd_puts("Unknown or unsupported mode\n");
> > > -             ret = -EINVAL;
> > >       }
> > >
> > >  out:
> > > --
> > > 2.21.0
> >
> 
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH V3 2/4] x86/resctrl: Enumerate per-thread MBA

[Reinette Chatre]

Hi Babu,

On 5/14/2020 12:04 PM, Babu Moger wrote:
> 
> 
>> -----Original Message-----
>> From: Reinette Chatre <reinette.chatre@intel.com>
>> Sent: Wednesday, May 6, 2020 6:50 PM
>> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
>> tony.luck@intel.com
>> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
>> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
>> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>
>> Subject: [PATCH V3 2/4] x86/resctrl: Enumerate per-thread MBA
>>
>> From: Fenghua Yu <fenghua.yu@intel.com>
>>

...

>> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
>> index 62b137c3c97a..bccfc9ff3cc1 100644
>> --- a/arch/x86/kernel/cpu/scattered.c
>> +++ b/arch/x86/kernel/cpu/scattered.c
>> @@ -35,6 +35,7 @@ static const struct cpuid_bit cpuid_bits[] = {
>>  	{ X86_FEATURE_CDP_L3,		CPUID_ECX,  2, 0x00000010, 1
>> },
>>  	{ X86_FEATURE_CDP_L2,		CPUID_ECX,  2, 0x00000010, 2
>> },
>>  	{ X86_FEATURE_MBA,		CPUID_EBX,  3, 0x00000010, 0 },
>> +	{ X86_FEATURE_PER_THREAD_MBA,	CPUID_ECX,  0, 0x00000010, 3
>> },
> 
> This is a CPUID feature. You can actually detect this feature without
> checking vendor model in patch @1. This patch looks good to me.
> 

This feature is different from the feature introduced in patch 1.

Reinette

RE: [PATCH V3 2/4] x86/resctrl: Enumerate per-thread MBA

[Babu Moger]

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Thursday, May 14, 2020 3:12 PM
> To: Moger, Babu <Babu.Moger@amd.com>; tglx@linutronix.de;
> fenghua.yu@intel.com; bp@alien8.de; tony.luck@intel.com
> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
> hpa@zytor.com; x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V3 2/4] x86/resctrl: Enumerate per-thread MBA
> 
> Hi Babu,
> 
> On 5/14/2020 12:04 PM, Babu Moger wrote:
> >
> >
> >> -----Original Message-----
> >> From: Reinette Chatre <reinette.chatre@intel.com>
> >> Sent: Wednesday, May 6, 2020 6:50 PM
> >> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
> >> tony.luck@intel.com
> >> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com;
> mingo@redhat.com;
> >> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
> >> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>
> >> Subject: [PATCH V3 2/4] x86/resctrl: Enumerate per-thread MBA
> >>
> >> From: Fenghua Yu <fenghua.yu@intel.com>
> >>
> 
> ...
> 
> >> diff --git a/arch/x86/kernel/cpu/scattered.c
> b/arch/x86/kernel/cpu/scattered.c
> >> index 62b137c3c97a..bccfc9ff3cc1 100644
> >> --- a/arch/x86/kernel/cpu/scattered.c
> >> +++ b/arch/x86/kernel/cpu/scattered.c
> >> @@ -35,6 +35,7 @@ static const struct cpuid_bit cpuid_bits[] = {
> >>  	{ X86_FEATURE_CDP_L3,		CPUID_ECX,  2, 0x00000010, 1
> >> },
> >>  	{ X86_FEATURE_CDP_L2,		CPUID_ECX,  2, 0x00000010, 2
> >> },
> >>  	{ X86_FEATURE_MBA,		CPUID_EBX,  3, 0x00000010, 0 },
> >> +	{ X86_FEATURE_PER_THREAD_MBA,	CPUID_ECX,  0, 0x00000010, 3
> >> },
> >
> > This is a CPUID feature. You can actually detect this feature without
> > checking vendor model in patch @1. This patch looks good to me.
> >
> 
> This feature is different from the feature introduced in patch 1.

Ok.  I mis-understood that.

Re: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread throttling mode

[Reinette Chatre]

Hi Babu,

On 5/14/2020 1:06 PM, Babu Moger wrote:
> 
> 
>> -----Original Message-----
>> From: Reinette Chatre <reinette.chatre@intel.com>
>> Sent: Thursday, May 14, 2020 1:11 PM
>> To: Moger, Babu <Babu.Moger@amd.com>; tglx@linutronix.de;
>> fenghua.yu@intel.com; bp@alien8.de; tony.luck@intel.com
>> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
>> hpa@zytor.com; x86@kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread
>> throttling mode
>>
>> Hi Babu,
>>
>> Thank you very much for taking a look.
>>
>> On 5/14/2020 9:45 AM, Babu Moger wrote:
>>> Hi Reinnette,
>>>
>>> The patches did not apply on my tree. I got the latest tree today. You
>>
>> Which tree did you use as baseline? These patches should apply cleanly
>> on top of the other resctrl patches already queued for inclusion into
>> v5.8 as found on the x86/cache branch of the tip repo.
> 
> Oh. Ok. I was using linux master.  Never mind..

My apologies, I neglected to include the repo to which these patches
apply into the cover letter. Will make sure to do it for next version.


>>>> -----Original Message-----
>>>> From: Reinette Chatre <reinette.chatre@intel.com>
>>>> Sent: Wednesday, May 6, 2020 6:50 PM
>>>> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
>>>> tony.luck@intel.com
>>>> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com;
>> mingo@redhat.com;
>>>> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
>>>> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>
>>>> Subject: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread
>>>> throttling mode
>>>>
>>
>> ...
>>

...

>>
>>> make it as a property in rdt_resource. Set it only once during the
>>> init(may be in get_mem_config_intel). Then you can use it wherever
>>> required. This also probably help James to make everything architecture
>>> independent. What do you think?
>>
>> If I understand correctly this would require understanding how each
>> architecture behaves in this regard to ensure the property within
>> rdt_resource is initialized correctly. While it could just be set within
>> get_mem_config_intel as you suggest, this property would be present in
>> AMD's resource and thus need a value ... could you please provide
>> guidance what this property should be on AMD? I looked at the bandwidth
>> enforcement section of
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdevelop
>> er.amd.com%2Fwp-
>> content%2Fresources%2F56375.pdf&amp;data=02%7C01%7Cbabu.moger%40a
>> md.com%7C5e7d546489d54002870808d7f8322934%7C3dd8961fe4884e608e11
>> a82d994e183d%7C0%7C0%7C637250766898923893&amp;sdata=hQ6usFmXiUX
>> p%2BZCrM2qdVn2Z3Ggx1c2g4nVwrEqrzG4%3D&amp;reserved=0 but it is not
>> obvious to me where bandwidth is actually enforced and how this
>> enforcement is affected when threads and/or cores are running tasks with
>> different CLOS that have different bandwidth limits assigned.
>>
>> With AMD's properties understood then the new "thread_throttle_mode"
>> file could be visible on all platforms, not just Intel, and display
>> accurate values for all and be architecture independent.
>>
>> Alternatively, the new property could have values: UNDEFINED, MIN, MAX,
>> or PER_THREAD ... with AMD having UNDEFINED and the
>> "thread_throttle_mode" continues to be visible on Intel platforms only.
>>
>> I would appreciate your thoughts on this.
> 
> Here is the text from spec.
> "Platform QOS features are implemented on a logical processor basis.
> Therefore, multiple hardware threads of a single physical CPU core may
> have independent resource monitoring and enforcement configurations."
> 
> So, bandwidth allocation is already at thread level. But AMD does not use
> memory delay throttle model to control the allocation like Intel does. So,
> I would say you can initialize it as UNDEFINED not to cause any confusion.

Will do. The next version intends to follow your guidance and moves to
treating this platform feature as a property of the memory bandwidth
resource using the "arch_.." prefix that James also follows and is
intended to be architecture independent.

Thank you very much for your valuable feedback. I look forward to hear
what you think about the next version.

Reinette

Re: [PATCH V3 3/4] x86/resctrl: Enable per-thread MBA

[Reinette Chatre]

Hi Babu,

On 5/14/2020 12:04 PM, Babu Moger wrote:
> 
> 
>> -----Original Message-----
>> From: Reinette Chatre <reinette.chatre@intel.com>
>> Sent: Wednesday, May 6, 2020 6:50 PM
>> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de;
>> tony.luck@intel.com
>> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com;
>> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org;
>> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@intel.com>
>> Subject: [PATCH V3 3/4] x86/resctrl: Enable per-thread MBA
>>
>> From: Fenghua Yu <fenghua.yu@intel.com>
>>

...

>>  static int rdt_thread_throttle_mode_show(struct kernfs_open_file *of,
>>  					 struct seq_file *seq, void *v)
>>  {
>>  	unsigned int throttle_mode = 0;
>>
>> +	if (boot_cpu_has(X86_FEATURE_PER_THREAD_MBA)) {
>> +		seq_puts(seq, "per-thread\n");
>> +
> You probably don't need an extra line here.
> 

This area looks slightly different after moving to a resource property
but unnecessary extra lines will be removed.

Thank you very much

Reinette