Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1555863 > unrolled thread

[PATCH 0/8 V1] x86/intel_rdt: Memory b/w Allocation support

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8 V1] x86/intel_rdt: Memory b/w Allocation support Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-10 20:40 +0100
    [PATCH 7/8] x86/intel_rdt/mba: Add schemata file support for MBA Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-10 20:40 +0100
      Re: [PATCH 7/8] x86/intel_rdt/mba: Add schemata file support for  MBA Thomas Gleixner <tglx@linutronix.de> - 2017-01-16 17:10 +0100
    [PATCH 5/8] x86/intel_rct/mba: Add MBA structures and initialize MBA Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-10 20:40 +0100
      Re: [PATCH 5/8] x86/intel_rct/mba: Add MBA structures and initialize  MBA Thomas Gleixner <tglx@linutronix.de> - 2017-01-16 15:10 +0100
    [PATCH 8/8] x86/intel_rdt: rmdir,umount and hotcpu updates for MBA Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-10 20:40 +0100
      Re: [PATCH 8/8] x86/intel_rdt: rmdir,umount and hotcpu updates for  MBA Thomas Gleixner <tglx@linutronix.de> - 2017-01-16 17:20 +0100

#1555863 — [PATCH 0/8 V1] x86/intel_rdt: Memory b/w Allocation support

Memory b/w allocation(MBA) is part of the Intel Resource Director
Technology (RDT). RDT helps monitor and share processor shared
resources.  MBA helps enforce a limit on the memory b/w, threads can use
when they are scheduled. OS does the enforcement using MSR(model
specific register) interface and mapping the threads to architecture
specific CLOSids(class of service IDs) just like the cache allocation
technology(CAT). The interface is the resctrl just like CAT.

This can be used along with MBM (memory b/w monitoring) and cache
allocation to control/restrict the applications cache and memory
resources as per the QoS or other performance requirements.  Use cases
could be large serverclusters, VM, clould and container based services
where the admin or orchestration tools can use this framework to
manage/allocate these processor shared resources like other memory/cpu
resources to provide QoS guarentees.

Sending the first patch series to support the feature. Patches are based
on on 4.10-rc2.

[PATCH 1/8] Documentation, x86: Documentation for Intel Mem b/w
[PATCH 2/8] x86/intel_rdt/mba: Generalize the naming to get ready for
[PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT
[PATCH 4/8] x86/intel_rdt/mba: Memory b/w allocation feature detect
[PATCH 5/8] x86/intel_rct/mba: Add MBA structures and initialize MBA
[PATCH 6/8] x86/intel_rdt/mba: Add info directory files for MBA
[PATCH 7/8] x86/intel_rdt/mba: Add schemata file support for MBA
[PATCH 8/8] x86/intel_rdt: rmdir,umount and hotcpu updates for MBA

[toc] | [next] | [standalone]

#1555864 — [PATCH 7/8] x86/intel_rdt/mba: Add schemata file support for MBA

Add support to update the MBA throttle_by values for the domains.
The MBA throttle_by values are specified for each domain which is L3
cache. The schemata string is parsed and validated for the correct
throttle_by values.

The throttle_by granularity is 100-max_throttle_by if scale is linear
and 2^n if non-linear scale.  OS then updates the corresponding domain
PQOS_MSRs which are indexed from 0xD50 for MBA.  The schemata APIs for
parsing and validating the schemata input are changed to accommodate
handling of both cbm and throttle values.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/include/asm/intel_rdt.h         |  7 ++++
 arch/x86/kernel/cpu/intel_rdt.c          | 35 +++++++++++------
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 66 +++++++++++++++++++++++---------
 3 files changed, 79 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 88725b6..dbf4c6e 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -76,6 +76,8 @@ struct rftype {
  * @no_ctrl:			Specifies max cache cbm or min mem b/w delay.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
+ * @display_str:		Format string to show schemata
+ * @validate:			API to validate the ctrl values.
  * @info_files:		resctrl info files for the resource
  * @infofiles_len:		Number of info files
  * @max_delay:		Max throttle delay
@@ -99,6 +101,9 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			no_ctrl;
+	char			*display_str;
+	int (*validate)		(char *buf, unsigned long *data,
+				  struct rdt_resource *r);
 	struct rftype		*info_files;
 	int			infofiles_len;
 	u32			max_delay;
@@ -142,6 +147,8 @@ struct msr_param {
 
 void rdt_get_cache_infofile(struct rdt_resource *r);
 void rdt_get_mbe_infofile(struct rdt_resource *r);
+int thrtl_validate(char *buf, unsigned long *data, struct rdt_resource *r);
+int cbm_validate(char *buf, unsigned long *data, struct rdt_resource *r);
 
 extern struct mutex rdtgroup_mutex;
 
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index bdfbd1d..9b0a00e 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -138,7 +138,7 @@ static inline bool cache_alloc_hsw_probe(void)
 	return false;
 }
 
-static void rdt_get_mem_config(struct rdt_resource *r)
+static int rdt_get_mem_config(struct rdt_resource *r)
 {
 	union cpuid_0x10_3_eax eax;
 	union cpuid_0x10_x_edx edx;
@@ -155,11 +155,18 @@ static void rdt_get_mem_config(struct rdt_resource *r)
 		r->delay_gran = MAX_MBA_THRTL - r->max_delay;
 
 	rdt_get_mbe_infofile(r);
+	r->validate = thrtl_validate;
+	r->display_str = kstrdup("%d=%d", GFP_KERNEL);
+	if (!r->display_str)
+		return -ENOMEM;
+
 	r->capable = true;
 	r->enabled = true;
+
+	return 0;
 }
 
-static void rdt_get_cache_config(int idx, struct rdt_resource *r)
+static int rdt_get_cache_config(int idx, struct rdt_resource *r)
 {
 	union cpuid_0x10_1_eax eax;
 	union cpuid_0x10_x_edx edx;
@@ -170,8 +177,16 @@ static void rdt_get_cache_config(int idx, struct rdt_resource *r)
 	r->cbm_len = eax.split.cbm_len + 1;
 	r->no_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
 	rdt_get_cache_infofile(r);
+	r->validate = cbm_validate;
+
+	r->display_str = kstrdup("%d=0x%x", GFP_KERNEL);
+	if (!r->display_str)
+		return -ENOMEM;
+
 	r->capable = true;
 	r->enabled = true;
+
+	return 0;
 }
 
 static void rdt_get_cdp_l3_config(int type)
@@ -190,15 +205,15 @@ static void rdt_get_cdp_l3_config(int type)
 	r->enabled = false;
 }
 
-static inline bool get_rdt_resources(void)
+static inline int get_rdt_resources(void)
 {
-	bool ret = false;
+	int ret = 0;
 
 	if (cache_alloc_hsw_probe())
-		return true;
+		return ret;
 
 	if (!boot_cpu_has(X86_FEATURE_RDT_A))
-		return false;
+		return -ENODEV;
 
 	if (boot_cpu_has(X86_FEATURE_CAT_L3)) {
 		rdt_get_cache_config(1, &rdt_resources_all[RDT_RESOURCE_L3]);
@@ -210,13 +225,11 @@ static inline bool get_rdt_resources(void)
 	}
 	if (boot_cpu_has(X86_FEATURE_CAT_L2)) {
 		/* CPUID 0x10.2 fields are same format at 0x10.1 */
-		rdt_get_cache_config(2, &rdt_resources_all[RDT_RESOURCE_L2]);
-		ret = true;
+		ret = rdt_get_cache_config(2, &rdt_resources_all[RDT_RESOURCE_L2]);
 	}
 
 	if (boot_cpu_has(X86_FEATURE_MBA)) {
-		rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
-		ret = true;
+		ret = rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
 	}
 
 	return ret;
@@ -431,7 +444,7 @@ static int __init intel_rdt_late_init(void)
 	struct rdt_resource *r;
 	int state, ret;
 
-	if (!get_rdt_resources())
+	if (get_rdt_resources())
 		return -ENODEV;
 
 	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
index 054d771..f2205d1 100644
--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -29,46 +29,75 @@
 #include <asm/intel_rdt.h>
 
 /*
+ * Check whether MBE 'throttle by' value is correct.
+ *	As per the SDM, when the scale is linear the
+ *	throttle_by granularity is '100 - max_thrtl_by'
+ *	and when its non-linear it is 'power of 2'.
+ */
+int thrtl_validate(char *buf, unsigned long *data, struct rdt_resource *r)
+{
+	u32 delay;
+	int ret;
+
+	ret = kstrtoul(buf, 10, data);
+	if (ret)
+		return ret;
+
+	delay = *data;
+	if (delay > r->max_delay ||
+	     (r->delay_linear && (delay % r->delay_gran)) ||
+	     (!r->delay_linear && !is_power_of_2(delay)))
+		return -EINVAL;
+
+	return 0;
+}
+
+/*
  * Check whether a cache bit mask is valid. The SDM says:
  *	Please note that all (and only) contiguous '1' combinations
  *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).
  * Additionally Haswell requires at least two bits set.
  */
-static bool cbm_validate(unsigned long var, struct rdt_resource *r)
+int cbm_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 {
-	unsigned long first_bit, zero_bit;
+	unsigned long first_bit, zero_bit, var;
+	int ret;
+
+	ret = kstrtoul(buf, 16, &var);
+	if (ret)
+		return ret;
 
 	if (var == 0 || var > r->no_ctrl)
-		return false;
+		return -EINVAL;
 
 	first_bit = find_first_bit(&var, r->cbm_len);
 	zero_bit = find_next_zero_bit(&var, r->cbm_len, first_bit);
 
 	if (find_next_bit(&var, r->cbm_len, zero_bit) < r->cbm_len)
-		return false;
+		return -EINVAL;
 
 	if ((zero_bit - first_bit) < r->min_cbm_bits)
-		return false;
-	return true;
+		return -EINVAL;
+
+	*data = var;
+
+	return 0;
 }
 
 /*
- * Read one cache bit mask (hex). Check that it is valid for the current
- * resource type.
+ * Read the user RDT control value into tempory buffer:
+ * Cache bit mask (hex) or Memory b/w throttle (decimal).
+ * Check that it is valid for the current resource type.
  */
-static int parse_cbm(char *buf, struct rdt_resource *r)
+static int parse_ctrls(char *buf, struct rdt_resource *r)
 {
 	unsigned long data;
-	int ret;
+	int ret = 0;
 
-	ret = kstrtoul(buf, 16, &data);
-	if (ret)
-		return ret;
-	if (!cbm_validate(data, r))
-		return -EINVAL;
+	ret = r->validate(buf, &data, r);
 	r->tmp_ctrl[r->num_tmp_ctrl++] = data;
 
-	return 0;
+	return ret;
 }
 
 /*
@@ -90,7 +119,7 @@ static int parse_line(char *line, struct rdt_resource *r)
 		id = strsep(&dom, "=");
 		if (kstrtoul(id, 10, &dom_id) || dom_id != d->id)
 			return -EINVAL;
-		if (parse_cbm(dom, r))
+		if (parse_ctrls(dom, r))
 			return -EINVAL;
 	}
 
@@ -219,7 +248,8 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
 	list_for_each_entry(dom, &r->domains, list) {
 		if (sep)
 			seq_puts(s, ";");
-		seq_printf(s, "%d=%x", dom->id, dom->ctrl_val[closid]);
+
+		seq_printf(s, r->display_str, dom->id, dom->ctrl_val[closid]);
 		sep = true;
 	}
 	seq_puts(s, "\n");
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1559860 — Re: [PATCH 7/8] x86/intel_rdt/mba: Add schemata file support for MBA

On Tue, 10 Jan 2017, Vikas Shivappa wrote:
> + * @display_str:		Format string to show schemata
> + * @validate:			API to validate the ctrl values.
>   * @info_files:		resctrl info files for the resource
>   * @infofiles_len:		Number of info files
>   * @max_delay:		Max throttle delay
> @@ -99,6 +101,9 @@ struct rdt_resource {
>  	int			cbm_len;
>  	int			min_cbm_bits;
>  	u32			no_ctrl;
> +	char			*display_str;
> +	int (*validate)		(char *buf, unsigned long *data,
> +				  struct rdt_resource *r);

Again this display and validation change wants to be seperate from the
bandwidth stuff.

It's not rocket science to split patches into preparatory and
implementation parts.

> +	r->display_str = kstrdup("%d=%d", GFP_KERNEL);
> +	if (!r->display_str)
> +		return -ENOMEM;

And the point of this allocation is? To consume extra memory for a constant
string which is in const data anyway.

       r->display_str = "%d=%d";

does not need allcotion and consumes exactly the same amount of const data
as the above. Oh well...

> -static inline bool get_rdt_resources(void)
> +static inline int get_rdt_resources(void)
>  {

And the point of this change is? Lots of churn to return the same -ENODEV
value at the call site. So why are you trying to return other values
instead of the simple boolean success/fail decision?

>  /*
> + * Check whether MBE 'throttle by' value is correct.
> + *	As per the SDM, when the scale is linear the
> + *	throttle_by granularity is '100 - max_thrtl_by'
> + *	and when its non-linear it is 'power of 2'.

That's wrong. We really want to let the user set a bandwidth percentage
value from 0 - 100 %. And then adjust it to the proper value which the
hardware can provide. So the user value is independent from granularity,
linear and the max throttling allowed.

>  /*
> - * Read one cache bit mask (hex). Check that it is valid for the current
> - * resource type.
> + * Read the user RDT control value into tempory buffer:
> + * Cache bit mask (hex) or Memory b/w throttle (decimal).
> + * Check that it is valid for the current resource type.
>   */
> -static int parse_cbm(char *buf, struct rdt_resource *r)
> +static int parse_ctrls(char *buf, struct rdt_resource *r)
>  {
>  	unsigned long data;
> -	int ret;
> +	int ret = 0;

What's the purpose of initializing ret to 0 if the next action is assigning
ret the return value of the validate function?

> -	ret = kstrtoul(buf, 16, &data);
> -	if (ret)
> -		return ret;
> -	if (!cbm_validate(data, r))
> -		return -EINVAL;
> +	ret = r->validate(buf, &data, r);
>  	r->tmp_ctrl[r->num_tmp_ctrl++] = data;
>  
> -	return 0;
> +	return ret;
>  }

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1555866 — [PATCH 5/8] x86/intel_rct/mba: Add MBA structures and initialize MBA

The MBA feature details are obtained via executing CPUID with EAX=10H
ECX= 3 and initialize the MBA structures from this info.

Add a new rdt resource 'MBA' to the global list of RDT resources.  Add
extensions to the generic RDT resource structure to store the MBA
specific feature details.  Parameters specific to delay values and delay
granularity are added to the RDT resource and domain structure.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/include/asm/intel_rdt.h         | 10 +++++
 arch/x86/kernel/cpu/intel_rdt.c          | 70 ++++++++++++++++++++++++++------
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c |  2 +-
 3 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 35e76b4..49ae832 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -11,6 +11,9 @@
 #define IA32_L3_QOS_CFG		0xc81
 #define IA32_L3_CBM_BASE	0xc90
 #define IA32_L2_CBM_BASE	0xd10
+#define IA32_MBE_THRTL_BASE	0xd50
+#define MAX_MBA_THRTL		100u
+#define MBE_IS_LINEAR		0x4
 
 #define L3_QOS_CDP_ENABLE	0x01ULL
 
@@ -74,6 +77,9 @@ struct rftype {
  * @no_ctrl:			Specifies max cache cbm or min mem b/w delay.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
+ * @max_delay:		Max throttle delay
+ * @delay_gran:		Throttle delay granularity
+ * @delay_linear:		true if delay is in linear scale
  * @domains:			All domains for this resource
  * @num_domains:		Number of domains active
  * @msr_base:			Base MSR address for CBMs
@@ -92,6 +98,9 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			no_ctrl;
+	u32			max_delay;
+	u32			delay_gran;
+	u32			delay_linear;
 	struct list_head	domains;
 	int			num_domains;
 	int			msr_base;
@@ -141,6 +150,7 @@ enum {
 	RDT_RESOURCE_L3DATA,
 	RDT_RESOURCE_L3CODE,
 	RDT_RESOURCE_L2,
+	RDT_RESOURCE_MBA,
 
 	/* Must be the last */
 	RDT_NUM_RESOURCES,
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index fced83c..6736e1d 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -76,6 +76,14 @@ struct rdt_resource rdt_resources_all[] = {
 		.cbm_idx_multi	= 1,
 		.cbm_idx_offset	= 0
 	},
+	{
+		.name		= "MB",
+		.domains	= domain_init(RDT_RESOURCE_MBA),
+		.msr_base	= IA32_MBE_THRTL_BASE,
+		.cache_level	= 3,
+		.cbm_idx_multi	= 1,
+		.cbm_idx_offset = 0
+	},
 };
 
 static int cbm_idx(struct rdt_resource *r, int closid)
@@ -130,6 +138,26 @@ static inline bool cache_alloc_hsw_probe(void)
 	return false;
 }
 
+static void rdt_get_mem_config(struct rdt_resource *r)
+{
+	union cpuid_0x10_3_eax eax;
+	union cpuid_0x10_x_edx edx;
+	u32 ebx, ecx;
+
+	cpuid_count(0x00000010, 3, &eax.full, &ebx, &ecx, &edx.full);
+	r->num_closid = edx.split.cos_max + 1;
+	r->max_delay = eax.split.max_delay + 1;
+	r->no_ctrl = 0;
+	if (ecx & MBE_IS_LINEAR)
+		r->delay_linear = true;
+
+	if (r->delay_linear)
+		r->delay_gran = MAX_MBA_THRTL - r->max_delay;
+
+	r->capable = true;
+	r->enabled = true;
+}
+
 static void rdt_get_cache_config(int idx, struct rdt_resource *r)
 {
 	union cpuid_0x10_1_eax eax;
@@ -185,6 +213,7 @@ static inline bool get_rdt_resources(void)
 	}
 
 	if (boot_cpu_has(X86_FEATURE_MBA)) {
+		rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
 		ret = true;
 	}
 
@@ -262,6 +291,32 @@ static struct rdt_domain *rdt_find_domain(struct rdt_resource *r, int id,
 	return NULL;
 }
 
+static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)
+{
+	int i;
+
+	d->ctrl_val = kmalloc_array(r->num_closid,
+				     sizeof(*d->ctrl_val), GFP_KERNEL);
+	if (!d->ctrl_val) {
+		kfree(d);
+		return -ENOMEM;
+	}
+
+	/*
+	 * Initialize the Control MSRs to having no control.
+	 * For Cache Allocation: Set all bits in cbm
+	 * For Memory Allocation: Set throttle_by to zero.
+	 */
+	for (i = 0; i < r->num_closid; i++) {
+		int idx = cbm_idx(r, i);
+
+		d->ctrl_val[i] = r->no_ctrl;
+		wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
+	}
+
+	return 0;
+}
+
 /*
  * domain_add_cpu - Add a cpu to a resource's domain list.
  *
@@ -277,7 +332,7 @@ static struct rdt_domain *rdt_find_domain(struct rdt_resource *r, int id,
  */
 static void domain_add_cpu(int cpu, struct rdt_resource *r)
 {
-	int i, id = get_cache_id(cpu, r->cache_level);
+	int id = get_cache_id(cpu, r->cache_level), ret;
 	struct list_head *add_pos = NULL;
 	struct rdt_domain *d;
 
@@ -298,18 +353,9 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
 
 	d->id = id;
 
-	d->ctrl_val = kmalloc_array(r->num_closid, sizeof(*d->ctrl_val), GFP_KERNEL);
-	if (!d->ctrl_val) {
-		kfree(d);
+	ret = domain_setup_ctrlval(r, d);
+	if (ret)
 		return;
-	}
-
-	for (i = 0; i < r->num_closid; i++) {
-		int idx = cbm_idx(r, i);
-
-		d->ctrl_val[i] = r->no_ctrl;
-		wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
-	}
 
 	cpumask_set_cpu(cpu, &d->cpu_mask);
 	list_add_tail(&d->list, add_pos);
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index edc6195..53f1917 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -503,7 +503,7 @@ static int rdt_no_ctrl_show(struct kernfs_open_file *of,
 {
 	struct rdt_resource *r = of->kn->parent->priv;
 
-	seq_printf(seq, "%x\n", r->no_ctrl);
+	seq_printf(seq, "0x%x\n", r->no_ctrl);
 
 	return 0;
 }
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1559763 — Re: [PATCH 5/8] x86/intel_rct/mba: Add MBA structures and initialize MBA

On Tue, 10 Jan 2017, Vikas Shivappa wrote:
> +static void rdt_get_mem_config(struct rdt_resource *r)
> +{
> +	union cpuid_0x10_3_eax eax;
> +	union cpuid_0x10_x_edx edx;
> +	u32 ebx, ecx;
> +
> +	cpuid_count(0x00000010, 3, &eax.full, &ebx, &ecx, &edx.full);
> +	r->num_closid = edx.split.cos_max + 1;
> +	r->max_delay = eax.split.max_delay + 1;
> +	r->no_ctrl = 0;
> +	if (ecx & MBE_IS_LINEAR)
> +		r->delay_linear = true;
> +
> +	if (r->delay_linear)
> +		r->delay_gran = MAX_MBA_THRTL - r->max_delay;

What's the point of this extra conditional?

	if (ecx & MBE_IS_LINEAR) {
		r->delay_linear = true;
		r->delay_gran = MAX_MBA_THRTL - r->max_delay;
	}	

would be too obvious and easy to understand, right?

> +static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)
> +{
> +	int i;
> +
> +	d->ctrl_val = kmalloc_array(r->num_closid,
> +				     sizeof(*d->ctrl_val), GFP_KERNEL);
> +	if (!d->ctrl_val) {
> +		kfree(d);

Freeing memory in the error path of some other random function is just wrong.

	if (!d->ctrl_val)
		return -ENOMEM;

and deal with the fallout at the calling function.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1555869 — [PATCH 8/8] x86/intel_rdt: rmdir,umount and hotcpu updates for MBA

During rmdir only reset the ctrl values for the rdtgroup's closid. This
is done so that that next time when the closid is reused they dont
reflect old values.

Remove the closid update during cpuonline in cqm as its
already in the CAT code. Since both cqm and CAT want the rmid and closid
to be zero when cpu is online, remove the PQR MSR write to zero during
cpuonline because the MSRs are at zero after cpu reset and also during
the first sched in they are updated.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/events/intel/cqm.c              |  1 -
 arch/x86/kernel/cpu/intel_rdt.c          |  1 -
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 18 +++++++++++++-----
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 8c00dc0..baf7ade 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -1573,7 +1573,6 @@ static int intel_cqm_cpu_starting(unsigned int cpu)
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 
 	state->rmid = 0;
-	state->closid = 0;
 	state->rmid_usecnt = 0;
 
 	WARN_ON(c->x86_cache_max_rmid != cqm_max_rmid);
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 9b0a00e..0c25227 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -403,7 +403,6 @@ static void clear_closid(int cpu)
 
 	per_cpu(cpu_closid, cpu) = 0;
 	state->closid = 0;
-	wrmsr(MSR_IA32_PQR_ASSOC, state->rmid, 0);
 }
 
 static int intel_rdt_online_cpu(unsigned int cpu)
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 9d9b7f4..97fc129 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -856,7 +856,7 @@ static struct dentry *rdt_mount(struct file_system_type *fs_type,
 	return dentry;
 }
 
-static int reset_all_cbms(struct rdt_resource *r)
+static int reset_all_ctrls(struct rdt_resource *r, u32 sclosid, u32 eclosid)
 {
 	struct msr_param msr_param;
 	cpumask_var_t cpu_mask;
@@ -867,8 +867,8 @@ static int reset_all_cbms(struct rdt_resource *r)
 		return -ENOMEM;
 
 	msr_param.res = r;
-	msr_param.low = 0;
-	msr_param.high = r->num_closid;
+	msr_param.low = sclosid;
+	msr_param.high = eclosid;
 
 	/*
 	 * Disable resource control for this resource by setting all
@@ -878,7 +878,7 @@ static int reset_all_cbms(struct rdt_resource *r)
 	list_for_each_entry(d, &r->domains, list) {
 		cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
 
-		for (i = 0; i < r->num_closid; i++)
+		for (i = sclosid; i < eclosid; i++)
 			d->ctrl_val[i] = r->no_ctrl;
 	}
 	cpu = get_cpu();
@@ -972,7 +972,7 @@ static void rdt_kill_sb(struct super_block *sb)
 
 	/*Put everything back to default values. */
 	for_each_enabled_rdt_resource(r)
-		reset_all_cbms(r);
+		reset_all_ctrls(r, 0, r->num_closid);
 	cdp_disable();
 	rmdir_all_sub();
 	static_branch_disable(&rdt_enable_key);
@@ -1067,6 +1067,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
 {
 	int ret, cpu, closid = rdtgroup_default.closid;
 	struct rdtgroup *rdtgrp;
+	struct rdt_resource *r;
 	cpumask_var_t tmpmask;
 
 	if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL))
@@ -1095,6 +1096,13 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
 	cpumask_or(tmpmask, tmpmask, &rdtgrp->cpu_mask);
 	rdt_update_closid(tmpmask, NULL);
 
+	/*
+	 * Put domain control values back to default for the
+	 * rdtgrp thats being removed.
+	 */
+	for_each_enabled_rdt_resource(r)
+		reset_all_ctrls(r, rdtgrp->closid, rdtgrp->closid + 1);
+
 	rdtgrp->flags = RDT_DELETED;
 	closid_free(rdtgrp->closid);
 	list_del(&rdtgrp->rdtgroup_list);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1559873 — Re: [PATCH 8/8] x86/intel_rdt: rmdir,umount and hotcpu updates for MBA

On Tue, 10 Jan 2017, Vikas Shivappa wrote:

> During rmdir only reset the ctrl values for the rdtgroup's closid. This
> is done so that that next time when the closid is reused they dont
> reflect old values.

How on earth is that related to MBA?

> Remove the closid update during cpuonline in cqm as its
> already in the CAT code. Since both cqm and CAT want the rmid and closid
> to be zero when cpu is online, remove the PQR MSR write to zero during
> cpuonline because the MSRs are at zero after cpu reset and also during

Are you sure that the MSRs are zero when the cpu goes offline and then
online again? That's not a reset.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web