x86/intel_rdt: Intel Cache Allocation Technology

+static inline bool cat_supported(struct cpuinfo_x86 *c)

Quoted 1 line · Vikas Shivappa

+static inline bool cat_supported(struct cpuinfo_x86 *c)

Is search and replace really that hard?

arch/x86/kernel/cpu/intel_rdt.c line 47

Quoted 3 lines

arch/x86/kernel/cpu/intel_rdt.c +3 -0

+/*
+* Called with the rdt_group_mutex held.
+*/

Whitespace damaged and pointless comment.

arch/x86/kernel/cpu/intel_rdt.c line 50

Quoted 3 lines

arch/x86/kernel/cpu/intel_rdt.c +3 -0

+static int rdt_free_closid(struct intel_rdt *ir)
+{
+

superfluous whitespace

arch/x86/kernel/cpu/intel_rdt.c line 59

Quoted 9 lines

arch/x86/kernel/cpu/intel_rdt.c +9 -0

+	lockdep_assert_held(&rdt_group_mutex);
+
+	WARN_ON(!ccmap[ir->clos].cgrp_count);
+	ccmap[ir->clos].cgrp_count--;
+	if (!ccmap[ir->clos].cgrp_count)
+		clear_bit(ir->clos, rdtss_info.closmap);
+
+	return 0;
+}

These patches really are very sloppy..

arch/x86/kernel/cpu/intel_rdt.c +13 -0

+
+	if (!cbm_is_contiguous(cbmvalue)) {
+		pr_err("bitmask should have >= 1 bits and be contiguous\n");
+		return -EINVAL;
+	}
+
+	par = parent_rdt(ir);
+	cbm_tmp = &ccmap[par->clos].cache_mask;
+	if (!bitmap_subset(&cbmvalue, cbm_tmp, MAX_CBM_LENGTH))
+		return -EINVAL;
+
+	rcu_read_lock();
+	rdt_for_each_child(css, ir) {

arch/x86/kernel/cpu/intel_rdt.c +4 -0

+	return 0;
+}
+
+static bool cbm_search(unsigned long cbm, int *closid)

arch/x86/kernel/cpu/intel_rdt.c +8 -0

+}
+
+static void __cpu_cbm_update(void *info)
+{
+	unsigned int closid = *((unsigned int *)info);
+
+	wrmsrl(CBM_FROM_INDEX(closid), ccmap[closid].cache_mask);
+}

arch/x86/kernel/cpu/intel_rdt.c +31 -0

+
+	if (ir == &rdt_root_group)
+		return -EPERM;
+	bitmap_set(&max_mask, 0, max_cbm);
+
+	/*
+	 * Need global mutex as cbm write may allocate a closid.
+	 */
+	mutex_lock(&rdt_group_mutex);
+	bitmap_and(&cache_mask, (unsigned long *)&cbmvalue, &max_mask, max_cbm);
+	cbm_tmp = &ccmap[ir->clos].cache_mask;
+
+	if (bitmap_equal(&cache_mask, cbm_tmp, MAX_CBM_LENGTH))
+		goto out;
+
+	err = validate_cbm(ir, cache_mask);
+	if (err)
+		goto out;
+
+	/*
+	 * At this point we are sure to change the cache_mask.Hence release the
+	 * reference to the current CLOSid and try to get a reference for
+	 * a different CLOSid.
+	 */
+	__clos_put(ir->clos);
+
+	if (cbm_search(cache_mask, &closid)) {
+		ir->clos = closid;
+		__clos_get(closid);
+	} else {
+		err = rdt_alloc_closid(ir);

arch/x86/kernel/cpu/intel_rdt.c +6 -0

+	mutex_unlock(&rdt_group_mutex);
+	return err;
+}
+
+static inline bool rdt_update_cpumask(int cpu)
+{

arch/x86/kernel/cpu/intel_rdt.c +14 -0

+	return true;
+}
+
+/*
+ * rdt_cpu_start() - If a new package has come up, update all
+ * the Cache bitmasks on the package.
+ */
+static inline void rdt_cpu_start(int cpu)
+{
+	mutex_lock(&rdt_group_mutex);
+	if (rdt_update_cpumask(cpu))
+		cbm_update_msrs(cpu);
+	mutex_unlock(&rdt_group_mutex);
+}

There's CAT in your subject, make up your minds already on what you want to call this stuff.

arch/x86/kernel/cpu/intel_rdt.c static inline bool cat_supported(struct cpuinfo_x86 *c) · line 87

Quoted 3 lines

arch/x86/kernel/cpu/intel_rdt.c +3 -0

+static void rdt_free_closid(unsigned int clos)
+{
+

superfluous whitespace

arch/x86/kernel/cpu/intel_rdt.c static inline bool cat_supported(struct cpuinfo_x86 *c)

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt.c +8 -0

+	lockdep_assert_held(&rdt_group_mutex);
+
+	clear_bit(clos, rdtss_info.closmap);
+}
 
+static inline bool cbm_is_contiguous(unsigned long var)
+{
+	unsigned long first_bit, zero_bit;
+	unsigned long maxcbm = MAX_CBM_LENGTH;

flip these two lines

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 157

Quoted 5 lines

arch/x86/kernel/cpu/intel_rdt.c +5 -0

+
+	if (!var)
+		return false;
+
+	first_bit = find_next_bit(&var, maxcbm, 0);

What was wrong with find_first_bit() ?

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 170

Quoted 13 lines

arch/x86/kernel/cpu/intel_rdt.c +13 -0

+	zero_bit = find_next_zero_bit(&var, maxcbm, first_bit);
+
+	if (find_next_bit(&var, maxcbm, zero_bit) < maxcbm)
+		return false;
+
+	return true;
+}
+
+static int cat_cbm_read(struct seq_file *m, void *v)
+{
+	struct intel_rdt *ir = css_rdt(seq_css(m));
+
+	seq_printf(m, "%08lx\n", ccmap[ir->clos].cache_mask);

inconsistent spacing, you mostly have a whilespace before the return statement, but here you have not.

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 178

Quoted 8 lines

arch/x86/kernel/cpu/intel_rdt.c +8 -0

+	return 0;
+}
+
+static int validate_cbm(struct intel_rdt *ir, unsigned long cbmvalue)
+{
+	struct intel_rdt *par, *c;
+	struct cgroup_subsys_state *css;
+	unsigned long *cbm_tmp;

No reason no to order these on line length is there?

Quoted 13 lines

arch/x86/kernel/cpu/intel_rdt.c +13 -0

+
+	if (!cbm_is_contiguous(cbmvalue)) {
+		pr_err("bitmask should have >= 1 bits and be contiguous\n");
+		return -EINVAL;
+	}
+
+	par = parent_rdt(ir);
+	cbm_tmp = &ccmap[par->clos].cache_mask;
+	if (!bitmap_subset(&cbmvalue, cbm_tmp, MAX_CBM_LENGTH))
+		return -EINVAL;
+
+	rcu_read_lock();
+	rdt_for_each_child(css, ir) {

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 201

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt.c +10 -0

+		c = css_rdt(css);
+		cbm_tmp = &ccmap[c->clos].cache_mask;
+		if (!bitmap_subset(cbm_tmp, &cbmvalue, MAX_CBM_LENGTH)) {
+			rcu_read_unlock();
+			pr_err("Children's mask not a subset\n");
+			return -EINVAL;
+		}
+	}
+
+	rcu_read_unlock();

Daft whitespace again.

Quoted 4 lines

arch/x86/kernel/cpu/intel_rdt.c +4 -0

+	return 0;
+}
+
+static bool cbm_search(unsigned long cbm, int *closid)

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 215

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt.c +10 -0

+{
+	int maxid = boot_cpu_data.x86_cat_closs;
+	unsigned int i;
+
+	for (i = 0; i < maxid; i++) {
+		if (bitmap_equal(&cbm, &ccmap[i].cache_mask, MAX_CBM_LENGTH)) {
+			*closid = i;
+			return true;
+		}
+	}

and again

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 226

Quoted 11 lines

arch/x86/kernel/cpu/intel_rdt.c +11 -0

+	return false;
+}
+
+static void cbmmap_dump(void)
+{
+	int i;
+
+	pr_debug("CBMMAP\n");
+	for (i = 0; i < boot_cpu_data.x86_cat_closs; i++)
+		pr_debug("cache_mask: 0x%x,clos_refcnt: %u\n",
+		 (unsigned int)ccmap[i].cache_mask, ccmap[i].clos_refcnt);

This is missing {}

Quoted 8 lines

arch/x86/kernel/cpu/intel_rdt.c +8 -0

+}
+
+static void __cpu_cbm_update(void *info)
+{
+	unsigned int closid = *((unsigned int *)info);
+
+	wrmsrl(CBM_FROM_INDEX(closid), ccmap[closid].cache_mask);
+}

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 281

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt.c +9 -0

 
+static int cat_cbm_write(struct cgroup_subsys_state *css,
+				 struct cftype *cft, u64 cbmvalue)
+{
+	struct intel_rdt *ir = css_rdt(css);
+	ssize_t err = 0;
+	unsigned long cache_mask, max_mask;
+	unsigned long *cbm_tmp;
+	unsigned int closid;
+	u32 max_cbm = boot_cpu_data.x86_cat_cbmlength;

That's just a right mess isn't it?

Quoted 31 lines

arch/x86/kernel/cpu/intel_rdt.c +31 -0

+
+	if (ir == &rdt_root_group)
+		return -EPERM;
+	bitmap_set(&max_mask, 0, max_cbm);
+
+	/*
+	 * Need global mutex as cbm write may allocate a closid.
+	 */
+	mutex_lock(&rdt_group_mutex);
+	bitmap_and(&cache_mask, (unsigned long *)&cbmvalue, &max_mask, max_cbm);
+	cbm_tmp = &ccmap[ir->clos].cache_mask;
+
+	if (bitmap_equal(&cache_mask, cbm_tmp, MAX_CBM_LENGTH))
+		goto out;
+
+	err = validate_cbm(ir, cache_mask);
+	if (err)
+		goto out;
+
+	/*
+	 * At this point we are sure to change the cache_mask.Hence release the
+	 * reference to the current CLOSid and try to get a reference for
+	 * a different CLOSid.
+	 */
+	__clos_put(ir->clos);
+
+	if (cbm_search(cache_mask, &closid)) {
+		ir->clos = closid;
+		__clos_get(closid);
+	} else {
+		err = rdt_alloc_closid(ir);

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 322

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt.c +10 -0

+		if (err)
+			goto out;
+
+		ccmap[ir->clos].cache_mask = cache_mask;
+		cbm_update_all(ir->clos);
+	}
+
+	cbmmap_dump();
+out:
+

Daft whitespace again.. Also inconsistent return paradigm, here you use an out label, where in validate_cbm() you did rcu_read_unlock() and return from the middle.

Quoted 6 lines

arch/x86/kernel/cpu/intel_rdt.c +6 -0

+	mutex_unlock(&rdt_group_mutex);
+	return err;
+}
+
+static inline bool rdt_update_cpumask(int cpu)
+{

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 338

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt.c +10 -0

+	int phys_id = topology_physical_package_id(cpu);
+	struct cpumask *mask = &rdt_cpumask;
+	int i;
+
+	for_each_cpu(i, mask) {
+		if (phys_id == topology_physical_package_id(i))
+			return false;
+	}
+
+	cpumask_set_cpu(cpu, mask);

More daft whitespace

Quoted 14 lines

arch/x86/kernel/cpu/intel_rdt.c +14 -0

+	return true;
+}
+
+/*
+ * rdt_cpu_start() - If a new package has come up, update all
+ * the Cache bitmasks on the package.
+ */
+static inline void rdt_cpu_start(int cpu)
+{
+	mutex_lock(&rdt_group_mutex);
+	if (rdt_update_cpumask(cpu))
+		cbm_update_msrs(cpu);
+	mutex_unlock(&rdt_group_mutex);
+}

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 362

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt.c +10 -0

+
+static void rdt_cpu_exit(unsigned int cpu)
+{
+	int phys_id = topology_physical_package_id(cpu);
+	int i;
+
+	mutex_lock(&rdt_group_mutex);
+	if (!cpumask_test_and_clear_cpu(cpu, &rdt_cpumask)) {
+		mutex_unlock(&rdt_group_mutex);
+		return;

And here we return from the middle again..

arch/x86/kernel/cpu/intel_rdt.c rdt_css_alloc(struct cgroup_subsys_state *parent_css) · line 375

Quoted 13 lines

arch/x86/kernel/cpu/intel_rdt.c +13 -0

+	}
+
+	for_each_online_cpu(i) {
+		if (i == cpu)
+			continue;
+
+		if (phys_id == topology_physical_package_id(i)) {
+			cpumask_set_cpu(i, &rdt_cpumask);
+			break;
+		}
+	}
+	mutex_unlock(&rdt_group_mutex);
+}

/me tired and gives up..

Quoted 3 lines · Peter Zijlstra

There's CAT in your subject, make up your minds already on what you want to call this stuff.

We dont have control over the names.It is clear from the patch 0/7 where its

explained that RDT is the umbrella term and CAT is a part of it and this patch series is only for CAT ... It also mentions what exact section of the Intel manual this refers to. Is there still some lack of clarification here ?

From what I understand the whole RDT thing is the umbrella term for

If its just your disliking the term thats already known.

But we're not implementing an umbrella right? We're implementing Cache QoS Enforcement (CQE aka. CAT).

Why confuse things with calling it random other names?

From what I understand the whole RDT thing is the umbrella term for Cache QoS Monitoring and Enforcement together. CQM is implemented elsewhere, this part is only implementing CQE.

So just call it that, calling it RDT is actively misleading, because it explicitly does _NOT_ do the monitoring half of it.

Quoted 7 lines · Vikas Shivappa

Quoted 3 lines · Peter Zijlstra

There's CAT in your subject, make up your minds already on what you want to call this stuff.

We dont have control over the names.It is clear from the patch 0/7 where its

If I read 0/n its _after_ I've read all the other patches. The thing is, 0/n should not contain anything persistent. Patches should stand on their own.

Quoted 4 lines

explained that RDT is the umbrella term and CAT is a part of it and this patch series is only for CAT ... It also mentions what exact section of the Intel manual this refers to. Is there still some lack of clarification here ?

But we're not implementing an umbrella right? We're implementing Cache QoS Enforcement (CQE aka. CAT).

Why confuse things with calling it random other names?

From what I understand the whole RDT thing is the umbrella term for Cache QoS Monitoring and Enforcement together. CQM is implemented elsewhere, this part is only implementing CQE.

So just call it that, calling it RDT is actively misleading, because it explicitly does _NOT_ do the monitoring half of it.

Quoted 1 line

If its just your disliking the term thats already known.

I think its crazy to go CQE no CAT no RDT, but I could get over that in time. But now it turns out you need _both_, and that's even more silly.

Will fix the whitespace issues (including before return) or other possible coding convention issues.

It could be more of a common sense to have this in checkpatch rather that manually having to pointing out. If you want to have fun with that go for it though.

Quoted 6 lines · Vikas Shivappa

Will fix the whitespace issues (including before return) or other possible coding convention issues.

It could be more of a common sense to have this in checkpatch rather that manually having to pointing out. If you want to have fun with that go for it though.

My main objection was that your coding style is entirely inconsistent with itself.

Sometimes you have a whitespace before return, sometimes you do not.

Sometimes you have exit labels with locks, sometimes you do not.

etc..

Pick one stick to it; although we'd all much prefer if you pick the one that's common to the kernel.

Adds support for IA32_PQR_ASSOC MSR writes during task scheduling.

The high 32 bits in the per processor MSR IA32_PQR_ASSOC represents the CLOSid. During context switch kernel implements this by writing the CLOSid of the cgroup to which the task belongs to the CPU's IA32_PQR_ASSOC MSR.

For Cache Allocation, this would let the task fill in the cache 'subset' represented by the cgroup's Cache bit mask(CBM).

arch/x86/include/asm/intel_rdt.h +26 -0

+static inline bool rdt_enabled(void)
+{
+	return static_key_false(&rdt_enable_key);
+}
 
+/*
+ * rdt_sched_in() - Writes the task's CLOSid to IA32_PQR_MSR
+ * if the current Closid is different than the new one.
+ */
+static inline void rdt_sched_in(struct task_struct *task)
+{
+	struct intel_rdt *ir;
+	unsigned int clos;
+
+	if (!rdt_enabled())
+		return;
+
+	/*
+	 * This needs to be fixed
+	 * to cache the whole PQR instead of just CLOSid.
+	 * PQR has closid in high 32 bits and CQM-RMID in low 10 bits.
+	 * Should not write a 0 to the low 10 bits of PQR
+	 * and corrupt RMID.
+	 */
+	clos = this_cpu_read(x86_cpu_clos);
+
+	rcu_read_lock();

arch/x86/include/asm/intel_rdt.h +5 -0

+
+#else
+
+static inline void rdt_sched_in(struct task_struct *task) {}
+
 #endif
 #endif

arch/x86/include/asm/switch_to.h +3 -0

index 751bf4b..82ef4b3 100644
@@ -8,6 +8,9 @@ struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		      struct tss_struct *tss);
 
+#include <asm/intel_rdt.h>
+#define post_arch_switch(current)	rdt_sched_in(current)
+
 #ifdef CONFIG_X86_32
 
 #ifdef CONFIG_CC_STACKPROTECTOR

kernel/sched/core.c +0 -0

index f9123a8..cacb490 100644

Quoted 10 lines · Vikas Shivappa

Adds support for IA32_PQR_ASSOC MSR writes during task scheduling.

The high 32 bits in the per processor MSR IA32_PQR_ASSOC represents the CLOSid. During context switch kernel implements this by writing the CLOSid of the cgroup to which the task belongs to the CPU's IA32_PQR_ASSOC MSR.

For Cache Allocation, this would let the task fill in the cache 'subset' represented by the cgroup's Cache bit mask(CBM).

Are you guys for real? Have you even looked at the trainwreck this makes?

Quoted 31 lines

arch/x86/include/asm/intel_rdt.h +30 -0

+static inline bool rdt_enabled(void)
+{
+	return static_key_false(&rdt_enable_key);
+}
 
+/*
+ * rdt_sched_in() - Writes the task's CLOSid to IA32_PQR_MSR
+ * if the current Closid is different than the new one.
+ */
+static inline void rdt_sched_in(struct task_struct *task)
+{
+	struct intel_rdt *ir;
+	unsigned int clos;
+
+	if (!rdt_enabled())
+		return;
+
+	/*
+	 * This needs to be fixed
+	 * to cache the whole PQR instead of just CLOSid.
+	 * PQR has closid in high 32 bits and CQM-RMID in low 10 bits.
+	 * Should not write a 0 to the low 10 bits of PQR
+	 * and corrupt RMID.
+	 */
+	clos = this_cpu_read(x86_cpu_clos);
+
+	rcu_read_lock();
+	ir = task_rdt(task);
+	if (ir->clos == clos) {
+		rcu_read_unlock();
+		return;

arch/x86/include/asm/intel_rdt.h static inline struct intel_rdt *parent_rdt(struct intel_rdt *ir) · line 88

Quoted 6 lines

arch/x86/include/asm/intel_rdt.h +6 -0

+	}
+
+	wrmsr(MSR_IA32_PQR_ASSOC, 0, ir->clos);
+	this_cpu_write(x86_cpu_clos, ir->clos);
+	rcu_read_unlock();
+}

You inject _ALL_ that into the scheduler hot path. Insane much?

Quoted 22 lines

arch/x86/include/asm/intel_rdt.h +5 -0

+
+#else
+
+static inline void rdt_sched_in(struct task_struct *task) {}
+
 #endif
 #endif

arch/x86/include/asm/switch_to.h +3 -0

index 751bf4b..82ef4b3 100644
@@ -8,6 +8,9 @@ struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		      struct tss_struct *tss);
 
+#include <asm/intel_rdt.h>
+#define post_arch_switch(current)	rdt_sched_in(current)
+
 #ifdef CONFIG_X86_32
 
 #ifdef CONFIG_CC_STACKPROTECTOR

kernel/sched/core.c static struct rq *finish_task_switch(struct task_struct *prev) · line 2247

Quoted 12 lines

kernel/sched/core.c +1 -0

index f9123a8..cacb490 100644
@@ -2241,6 +2241,7 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	prev_state = prev->state;
 	vtime_task_switch(prev);
 	finish_arch_switch(prev);
+	post_arch_switch(current);
 	perf_event_task_sched_in(prev, current);
 	finish_lock_switch(rq, prev);
 	finish_arch_post_lock_switch();

Not a word in the Changelog on this hook; that's double fail.

Please _THINK_ before writing code.

Quoted 13 lines

Quoted 12 lines

arch/x86/include/asm/switch_to.h +3 -0

@@ -8,6 +8,9 @@ struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		      struct tss_struct *tss);
 
+#include <asm/intel_rdt.h>
+#define post_arch_switch(current)	rdt_sched_in(current)
+
 #ifdef CONFIG_X86_32
 
 #ifdef CONFIG_CC_STACKPROTECTOR

Quoted 14 lines

kernel/sched/core.c static struct rq *finish_task_switch(struct task_struct *prev) · line 2247

Quoted 12 lines

kernel/sched/core.c +1 -0

index f9123a8..cacb490 100644
@@ -2241,6 +2241,7 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	prev_state = prev->state;
 	vtime_task_switch(prev);
 	finish_arch_switch(prev);
+	post_arch_switch(current);
 	perf_event_task_sched_in(prev, current);
 	finish_lock_switch(rq, prev);
 	finish_arch_post_lock_switch();

Not a word in the Changelog on this hook; that's double fail.

will add the changelog. we want the current task which no other existing hook provides.

Quoted 28 lines · Vikas Shivappa

Quoted 13 lines

Quoted 12 lines

arch/x86/include/asm/switch_to.h +3 -0

@@ -8,6 +8,9 @@ struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		      struct tss_struct *tss);
 
+#include <asm/intel_rdt.h>
+#define post_arch_switch(current)	rdt_sched_in(current)
+
 #ifdef CONFIG_X86_32
 
 #ifdef CONFIG_CC_STACKPROTECTOR

Quoted 14 lines

kernel/sched/core.c static struct rq *finish_task_switch(struct task_struct *prev) · line 2247

Quoted 12 lines

kernel/sched/core.c +1 -0

index f9123a8..cacb490 100644
@@ -2241,6 +2241,7 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	prev_state = prev->state;
 	vtime_task_switch(prev);
 	finish_arch_switch(prev);
+	post_arch_switch(current);
 	perf_event_task_sched_in(prev, current);
 	finish_lock_switch(rq, prev);
 	finish_arch_post_lock_switch();

Not a word in the Changelog on this hook; that's double fail.

Quoted 2 lines

will add the changelog. we want the current task which no other existing hook provides.

No.

1) two arch hooks right after one another is FAIL
1a) just 'fix' the existing hook
2) current is cheap and easily obtainable without passing it as an argument

3) why do you need the hook in the first place?
3a) why can't you put this in __switch_to()? This is very much x86 only
code.

Quoted 6 lines

No.

1) two arch hooks right after one another is FAIL
1a) just 'fix' the existing hook
2) current is cheap and easily obtainable without passing it as an argument

will fix to just use an existing hook in finish_task_switch and current(get_current) since the stack would already be changed ..

Thanks,
Vikas

Quoted 3 lines

3) why do you need the hook in the first place?
3a) why can't you put this in __switch_to()? This is very much x86 only
code.

Quoted 10 lines · Vikas Shivappa

Quoted 6 lines

No.

1) two arch hooks right after one another is FAIL
1a) just 'fix' the existing hook
2) current is cheap and easily obtainable without passing it as an argument

will fix to just use an existing hook in finish_task_switch and current(get_current) since the stack would already be changed ..

Quoted 6 lines

Thanks,
Vikas

Quoted 3 lines

3) why do you need the hook in the first place?
3a) why can't you put this in __switch_to()? This is very much x86 only
code.

^ please also answer 3, why can't this go in __swtich_to()?

Quoted 7 lines · Vikas Shivappa

Adds support for IA32_PQR_ASSOC MSR writes during task scheduling.

The high 32 bits in the per processor MSR IA32_PQR_ASSOC represents the CLOSid. During context switch kernel implements this by writing the CLOSid of the cgroup to which the task belongs to the CPU's IA32_PQR_ASSOC MSR.

Quoted 3 lines

For Cache Allocation, this would let the task fill in the cache 'subset' represented by the cgroup's Cache bit mask(CBM).

Are you guys for real? Have you even looked at the trainwreck this makes?

Quoted 28 lines

arch/x86/include/asm/intel_rdt.h +26 -0

+static inline bool rdt_enabled(void)
+{
+	return static_key_false(&rdt_enable_key);
+}
 
+/*
+ * rdt_sched_in() - Writes the task's CLOSid to IA32_PQR_MSR
+ * if the current Closid is different than the new one.
+ */
+static inline void rdt_sched_in(struct task_struct *task)
+{
+	struct intel_rdt *ir;
+	unsigned int clos;
+
+	if (!rdt_enabled())
+		return;
+
+	/*
+	 * This needs to be fixed
+	 * to cache the whole PQR instead of just CLOSid.
+	 * PQR has closid in high 32 bits and CQM-RMID in low 10 bits.
+	 * Should not write a 0 to the low 10 bits of PQR
+	 * and corrupt RMID.
+	 */
+	clos = this_cpu_read(x86_cpu_clos);
+
+	rcu_read_lock();

arch/x86/include/asm/intel_rdt.h static inline struct intel_rdt *parent_rdt(struct intel_rdt *ir) · line 88

Quoted 10 lines

arch/x86/include/asm/intel_rdt.h +10 -0

+	ir = task_rdt(task);
+	if (ir->clos == clos) {
+		rcu_read_unlock();
+		return;
+	}
+
+	wrmsr(MSR_IA32_PQR_ASSOC, 0, ir->clos);
+	this_cpu_write(x86_cpu_clos, ir->clos);
+	rcu_read_unlock();
+}

You inject _ALL_ that into the scheduler hot path. Insane much?

Quoted 27 lines

arch/x86/include/asm/intel_rdt.h +5 -0

+
+#else
+
+static inline void rdt_sched_in(struct task_struct *task) {}
+
 #endif
 #endif

arch/x86/include/asm/switch_to.h +3 -0

index 751bf4b..82ef4b3 100644
@@ -8,6 +8,9 @@ struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		      struct tss_struct *tss);
 
+#include <asm/intel_rdt.h>
+#define post_arch_switch(current)	rdt_sched_in(current)
+
 #ifdef CONFIG_X86_32
 
 #ifdef CONFIG_CC_STACKPROTECTOR

kernel/sched/core.c +0 -0

index f9123a8..cacb490 100644

kernel/sched/core.c static struct rq *finish_task_switch(struct task_struct *prev) · line 2247

Quoted 7 lines

kernel/sched/core.c +1 -0

@@ -2241,6 +2241,7 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	prev_state = prev->state;
 	vtime_task_switch(prev);
 	finish_arch_switch(prev);
+	post_arch_switch(current);
 	perf_event_task_sched_in(prev, current);
 	finish_lock_switch(rq, prev);
 	finish_arch_post_lock_switch();

Not a word in the Changelog on this hook; that's double fail.

Please _THINK_ before writing code.

However Matt pointed out I could improve this to
if (static_key_false)

	{ rdt_sched_in(); }

instead of a static inline which i will update. Will update the commit message to include these details.