* [PATCH V4 1/4] x86/resctrl: Enable user to view and select thread throttling mode
2020-05-16 18:28 [PATCH V4 0/4] x86/resctrl: Enable user to view and select thread throttling mode Reinette Chatre
@ 2020-05-16 18:28 ` Reinette Chatre
2020-05-18 17:28 ` Babu Moger
2020-05-16 18:28 ` [PATCH V4 2/4] x86/resctrl: Enumerate per-thread MBA Reinette Chatre
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Reinette Chatre @ 2020-05-16 18:28 UTC (permalink / raw)
To: tglx, fenghua.yu, bp, tony.luck
Cc: kuo-lang.tseng, ravi.v.shankar, mingo, babu.moger, hpa, x86,
linux-kernel, 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.
AMD confirmed in
https://lore.kernel.org/lkml/18d277fd-6523-319c-d560-66b63ff606b8@amd.com
that AMD bandwidth allocation is already at thread level. But AMD does not
use memory delay throttle model to control the allocation like Intel does.
So, to avoid any confusion the thread throttling mode would be UNDEFINED
on AMD systems and the "thread_throttle_mode" file will not be visible.
Cc: Babu Moger <babu.moger@amd.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
Changes since V3:
- Maintain the thread throttling mode as a property ("arch_throttle_mode")
of the memory bandwidth allocation resource
instead of calling a function that queries the system's model each time
this information is needed. Use this new property generically throughout
with the goal of being independent of architecture. (Babu)
- Remove "intel" from thread_throttle_mode_init_intel_rw() and
thread_throttle_mode_init_intel_ro() in anticipation of usage by other
architectures.
- Use function parameter to update_mba_cfg() directly instead of having
local variable point to it. (Fenghua)
- Remove unnecessary additional check whether platform supports feature
from the "thread_throttle_mode" callback. The file will only be
accessible on platforms that support the feature.
- Rework commit message to be more complete regarding
support of all architectures after incorporating AMD feedback.
- View the thread throttle mode with rdtgroup mutex held since it is using
cached information that may be changed concurrently.
Documentation/x86/resctrl_ui.rst | 19 ++-
arch/x86/kernel/cpu/resctrl/core.c | 54 +++++++
arch/x86/kernel/cpu/resctrl/internal.h | 47 +++++-
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 196 ++++++++++++++++++++++++-
4 files changed, 307 insertions(+), 9 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..129ff0cec7a7 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -250,6 +250,45 @@ static inline bool rdt_get_mb_table(struct rdt_resource *r)
return false;
}
+/*
+ * Restore the MBA configuration from the cached configuration. Used for
+ * the case when an entire package was offline at the time the user made
+ * the configuration change.
+ */
+static void mba_cfg_reconfigure_throttle_mode(struct rdt_resource *r)
+{
+ if (!r->alloc_capable)
+ return;
+
+ if (r == &rdt_resources_all[RDT_RESOURCE_MBA] &&
+ r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX)
+ wrmsrl(MSR_MBA_CFG, mba_cfg_msr);
+}
+
+/*
+ * 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.
+ */
+static 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 +309,14 @@ static bool __get_mem_config_intel(struct rdt_resource *r)
}
r->data_width = 3;
+ if (mba_cfg_supports_min_max_intel()) {
+ r->membw.arch_throttle_mode = THREAD_THROTTLE_MIN_MAX;
+ thread_throttle_mode_init_rw();
+ } else {
+ r->membw.arch_throttle_mode = THREAD_THROTTLE_MAX_ONLY;
+ thread_throttle_mode_init_ro();
+ }
+
r->alloc_capable = true;
r->alloc_enabled = true;
@@ -289,6 +336,11 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r)
/* AMD does not use delay */
r->membw.delay_linear = false;
+ /*
+ * AMD does not use memory delay throttle model to control
+ * the allocation like Intel does.
+ */
+ r->membw.arch_throttle_mode = THREAD_THROTTLE_UNDEFINED;
r->membw.min_bw = 0;
r->membw.bw_gran = 1;
/* Max value is 2048, Data width should be 4 in decimal */
@@ -580,6 +632,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
rdt_domain_reconfigure_cdp(r);
+ mba_cfg_reconfigure_throttle_mode(r);
+
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..6b9b21d67c9b 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;
@@ -368,6 +378,26 @@ struct rdt_cache {
unsigned int shareable_bits;
};
+/**
+ * enum membw_throttle_mode - System's memory bandwidth throttling mode
+ * @THREAD_THROTTLE_UNDEFINED: Not relevant to the system
+ * @THREAD_THROTTLE_MIN_MAX: Memory bandwidth is throttled at the core and
+ * can be configured to use smallest or largest
+ * bandwidth percentage assigned to threads.
+ * Systems that support this mode will support
+ * MSR_MBA_CFG and the configuration of
+ * thread throttling mode via resctrl
+ * file "thread_throttle_mode".
+ * @THREAD_THROTTLE_MAX_ONLY: Memory bandwidth is throttled at the core
+ * always using smallest bandwidth percentage
+ * assigned to threads, aka "max throttling"
+ */
+enum membw_throttle_mode {
+ THREAD_THROTTLE_UNDEFINED = 0,
+ THREAD_THROTTLE_MIN_MAX,
+ THREAD_THROTTLE_MAX_ONLY,
+};
+
/**
* struct rdt_membw - Memory bandwidth allocation related data
* @max_delay: Max throttle delay. Delay is the hardware
@@ -375,16 +405,19 @@ struct rdt_cache {
* @min_bw: Minimum memory bandwidth percentage user can request
* @bw_gran: Granularity at which the memory bandwidth is allocated
* @delay_linear: True if memory B/W delay is in linear scale
+ * @arch_throttle_mode: Bandwidth throttling mode when threads request
+ * different memory bandwidths
* @mba_sc: True if MBA software controller(mba_sc) is enabled
* @mb_map: Mapping of memory B/W percentage to memory B/W delay
*/
struct rdt_membw {
- u32 max_delay;
- u32 min_bw;
- u32 bw_gran;
- u32 delay_linear;
- bool mba_sc;
- u32 *mb_map;
+ u32 max_delay;
+ u32 min_bw;
+ u32 bw_gran;
+ u32 delay_linear;
+ enum membw_throttle_mode arch_throttle_mode;
+ bool mba_sc;
+ u32 *mb_map;
};
static inline bool is_llc_occupancy_enabled(void)
@@ -611,5 +644,7 @@ 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);
+void thread_throttle_mode_init_rw(void);
+void thread_throttle_mode_init_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..3ce6319b7226 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,141 @@ 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)
+{
+ struct rdt_resource *r = of->kn->parent->priv;
+ unsigned int throttle_mode = 0;
+
+ mutex_lock(&rdtgroup_mutex);
+
+ if (r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX)
+ throttle_mode = mba_cfg_msr & MBA_THROTTLE_MODE_MASK;
+
+ seq_puts(seq,
+ throttle_mode == MBA_THROTTLE_MODE_MIN ? "min\n" : "max\n");
+
+ mutex_unlock(&rdtgroup_mutex);
+ return 0;
+}
+
+static void update_mba_cfg(void *data)
+{
+ wrmsrl(MSR_MBA_CFG, *(u64 *)data);
+}
+
+/*
+ * 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;
+}
+
+static void mba_cfg_reset_all(void)
+{
+ struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA];
+
+ if (!r->alloc_capable)
+ return;
+
+ if (r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX)
+ rdt_system_mba_cfg_set(0);
+}
+
+/*
+ * Callback will only be associated with the "thread_throttle_mode" file on
+ * systems that are capable of memory bandwidth allocation
+ * (RDT_RESOURCE_MBA is alloc_capable) AND the thread throttle mode is
+ * THREAD_THROTTLE_MIN_MAX.
+ *
+ * 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();
+
+ 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 +1649,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 +1718,52 @@ 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;
+}
+
+/*
+ * Only systems that support memory bandwidth allocation and have
+ * THREAD_THROTTLE_MIN_MAX set will support a writable thread_throttle_mode
+ * file with which the user can modify the thread throttling mode.
+ */
+void __init thread_throttle_mode_init_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_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 +1775,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;
@@ -2371,6 +2564,7 @@ static void rdt_kill_sb(struct super_block *sb)
/*Put everything back to default values. */
for_each_alloc_enabled_rdt_resource(r)
reset_all_ctrls(r);
+ mba_cfg_reset_all();
cdp_disable_all();
rmdir_all_sub();
rdt_pseudo_lock_release();
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* RE: [PATCH V4 1/4] x86/resctrl: Enable user to view and select thread throttling mode
2020-05-16 18:28 ` [PATCH V4 1/4] " Reinette Chatre
@ 2020-05-18 17:28 ` Babu Moger
2020-05-18 22:46 ` Reinette Chatre
0 siblings, 1 reply; 12+ messages in thread
From: Babu Moger @ 2020-05-18 17:28 UTC (permalink / raw)
To: Reinette Chatre, tglx, fenghua.yu, bp, tony.luck
Cc: kuo-lang.tseng, ravi.v.shankar, mingo, hpa, x86, linux-kernel
Hi Reinette,
Looks good most part. Few minor comments.
> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Saturday, May 16, 2020 1:29 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 V4 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.
>
> AMD confirmed in
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ker
> nel.org%2Flkml%2F18d277fd-6523-319c-d560-
> 66b63ff606b8%40amd.com&data=02%7C01%7Cbabu.moger%40amd.com
> %7C49a96bbf29ca4d7fa65d08d7f9c70d7e%7C3dd8961fe4884e608e11a82d994
> e183d%7C0%7C0%7C637252505635232310&sdata=Z6cFJuy50gDuGDdh%2
> F0yi0YKHHfrbX0vETW3ZdYNDDGE%3D&reserved=0
> that AMD bandwidth allocation is already at thread level. But AMD does not
> use memory delay throttle model to control the allocation like Intel does.
> So, to avoid any confusion the thread throttling mode would be UNDEFINED
> on AMD systems and the "thread_throttle_mode" file will not be visible.
>
> Cc: Babu Moger <babu.moger@amd.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Changes since V3:
> - Maintain the thread throttling mode as a property ("arch_throttle_mode")
> of the memory bandwidth allocation resource
> instead of calling a function that queries the system's model each time
> this information is needed. Use this new property generically throughout
> with the goal of being independent of architecture. (Babu)
> - Remove "intel" from thread_throttle_mode_init_intel_rw() and
> thread_throttle_mode_init_intel_ro() in anticipation of usage by other
> architectures.
> - Use function parameter to update_mba_cfg() directly instead of having
> local variable point to it. (Fenghua)
> - Remove unnecessary additional check whether platform supports feature
> from the "thread_throttle_mode" callback. The file will only be
> accessible on platforms that support the feature.
> - Rework commit message to be more complete regarding
> support of all architectures after incorporating AMD feedback.
> - View the thread throttle mode with rdtgroup mutex held since it is using
> cached information that may be changed concurrently.
>
> Documentation/x86/resctrl_ui.rst | 19 ++-
> arch/x86/kernel/cpu/resctrl/core.c | 54 +++++++
> arch/x86/kernel/cpu/resctrl/internal.h | 47 +++++-
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 196 ++++++++++++++++++++++++-
> 4 files changed, 307 insertions(+), 9 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..129ff0cec7a7 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -250,6 +250,45 @@ static inline bool rdt_get_mb_table(struct rdt_resource
> *r)
> return false;
> }
>
> +/*
> + * Restore the MBA configuration from the cached configuration. Used for
> + * the case when an entire package was offline at the time the user made
> + * the configuration change.
> + */
> +static void mba_cfg_reconfigure_throttle_mode(struct rdt_resource *r)
> +{
> + if (!r->alloc_capable)
> + return;
> +
> + if (r == &rdt_resources_all[RDT_RESOURCE_MBA] &&
> + r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX)
> + wrmsrl(MSR_MBA_CFG, mba_cfg_msr);
> +}
How about this? It is kind of consistent with other checks that are done.
If (r->alloc_capable && (r == &rdt_resources_all[RDT_RESOURCE_MBA]) &&
(r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX))
wrmsrl(MSR_MBA_CFG, mba_cfg_msr);
> +
> +/*
> + * 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.
> + */
> +static 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;
Is this last return required? I don't think so. We will never go here.
> +}
> +
> static bool __get_mem_config_intel(struct rdt_resource *r)
> {
> union cpuid_0x10_3_eax eax;
> @@ -270,6 +309,14 @@ static bool __get_mem_config_intel(struct
> rdt_resource *r)
> }
> r->data_width = 3;
>
> + if (mba_cfg_supports_min_max_intel()) {
> + r->membw.arch_throttle_mode =
> THREAD_THROTTLE_MIN_MAX;
> + thread_throttle_mode_init_rw();
> + } else {
> + r->membw.arch_throttle_mode =
> THREAD_THROTTLE_MAX_ONLY;
> + thread_throttle_mode_init_ro();
> + }
> +
> r->alloc_capable = true;
> r->alloc_enabled = true;
>
> @@ -289,6 +336,11 @@ static bool __rdt_get_mem_config_amd(struct
> rdt_resource *r)
> /* AMD does not use delay */
> r->membw.delay_linear = false;
>
> + /*
> + * AMD does not use memory delay throttle model to control
> + * the allocation like Intel does.
> + */
> + r->membw.arch_throttle_mode = THREAD_THROTTLE_UNDEFINED;
> r->membw.min_bw = 0;
> r->membw.bw_gran = 1;
> /* Max value is 2048, Data width should be 4 in decimal */
> @@ -580,6 +632,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource
> *r)
>
> rdt_domain_reconfigure_cdp(r);
>
> + mba_cfg_reconfigure_throttle_mode(r);
> +
> 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..6b9b21d67c9b 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;
> @@ -368,6 +378,26 @@ struct rdt_cache {
> unsigned int shareable_bits;
> };
>
> +/**
> + * enum membw_throttle_mode - System's memory bandwidth throttling mode
> + * @THREAD_THROTTLE_UNDEFINED: Not relevant to the system
> + * @THREAD_THROTTLE_MIN_MAX: Memory bandwidth is throttled at the
> core and
> + * can be configured to use smallest or largest
> + * bandwidth percentage assigned to threads.
> + * Systems that support this mode will support
> + * MSR_MBA_CFG and the configuration of
> + * thread throttling mode via resctrl
> + * file "thread_throttle_mode".
> + * @THREAD_THROTTLE_MAX_ONLY: Memory bandwidth is throttled at the
> core
> + * always using smallest bandwidth percentage
> + * assigned to threads, aka "max throttling"
> + */
> +enum membw_throttle_mode {
> + THREAD_THROTTLE_UNDEFINED = 0,
> + THREAD_THROTTLE_MIN_MAX,
> + THREAD_THROTTLE_MAX_ONLY,
> +};
> +
> /**
> * struct rdt_membw - Memory bandwidth allocation related data
> * @max_delay: Max throttle delay. Delay is the hardware
> @@ -375,16 +405,19 @@ struct rdt_cache {
> * @min_bw: Minimum memory bandwidth percentage user can
> request
> * @bw_gran: Granularity at which the memory bandwidth is allocated
> * @delay_linear: True if memory B/W delay is in linear scale
> + * @arch_throttle_mode: Bandwidth throttling mode when threads
> request
> + * different memory bandwidths
> * @mba_sc: True if MBA software controller(mba_sc) is enabled
> * @mb_map: Mapping of memory B/W percentage to memory B/W
> delay
> */
> struct rdt_membw {
> - u32 max_delay;
> - u32 min_bw;
> - u32 bw_gran;
> - u32 delay_linear;
> - bool mba_sc;
> - u32 *mb_map;
> + u32 max_delay;
> + u32 min_bw;
> + u32 bw_gran;
> + u32 delay_linear;
> + enum membw_throttle_mode arch_throttle_mode;
> + bool mba_sc;
> + u32 *mb_map;
> };
>
> static inline bool is_llc_occupancy_enabled(void)
> @@ -611,5 +644,7 @@ 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);
> +void thread_throttle_mode_init_rw(void);
> +void thread_throttle_mode_init_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..3ce6319b7226 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,141 @@ 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)
> +{
> + struct rdt_resource *r = of->kn->parent->priv;
> + unsigned int throttle_mode = 0;
> +
> + mutex_lock(&rdtgroup_mutex);
> +
> + if (r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX)
> + throttle_mode = mba_cfg_msr &
> MBA_THROTTLE_MODE_MASK;
> +
> + seq_puts(seq,
> + throttle_mode == MBA_THROTTLE_MODE_MIN ? "min\n" :
> "max\n");
> +
> + mutex_unlock(&rdtgroup_mutex);
> + return 0;
> +}
> +
> +static void update_mba_cfg(void *data)
> +{
> + wrmsrl(MSR_MBA_CFG, *(u64 *)data);
> +}
> +
> +/*
> + * 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;
> +}
> +
> +static void mba_cfg_reset_all(void)
> +{
> + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA];
> +
> + if (!r->alloc_capable)
> + return;
> +
> + if (r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX)
> + rdt_system_mba_cfg_set(0);
> +}
> +
> +/*
> + * Callback will only be associated with the "thread_throttle_mode" file on
> + * systems that are capable of memory bandwidth allocation
> + * (RDT_RESOURCE_MBA is alloc_capable) AND the thread throttle mode is
> + * THREAD_THROTTLE_MIN_MAX.
> + *
> + * 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();
> +
> + 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 +1649,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 +1718,52 @@ 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;
> +}
> +
> +/*
> + * Only systems that support memory bandwidth allocation and have
> + * THREAD_THROTTLE_MIN_MAX set will support a writable
> thread_throttle_mode
> + * file with which the user can modify the thread throttling mode.
> + */
> +void __init thread_throttle_mode_init_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_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 +1775,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;
> @@ -2371,6 +2564,7 @@ static void rdt_kill_sb(struct super_block *sb)
> /*Put everything back to default values. */
> for_each_alloc_enabled_rdt_resource(r)
> reset_all_ctrls(r);
> + mba_cfg_reset_all();
> cdp_disable_all();
> rmdir_all_sub();
> rdt_pseudo_lock_release();
> --
> 2.21.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V4 1/4] x86/resctrl: Enable user to view and select thread throttling mode
2020-05-18 17:28 ` Babu Moger
@ 2020-05-18 22:46 ` Reinette Chatre
0 siblings, 0 replies; 12+ messages in thread
From: Reinette Chatre @ 2020-05-18 22:46 UTC (permalink / raw)
To: Babu Moger, tglx, fenghua.yu, bp, tony.luck
Cc: kuo-lang.tseng, ravi.v.shankar, mingo, hpa, x86, linux-kernel
Hi Babu,
On 5/18/2020 10:28 AM, Babu Moger wrote:
> Hi Reinette,
> Looks good most part. Few minor comments.
Thank you very much for taking a look.
>
>> -----Original Message-----
>> From: Reinette Chatre <reinette.chatre@intel.com>
>> Sent: Saturday, May 16, 2020 1:29 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 V4 1/4] x86/resctrl: Enable user to view and select thread
>> throttling mode
>>
...
>> +static void mba_cfg_reconfigure_throttle_mode(struct rdt_resource *r)
>> +{
>> + if (!r->alloc_capable)
>> + return;
>> +
>> + if (r == &rdt_resources_all[RDT_RESOURCE_MBA] &&
>> + r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX)
>> + wrmsrl(MSR_MBA_CFG, mba_cfg_msr);
>> +}
>
> How about this? It is kind of consistent with other checks that are done.
>
> If (r->alloc_capable && (r == &rdt_resources_all[RDT_RESOURCE_MBA]) &&
> (r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX))
> wrmsrl(MSR_MBA_CFG, mba_cfg_msr);
>
Sure. Will do (with fewer parentheses).
>> +
>> +/*
>> + * 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.
>> + */
>> +static 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;
>
> Is this last return required? I don't think so. We will never go here.
>
Indeed. Thank you for catching this.
Reinette
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V4 2/4] x86/resctrl: Enumerate per-thread MBA
2020-05-16 18:28 [PATCH V4 0/4] x86/resctrl: Enable user to view and select thread throttling mode Reinette Chatre
2020-05-16 18:28 ` [PATCH V4 1/4] " Reinette Chatre
@ 2020-05-16 18:28 ` Reinette Chatre
2020-05-18 17:29 ` Babu Moger
2020-05-16 18:28 ` [PATCH V4 3/4] x86/resctrl: Enable " Reinette Chatre
2020-05-16 18:28 ` [PATCH V4 4/4] x86/resctrl: Use appropriate API for strings terminated by newline Reinette Chatre
3 siblings, 1 reply; 12+ messages in thread
From: Reinette Chatre @ 2020-05-16 18:28 UTC (permalink / raw)
To: tglx, fenghua.yu, bp, tony.luck
Cc: kuo-lang.tseng, ravi.v.shankar, mingo, babu.moger, hpa, x86,
linux-kernel, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* RE: [PATCH V4 2/4] x86/resctrl: Enumerate per-thread MBA
2020-05-16 18:28 ` [PATCH V4 2/4] x86/resctrl: Enumerate per-thread MBA Reinette Chatre
@ 2020-05-18 17:29 ` Babu Moger
0 siblings, 0 replies; 12+ messages in thread
From: Babu Moger @ 2020-05-18 17:29 UTC (permalink / raw)
To: Reinette Chatre, tglx, fenghua.yu, bp, tony.luck
Cc: kuo-lang.tseng, ravi.v.shankar, mingo, hpa, x86, linux-kernel
> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Saturday, May 16, 2020 1:29 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 V4 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>
Reviewed-by: Babu Moger <babu.moger@amd.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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V4 3/4] x86/resctrl: Enable per-thread MBA
2020-05-16 18:28 [PATCH V4 0/4] x86/resctrl: Enable user to view and select thread throttling mode Reinette Chatre
2020-05-16 18:28 ` [PATCH V4 1/4] " Reinette Chatre
2020-05-16 18:28 ` [PATCH V4 2/4] x86/resctrl: Enumerate per-thread MBA Reinette Chatre
@ 2020-05-16 18:28 ` Reinette Chatre
2020-05-18 17:29 ` Babu Moger
2020-05-16 18:28 ` [PATCH V4 4/4] x86/resctrl: Use appropriate API for strings terminated by newline Reinette Chatre
3 siblings, 1 reply; 12+ messages in thread
From: Reinette Chatre @ 2020-05-16 18:28 UTC (permalink / raw)
To: tglx, fenghua.yu, bp, tony.luck
Cc: kuo-lang.tseng, ravi.v.shankar, mingo, babu.moger, hpa, x86,
linux-kernel, 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>
[reinette: transition patch to use membw_throttle_mode enum]
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
Changes since V3:
- Use new thread throttling mode property.
- Remove unnecessary empty line. (Babu)
Documentation/x86/resctrl_ui.rst | 3 +++
arch/x86/kernel/cpu/resctrl/core.c | 5 ++++-
arch/x86/kernel/cpu/resctrl/internal.h | 2 ++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 11 +++++++++++
4 files changed, 20 insertions(+), 1 deletion(-)
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/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 129ff0cec7a7..bf1ff07efac8 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -309,7 +309,10 @@ static bool __get_mem_config_intel(struct rdt_resource *r)
}
r->data_width = 3;
- if (mba_cfg_supports_min_max_intel()) {
+ if (boot_cpu_has(X86_FEATURE_PER_THREAD_MBA)) {
+ r->membw.arch_throttle_mode = THREAD_THROTTLE_PER_THREAD;
+ thread_throttle_mode_init_ro();
+ } else if (mba_cfg_supports_min_max_intel()) {
r->membw.arch_throttle_mode = THREAD_THROTTLE_MIN_MAX;
thread_throttle_mode_init_rw();
} else {
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 6b9b21d67c9b..e198ea2a8468 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -391,11 +391,13 @@ struct rdt_cache {
* @THREAD_THROTTLE_MAX_ONLY: Memory bandwidth is throttled at the core
* always using smallest bandwidth percentage
* assigned to threads, aka "max throttling"
+ * @THREAD_THROTTLE_PER_THREAD: Memory bandwidth is throttled at the thread
*/
enum membw_throttle_mode {
THREAD_THROTTLE_UNDEFINED = 0,
THREAD_THROTTLE_MIN_MAX,
THREAD_THROTTLE_MAX_ONLY,
+ THREAD_THROTTLE_PER_THREAD,
};
/**
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3ce6319b7226..088a1536bccc 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1038,6 +1038,11 @@ 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)
@@ -1047,12 +1052,18 @@ static int rdt_thread_throttle_mode_show(struct kernfs_open_file *of,
mutex_lock(&rdtgroup_mutex);
+ if (r->membw.arch_throttle_mode == THREAD_THROTTLE_PER_THREAD) {
+ seq_puts(seq, "per-thread\n");
+ goto out;
+ }
+
if (r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX)
throttle_mode = mba_cfg_msr & MBA_THROTTLE_MODE_MASK;
seq_puts(seq,
throttle_mode == MBA_THROTTLE_MODE_MIN ? "min\n" : "max\n");
+out:
mutex_unlock(&rdtgroup_mutex);
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* RE: [PATCH V4 3/4] x86/resctrl: Enable per-thread MBA
2020-05-16 18:28 ` [PATCH V4 3/4] x86/resctrl: Enable " Reinette Chatre
@ 2020-05-18 17:29 ` Babu Moger
0 siblings, 0 replies; 12+ messages in thread
From: Babu Moger @ 2020-05-18 17:29 UTC (permalink / raw)
To: Reinette Chatre, tglx, fenghua.yu, bp, tony.luck
Cc: kuo-lang.tseng, ravi.v.shankar, mingo, hpa, x86, linux-kernel
> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Saturday, May 16, 2020 1:29 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 V4 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>
> [reinette: transition patch to use membw_throttle_mode enum]
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
Reviewed-by: Babu Moger <babu.moger@amd.com>
> ---
> Changes since V3:
> - Use new thread throttling mode property.
> - Remove unnecessary empty line. (Babu)
>
> Documentation/x86/resctrl_ui.rst | 3 +++
> arch/x86/kernel/cpu/resctrl/core.c | 5 ++++-
> arch/x86/kernel/cpu/resctrl/internal.h | 2 ++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 11 +++++++++++
> 4 files changed, 20 insertions(+), 1 deletion(-)
>
> 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/core.c
> b/arch/x86/kernel/cpu/resctrl/core.c
> index 129ff0cec7a7..bf1ff07efac8 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -309,7 +309,10 @@ static bool __get_mem_config_intel(struct
> rdt_resource *r)
> }
> r->data_width = 3;
>
> - if (mba_cfg_supports_min_max_intel()) {
> + if (boot_cpu_has(X86_FEATURE_PER_THREAD_MBA)) {
> + r->membw.arch_throttle_mode =
> THREAD_THROTTLE_PER_THREAD;
> + thread_throttle_mode_init_ro();
> + } else if (mba_cfg_supports_min_max_intel()) {
> r->membw.arch_throttle_mode =
> THREAD_THROTTLE_MIN_MAX;
> thread_throttle_mode_init_rw();
> } else {
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> b/arch/x86/kernel/cpu/resctrl/internal.h
> index 6b9b21d67c9b..e198ea2a8468 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -391,11 +391,13 @@ struct rdt_cache {
> * @THREAD_THROTTLE_MAX_ONLY: Memory bandwidth is throttled at the
> core
> * always using smallest bandwidth percentage
> * assigned to threads, aka "max throttling"
> + * @THREAD_THROTTLE_PER_THREAD: Memory bandwidth is throttled
> at the thread
> */
> enum membw_throttle_mode {
> THREAD_THROTTLE_UNDEFINED = 0,
> THREAD_THROTTLE_MIN_MAX,
> THREAD_THROTTLE_MAX_ONLY,
> + THREAD_THROTTLE_PER_THREAD,
> };
>
> /**
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 3ce6319b7226..088a1536bccc 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1038,6 +1038,11 @@ 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)
> @@ -1047,12 +1052,18 @@ static int rdt_thread_throttle_mode_show(struct
> kernfs_open_file *of,
>
> mutex_lock(&rdtgroup_mutex);
>
> + if (r->membw.arch_throttle_mode ==
> THREAD_THROTTLE_PER_THREAD) {
> + seq_puts(seq, "per-thread\n");
> + goto out;
> + }
> +
> if (r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX)
> throttle_mode = mba_cfg_msr &
> MBA_THROTTLE_MODE_MASK;
>
> seq_puts(seq,
> throttle_mode == MBA_THROTTLE_MODE_MIN ? "min\n" :
> "max\n");
>
> +out:
> mutex_unlock(&rdtgroup_mutex);
> return 0;
> }
> --
> 2.21.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V4 4/4] x86/resctrl: Use appropriate API for strings terminated by newline
2020-05-16 18:28 [PATCH V4 0/4] x86/resctrl: Enable user to view and select thread throttling mode Reinette Chatre
` (2 preceding siblings ...)
2020-05-16 18:28 ` [PATCH V4 3/4] x86/resctrl: Enable " Reinette Chatre
@ 2020-05-16 18:28 ` Reinette Chatre
2020-05-18 11:50 ` Andy Shevchenko
3 siblings, 1 reply; 12+ messages in thread
From: Reinette Chatre @ 2020-05-16 18:28 UTC (permalink / raw)
To: tglx, fenghua.yu, bp, tony.luck
Cc: kuo-lang.tseng, ravi.v.shankar, mingo, babu.moger, hpa, x86,
linux-kernel, Reinette Chatre, Andy Shevchenko
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 088a1536bccc..0e302d3c44e6 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1415,12 +1415,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) {
@@ -1432,11 +1431,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) {
@@ -1445,14 +1450,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;
@@ -1463,14 +1468,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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH V4 4/4] x86/resctrl: Use appropriate API for strings terminated by newline
2020-05-16 18:28 ` [PATCH V4 4/4] x86/resctrl: Use appropriate API for strings terminated by newline Reinette Chatre
@ 2020-05-18 11:50 ` Andy Shevchenko
2020-05-18 16:06 ` Reinette Chatre
0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-05-18 11:50 UTC (permalink / raw)
To: Reinette Chatre
Cc: tglx, fenghua.yu, bp, tony.luck, kuo-lang.tseng, ravi.v.shankar,
mingo, babu.moger, hpa, x86, linux-kernel
On Sat, May 16, 2020 at 11:28:41AM -0700, Reinette Chatre wrote:
> 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.
Sorry for late comment, but just have noticed...
> if (mode == RDT_MODE_PSEUDO_LOCKED) {
> @@ -1445,14 +1450,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;
...can we simple move this and similar (in other branches) to...
> - } else {
> - rdt_last_cmd_puts("Unknown or unsupported mode\n");
> - ret = -EINVAL;
> }
...here as
rdtgrp->mode = user_m;
?
> out:
Note, I didn't check all branches carefully.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V4 4/4] x86/resctrl: Use appropriate API for strings terminated by newline
2020-05-18 11:50 ` Andy Shevchenko
@ 2020-05-18 16:06 ` Reinette Chatre
2020-05-18 16:23 ` Reinette Chatre
0 siblings, 1 reply; 12+ messages in thread
From: Reinette Chatre @ 2020-05-18 16:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: tglx, fenghua.yu, bp, tony.luck, kuo-lang.tseng, ravi.v.shankar,
mingo, babu.moger, hpa, x86, linux-kernel
Hi Andy,
On 5/18/2020 4:50 AM, Andy Shevchenko wrote:
> On Sat, May 16, 2020 at 11:28:41AM -0700, Reinette Chatre wrote:
>> 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.
>
> Sorry for late comment, but just have noticed...
No problem. I do appreciate your feedback because it helps me to improve
the code.
>
>> if (mode == RDT_MODE_PSEUDO_LOCKED) {
>> @@ -1445,14 +1450,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;
>
> ...can we simple move this and similar (in other branches) to...
>
>
>> - } else {
>> - rdt_last_cmd_puts("Unknown or unsupported mode\n");
>> - ret = -EINVAL;
>> }
>
> ...here as
>
> rdtgrp->mode = user_m;
Will do.
It also looks like the only reason for the "mode" local variable was to
make those earlier "strcmp" lines shorter. With those long lines removed
in this patch this local variable is no longer needed and I will remove
it also.
Reinette
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V4 4/4] x86/resctrl: Use appropriate API for strings terminated by newline
2020-05-18 16:06 ` Reinette Chatre
@ 2020-05-18 16:23 ` Reinette Chatre
0 siblings, 0 replies; 12+ messages in thread
From: Reinette Chatre @ 2020-05-18 16:23 UTC (permalink / raw)
To: Andy Shevchenko
Cc: tglx, fenghua.yu, bp, tony.luck, kuo-lang.tseng, ravi.v.shankar,
mingo, babu.moger, hpa, x86, linux-kernel
Hi Andy,
On 5/18/2020 9:06 AM, Reinette Chatre wrote:
> On 5/18/2020 4:50 AM, Andy Shevchenko wrote:
>> On Sat, May 16, 2020 at 11:28:41AM -0700, Reinette Chatre wrote:
>>> 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.
>>
>> Sorry for late comment, but just have noticed...
>
> No problem. I do appreciate your feedback because it helps me to improve
> the code.
>
>>
>>> if (mode == RDT_MODE_PSEUDO_LOCKED) {
>>> @@ -1445,14 +1450,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;
>>
>> ...can we simple move this and similar (in other branches) to...
>>
>>
>>> - } else {
>>> - rdt_last_cmd_puts("Unknown or unsupported mode\n");
>>> - ret = -EINVAL;
>>> }
>>
>> ...here as
>>
>> rdtgrp->mode = user_m;
>
> Will do.
Actually ... now that I take a closer look, this would let an invalid
mode change (RDT_MODE_PSEUDO_LOCKED) slip through so I plan to keep this
code as is.
> It also looks like the only reason for the "mode" local variable was to
> make those earlier "strcmp" lines shorter. With those long lines removed
> in this patch this local variable is no longer needed and I will remove
> it also.
I will still do this.
Reinette
^ permalink raw reply [flat|nested] 12+ messages in thread