x86/intel_rdt: Intel Cache Allocation support

 arch/x86/include/asm/intel_rdt.h |  38 +++++++++++++
 arch/x86/kernel/cpu/intel_rdt.c  | 112 +++++++++++++++++++++++++++++++++++++--
 include/linux/cgroup_subsys.h    |   4 ++

arch/x86/include/asm/intel_rdt.h new file +14 -0

+	/* Class of service for the cgroup.*/
+	unsigned int clos;
+};
+
+struct clos_cbm_map {
+	unsigned long cache_mask;
+	unsigned int clos_refcnt;
+};
 
+/*
+ * ccmap maintains 1:1 mapping between CLOSid and cache bitmask.
+ */
+static struct clos_cbm_map *ccmap;
+static struct rdt_subsys_info rdtss_info;
+static DEFINE_MUTEX(rdt_group_mutex);

arch/x86/kernel/cpu/intel_rdt.c +28 -2

+	 */
+	if (!parent)
+		return &rdt_root_group.css;
+
+	ir = kzalloc(sizeof(struct intel_rdt), GFP_KERNEL);
+	if (!ir)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&rdt_group_mutex);
+	ir->clos = parent->clos;
+	__clos_get(ir->clos);
+	mutex_unlock(&rdt_group_mutex);
+
+	return &ir->css;
+}
+
+static void intel_rdt_css_free(struct cgroup_subsys_state *css)
+{
+	struct intel_rdt *ir = css_rdt(css);
+
+	mutex_lock(&rdt_group_mutex);
+	__clos_put(ir->clos);
+	kfree(ir);
+	mutex_unlock(&rdt_group_mutex);
+}
 
 static int __init intel_rdt_late_init(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
-	int maxid, max_cbm_len;
+	static struct clos_cbm_map *ccm;
+	int maxid, max_cbm_len, err = 0;
+	size_t sizeb;
 
-	if (!cpu_has(c, X86_FEATURE_CAT_L3))

Quoted 3 lines · Vikas Shivappa

 arch/x86/include/asm/intel_rdt.h |  38 +++++++++++++
 arch/x86/kernel/cpu/intel_rdt.c  | 112 +++++++++++++++++++++++++++++++++++++--
 include/linux/cgroup_subsys.h    |   4 ++

Where is the Documentation for the new cgroup subsystem?

arch/x86/include/asm/intel_rdt.h line 9

Quoted 3 lines

arch/x86/include/asm/intel_rdt.h new file +2 -0

+struct rdt_subsys_info {
+	/* Clos Bitmap to keep track of available CLOSids.*/

If you want to document your struct members, please use KernelDoc formatting, so tools can pick it up as well.

arch/x86/include/asm/intel_rdt.h line 14

Quoted 5 lines

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

+	unsigned long *closmap;
+};
+
+struct intel_rdt {
+	struct cgroup_subsys_state css;

Ditto

Quoted 20 lines

arch/x86/include/asm/intel_rdt.h new file +18 -0

+	/* Class of service for the cgroup.*/
+	unsigned int clos;
+};
+
+struct clos_cbm_map {
+	unsigned long cache_mask;
+	unsigned int clos_refcnt;
+};
 
+/*
+ * ccmap maintains 1:1 mapping between CLOSid and cache bitmask.
+ */
+static struct clos_cbm_map *ccmap;
+static struct rdt_subsys_info rdtss_info;
+static DEFINE_MUTEX(rdt_group_mutex);
+struct intel_rdt rdt_root_group;
+
+static void intel_rdt_free_closid(unsigned int clos)
+{

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

Quoted 6 lines

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

+	lockdep_assert_held(&rdt_group_mutex);
+
+	clear_bit(clos, rdtss_info.closmap);
+}
+
+static void __clos_get(unsigned int closid)

What's the purpose of these double underscores?

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

Quoted 4 lines

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

+{
+	struct clos_cbm_map *ccm = &ccmap[closid];
+
+	lockdep_assert_held(&rdt_group_mutex);

Do we really need all these lockdep asserts for a handfull of call sites?

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

Quoted 2 lines

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

+	ccm->clos_refcnt += 1;

What's wrong with:

      ccmap[closid].clos_refcnt++;

Hmm?

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

Quoted 9 lines

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

+}
+
+static void __clos_put(unsigned int closid)
+{
+	struct clos_cbm_map *ccm = &ccmap[closid];
+
+	lockdep_assert_held(&rdt_group_mutex);
+	WARN_ON(!ccm->clos_refcnt);

So we have a warning but we do not act on it.

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

Quoted 4 lines

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

+
+	ccm->clos_refcnt -= 1;
+	if (!ccm->clos_refcnt)

You can do that in a single line w/o the pointer magic. We want easy to read and small code rather than pointlessly blown up helper functions which just eat screen space.

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

Quoted 13 lines

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

+		intel_rdt_free_closid(closid);
+}
+
+static struct cgroup_subsys_state *
+intel_rdt_css_alloc(struct cgroup_subsys_state *parent_css)
+{
+	struct intel_rdt *parent = css_rdt(parent_css);
+	struct intel_rdt *ir;
+
+	/*
+	 * Cannot return failure on systems with no Cache Allocation
+	 * as the cgroup_init does not handle failures gracefully.

This comment is blatantly wrong. 5 lines further down you return -ENOMEM. Now what?

Quoted 43 lines

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

+	 */
+	if (!parent)
+		return &rdt_root_group.css;
+
+	ir = kzalloc(sizeof(struct intel_rdt), GFP_KERNEL);
+	if (!ir)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&rdt_group_mutex);
+	ir->clos = parent->clos;
+	__clos_get(ir->clos);
+	mutex_unlock(&rdt_group_mutex);
+
+	return &ir->css;
+}
+
+static void intel_rdt_css_free(struct cgroup_subsys_state *css)
+{
+	struct intel_rdt *ir = css_rdt(css);
+
+	mutex_lock(&rdt_group_mutex);
+	__clos_put(ir->clos);
+	kfree(ir);
+	mutex_unlock(&rdt_group_mutex);
+}
 
 static int __init intel_rdt_late_init(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
-	int maxid, max_cbm_len;
+	static struct clos_cbm_map *ccm;
+	int maxid, max_cbm_len, err = 0;
+	size_t sizeb;
 
-	if (!cpu_has(c, X86_FEATURE_CAT_L3))
+	if (!cpu_has(c, X86_FEATURE_CAT_L3)) {
+		rdt_root_group.css.ss->disabled = 1;
 		return -ENODEV;
-
+	}
 	maxid = c->x86_rdt_max_closid;
 	max_cbm_len = c->x86_rdt_max_cbm_len;

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

Quoted 2 lines

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

+	sizeb = BITS_TO_LONGS(maxid) * sizeof(long);
+	rdtss_info.closmap = kzalloc(sizeb, GFP_KERNEL);

What's the point of this bitmap? In later patches you have a restriction to 64bits and the SDM says that CBM_LEN is limited to 32.

So where is the point for allocating a bitmap?

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

Quoted 12 lines

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

+	if (!rdtss_info.closmap) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	sizeb = maxid * sizeof(struct clos_cbm_map);
+	ccmap = kzalloc(sizeb, GFP_KERNEL);
+	if (!ccmap) {
+		kfree(rdtss_info.closmap);
+		err = -ENOMEM;
+		goto out_err;

What's wrong with return -ENOMEM? We only use goto if we have to cleanup stuff, certainly not for just returning err.

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

Quoted 10 lines

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

+	}
+
+	set_bit(0, rdtss_info.closmap);
+	rdt_root_group.clos = 0;
+	ccm = &ccmap[0];
+	bitmap_set(&ccm->cache_mask, 0, max_cbm_len);
+	ccm->clos_refcnt = 1;
+
 	pr_info("Max bitmask length:%u,Max ClosIds: %u\n", max_cbm_len, maxid);

We surely do not want to sprinkle these all over dmesg.

arch/x86/kernel/cpu/intel_rdt.c +2 -1

+out_err:
 
-       return 0;
+       return err;

Thanks,

tglx

Quoted 4 lines

Quoted 1 line

+static void __clos_get(unsigned int closid)

What's the purpose of these double underscores?

Similar to __get_rmid.

Quoted 7 lines

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

Quoted 4 lines

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

+{
+	struct clos_cbm_map *ccm = &ccmap[closid];
+
+	lockdep_assert_held(&rdt_group_mutex);

Do we really need all these lockdep asserts for a handfull of call sites?

Well, we still have some calls some may be frequent depending on the usage.. should the assert decided based on number of times its called ?

Quoted 4 lines

Quoted 4 lines

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

+static struct cgroup_subsys_state *
+intel_rdt_css_alloc(struct cgroup_subsys_state *parent_css)
+{
+	struct intel_rdt *parent = css_rdt(parent_css);

Quoted 9 lines

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

Quoted 5 lines

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

+	struct intel_rdt *ir;
+
+	/*
+	 * Cannot return failure on systems with no Cache Allocation
+	 * as the cgroup_init does not handle failures gracefully.

This comment is blatantly wrong. 5 lines further down you return -ENOMEM. Now what?

Well , this is for cgroup_init called with parent=null which is when its initializing the cgroup subsystem. I cant return error in this case but I can otherwise.

static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early) {

	/* Create the root cgroup state for this subsystem */
	ss->root = &cgrp_dfl_root;
	css = ss->css_alloc(cgroup_css(&cgrp_dfl_root.cgrp, ss));
	/* We don't handle early failures gracefully */
	BUG_ON(IS_ERR(css));

Quoted 7 lines

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

Quoted 7 lines

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

+	 */
+	if (!parent)
+		return &rdt_root_group.css;
+
+	ir = kzalloc(sizeof(struct intel_rdt), GFP_KERNEL);
+	if (!ir)
+		return ERR_PTR(-ENOMEM);

Quoted 10 lines

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

Quoted 5 lines

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

 	maxid = c->x86_rdt_max_closid;
 	max_cbm_len = c->x86_rdt_max_cbm_len;
 
+	sizeb = BITS_TO_LONGS(maxid) * sizeof(long);
+	rdtss_info.closmap = kzalloc(sizeb, GFP_KERNEL);

What's the point of this bitmap? In later patches you have a restriction to 64bits and the SDM says that CBM_LEN is limited to 32.

So where is the point for allocating a bitmap?

The cache bitmask is really a bitmask where every bit represents a cache way , so its way based mapping . Although currently the max is 32 bits we still need to treat it as a bitmask.

In the first patch version you are the one who suggested to use all the bitmap functions to check the presence of contiguous bits (although I was already using the bitmaps, I had not used for some cases). So i made the change and other similar code was changed to using bitmap/bit manipulation APIs. There are scenarios where in we need to check for subset of bitmasks and other cases but agree they can all be done without the bitmask APIs as well. But now your comment contradicts the old comment ?

Quoted 15 lines

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

Quoted 12 lines

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

+	if (!rdtss_info.closmap) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	sizeb = maxid * sizeof(struct clos_cbm_map);
+	ccmap = kzalloc(sizeb, GFP_KERNEL);
+	if (!ccmap) {
+		kfree(rdtss_info.closmap);
+		err = -ENOMEM;
+		goto out_err;

What's wrong with return -ENOMEM? We only use goto if we have to cleanup stuff, certainly not for just returning err.

This was due to PeterZ's feedback that the return paradigm needs to not have too many exit points or return a lot of times from the middle of code.. Both contradict now ?

Quoted 13 lines

Quoted 11 lines

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

+	}
+
+	set_bit(0, rdtss_info.closmap);
+	rdt_root_group.clos = 0;
+	ccm = &ccmap[0];
+	bitmap_set(&ccm->cache_mask, 0, max_cbm_len);
+	ccm->clos_refcnt = 1;
+
 	pr_info("Max bitmask length:%u,Max ClosIds: %u\n", max_cbm_len,

maxid);

We surely do not want to sprinkle these all over dmesg.

This is just printed once! how is that sprinke all over? - we have a dmsg
print for Cache monitoring as well when cqm is enabled.

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

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

+	}
+
+	set_bit(0, rdtss_info.closmap);
+	rdt_root_group.clos = 0;

Quoted 8 lines

Quoted 6 lines

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

+	ccm = &ccmap[0];
+	bitmap_set(&ccm->cache_mask, 0, max_cbm_len);
+	ccm->clos_refcnt = 1;
+
 	pr_info("Max bitmask length:%u,Max ClosIds: %u\n", max_cbm_len,

maxid);

We surely do not want to sprinkle these all over dmesg.

Quoted 2 lines

This is just printed once! how is that sprinke all over? - we have a dmsg
print for Cache monitoring as well when cqm is enabled.

Sorry, mapped that to the wrong function. Though the message itself is horrible.

"Max bitmask length:32,Max ClosIds: 16"

With some taste and formatting applied this would read:

     	  "Max. bitmask length: 32, max. closids: 16"

Can you spot the difference?

Quoted 19 lines · Thomas Gleixner

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

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

+	}
+
+	set_bit(0, rdtss_info.closmap);
+	rdt_root_group.clos = 0;

Quoted 8 lines

Quoted 6 lines

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

+	ccm = &ccmap[0];
+	bitmap_set(&ccm->cache_mask, 0, max_cbm_len);
+	ccm->clos_refcnt = 1;
+
 	pr_info("Max bitmask length:%u,Max ClosIds: %u\n", max_cbm_len,

maxid);

We surely do not want to sprinkle these all over dmesg.

Quoted 2 lines

This is just printed once! how is that sprinke all over? - we have a dmsg
print for Cache monitoring as well when cqm is enabled.

Sorry, mapped that to the wrong function. Though the message itself is horrible.

Quoted 7 lines

"Max bitmask length:32,Max ClosIds: 16"

With some taste and formatting applied this would read:

     	  "Max. bitmask length: 32, max. closids: 16"

Can you spot the difference?

I sure can.

Also, I'd still like to ask about the usability of that message. What does it bring us?

And if the dmesg ring buffer wraps around and it gets overwritten, what do we do then?

So basically this message does tell us about some max bitmap length and so on. IOW, useless, right?

Can it be read out from CPUID maybe? If so, stuff which is interested in it can generate it then.

Signature · 6 lines

-- 
Regards/Gruss,
   Boris.

ECO tip #101: Trim your mails when you reply.
--

Yes the max bitmask and max closid can be read from cpuid. the presence of rdt and l3 cache alloc is indicated by rdt and cat_l3 in /proc/cpuinfo. So even if this message gets overwritten , user can see the feature enabled in kernel.

Can be modified somthing similar to "Intel cache allocation enabled" as well.

Quoted 6 lines · Vikas Shivappa

Quoted 4 lines

Quoted 1 line

+static void __clos_get(unsigned int closid)

What's the purpose of these double underscores?

Similar to __get_rmid.

Errm. We use double underscore for such cases:

__bla()

{
	do_stuff();
}

bla()

{
	lock();
	__bla();
	unlock();
}

So you have two sorts of callers. Locked and unlocked. But I don't see something like this. It's just random underscores for no value.

Quoted 9 lines

Quoted 8 lines

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

Quoted 5 lines

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

+{
+	struct clos_cbm_map *ccm = &ccmap[closid];
+
+	lockdep_assert_held(&rdt_group_mutex);

Do we really need all these lockdep asserts for a handfull of call sites?

Quoted 2 lines

Well, we still have some calls some may be frequent depending on the usage.. should the assert decided based on number of times its called ?

We add these things for complex locking scenarios, but for single callsites where locking is obvious its not really valuable. But that's the least of my worries.

Quoted 15 lines

Quoted 14 lines

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

Quoted 10 lines

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

+static struct cgroup_subsys_state *
+intel_rdt_css_alloc(struct cgroup_subsys_state *parent_css)
+{
+	struct intel_rdt *parent = css_rdt(parent_css);
+	struct intel_rdt *ir;
+
+	/*
+	 * Cannot return failure on systems with no Cache Allocation
+	 * as the cgroup_init does not handle failures gracefully.

This comment is blatantly wrong. 5 lines further down you return -ENOMEM. Now what?

Quoted 3 lines

Well , this is for cgroup_init called with parent=null which is when its initializing the cgroup subsystem. I cant return error in this case but I can otherwise.

And then you run into the same BUG_ON ...

Quoted 9 lines

static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early) {

	/* Create the root cgroup state for this subsystem */
	ss->root = &cgrp_dfl_root;
	css = ss->css_alloc(cgroup_css(&cgrp_dfl_root.cgrp, ss));
	/* We don't handle early failures gracefully */
	BUG_ON(IS_ERR(css));

I know. But still the comment is misleading.

/*
* cgroup_init() expects valid css pointer. Return
* the rdt_root_group.css instead of a failure code. */

Can you see the difference?

Quoted 8 lines

Quoted 8 lines

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

Quoted 8 lines

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

+	 */
+	if (!parent)
+		return &rdt_root_group.css;
+
+	ir = kzalloc(sizeof(struct intel_rdt), GFP_KERNEL);
+	if (!ir)
+		return ERR_PTR(-ENOMEM);

And why can't you return something useful here instead of letting the thing run into a BUG?

Quoted 16 lines

Quoted 11 lines

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

Quoted 6 lines

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

 	maxid = c->x86_rdt_max_closid;
 	max_cbm_len = c->x86_rdt_max_cbm_len;
 
+	sizeb = BITS_TO_LONGS(maxid) * sizeof(long);
+	rdtss_info.closmap = kzalloc(sizeb, GFP_KERNEL);

What's the point of this bitmap? In later patches you have a restriction to 64bits and the SDM says that CBM_LEN is limited to 32.

So where i the point for allocating a bitmap?

The cache bitmask is really a bitmask where every bit represents a cache way , so its way based mapping . Although currently the max is 32 bits we still need to treat it as a bitmask.

Quoted 7 lines

In the first patch version you are the one who suggested to use all the bitmap functions to check the presence of contiguous bits (although I was already using the bitmaps, I had not used for some cases). So i made the change and other similar code was changed to using bitmap/bit manipulation APIs. There are scenarios where in we need to check for subset of bitmasks and other cases but agree they can all be done without the bitmask APIs as well. But now your comment contradicts the old comment ?

Oh well. You can still use bitmap functions on an u64 where appropriate.

Quoted 17 lines

Quoted 16 lines

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

Quoted 13 lines

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

+	if (!rdtss_info.closmap) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	sizeb = maxid * sizeof(struct clos_cbm_map);
+	ccmap = kzalloc(sizeb, GFP_KERNEL);
+	if (!ccmap) {
+		kfree(rdtss_info.closmap);
+		err = -ENOMEM;
+		goto out_err;

What's wrong with return -ENOMEM? We only use goto if we have to cleanup stuff, certainly not for just returning err.

Quoted 3 lines

This was due to PeterZ's feedback that the return paradigm needs to not have too many exit points or return a lot of times from the middle of code.. Both contradict now ?

There are different opinions about that. But again, that's the least of my worries.

Quoted 15 lines

Quoted 14 lines

Quoted 11 lines

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

+	}
+
+	set_bit(0, rdtss_info.closmap);
+	rdt_root_group.clos = 0;
+	ccm = &ccmap[0];
+	bitmap_set(&ccm->cache_mask, 0, max_cbm_len);
+	ccm->clos_refcnt = 1;
+
 	pr_info("Max bitmask length:%u,Max ClosIds: %u\n", max_cbm_len,

Quoted 1 line

maxid);

We surely do not want to sprinkle these all over dmesg.

Quoted 2 lines

This is just printed once! how is that sprinke all over? - we have a dmsg
print for Cache monitoring as well when cqm is enabled.

Sorry, mapped that to the wrong function. Though the message itself is horrible.

"Max bitmask length:32,Max ClosIds: 16"

With some taste and formatting applied this would read:

    	  "Max. bitmask length: 32, max. closids: 16"

Can you spot the difference?

Thanks,

tglx

Quoted 7 lines · Thomas Gleixner

Quoted 4 lines · Vikas Shivappa

 arch/x86/include/asm/intel_rdt.h |  38 +++++++++++++
 arch/x86/kernel/cpu/intel_rdt.c  | 112
+++++++++++++++++++++++++++++++++++++--
 include/linux/cgroup_subsys.h    |   4 ++

Where is the Documentation for the new cgroup subsystem?

Documentation/cgroups/rdt.txt

Quoted 8 lines · Vikas Shivappa

Quoted 6 lines · Thomas Gleixner

Quoted 4 lines · Vikas Shivappa

 arch/x86/include/asm/intel_rdt.h |  38 +++++++++++++
 arch/x86/kernel/cpu/intel_rdt.c  | 112
+++++++++++++++++++++++++++++++++++++--
 include/linux/cgroup_subsys.h    |   4 ++

Quoted 3 lines

Quoted 1 line

Where is the Documentation for the new cgroup subsystem?

Documentation/cgroups/rdt.txt

That's where it should be, but according to the diffstat it is not. It comes as the last patch in the series, but it should be there with the patch which introduces the subsystem or even before that.

And the features which are added in subsequent patches should be reflected with updates to the documentation.

Thanks,

tglx

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

+/*
+ * cbm_update_msrs() - Updates all the existing IA32_L3_MASK_n MSRs
+ * which are one per CLOSid, except IA32_L3_MASK_0 on the current package.
+ * @cpu : the cpu on which the mask is updated.

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

+				 struct cftype *cft, u64 cbmvalue)
+{
+	u32 max_cbm = boot_cpu_data.x86_rdt_max_cbm_len;
+	struct intel_rdt *ir = css_rdt(css);
+	unsigned long cache_mask, max_mask;
+	unsigned long *cbm_tmp;
+	unsigned int closid;
+	ssize_t err = 0;
+
+	if (ir == &rdt_root_group)
+		return -EPERM;
+	bitmap_set(&max_mask, 0, max_cbm);
+

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

+static void intel_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;
+	}

arch/x86/kernel/cpu/intel_rdt.c static struct rdt_subsys_info rdtss_info; · line 40

Quoted 6 lines · Vikas Shivappa

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

@@ -35,6 +36,42 @@ static struct rdt_subsys_info rdtss_info;
 static DEFINE_MUTEX(rdt_group_mutex);
 struct intel_rdt rdt_root_group;
 
+/*
+ * Mask of CPUs for writing CBM values. We only need one per-socket.

One mask? One CPU? One what? Please add comments which are understandable and useful for people who did NOT write that code.

arch/x86/kernel/cpu/intel_rdt.c static struct rdt_subsys_info rdtss_info;

Quoted 7 lines

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

+ */
+static cpumask_t rdt_cpumask;
 
+static void __cpu_cbm_update(void *info)
+{
+	unsigned int closid = *((unsigned int *)info);

So all you want is to transfer closid, right? Why having a pointer instead of just doing

	on_each_cpu_mask(...., (void *) id);

and

	id = (unsigned long) id;

Looks too simple and readable, right?

Quoted 4 lines

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

+/*
+ * cbm_update_msrs() - Updates all the existing IA32_L3_MASK_n MSRs
+ * which are one per CLOSid, except IA32_L3_MASK_0 on the current package.
+ * @cpu : the cpu on which the mask is updated.

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 255

Quoted 10 lines

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

+ */
+static inline void cbm_update_msrs(int cpu)
+{
+	int maxid = boot_cpu_data.x86_rdt_max_closid;
+	unsigned int i;
+
+	if (WARN_ON(cpu != smp_processor_id()))
+		return;
+
+	for (i = 1; i < maxid; i++) {

Lacks a comment why this starts with 1 and not with 0 as one would expect.

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 259

Quoted 5 lines

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

+		if (ccmap[i].clos_refcnt)
+			__cpu_cbm_update(&i);
+	}
+}

This is only used by the cpu hotplug code. So why does it not live close to it?

them just in place?

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 272

Quoted 12 lines

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

+/*
+ * intel_cache_alloc_cbm_write() - Validates and writes the
+ * cache bit mask(cbm) to the IA32_L3_MASK_n
+ * and also store the same in the ccmap.
+ *
+ * CLOSids are reused for cgroups which have same bitmask.
+ * - This helps to use the scant CLOSids optimally.
+ * - This also implies that at context switch write
+ * to PQR-MSR is done only when a task with a
+ * different bitmask is scheduled in.
+ */
+static int intel_cache_alloc_cbm_write(struct cgroup_subsys_state *css,

Can you please drop these intel_ prefixes? They provide no value and just eat space.

Quoted 20 lines

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

+				 struct cftype *cft, u64 cbmvalue)
+{
+	u32 max_cbm = boot_cpu_data.x86_rdt_max_cbm_len;
+	struct intel_rdt *ir = css_rdt(css);
+	unsigned long cache_mask, max_mask;
+	unsigned long *cbm_tmp;
+	unsigned int closid;
+	ssize_t err = 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;

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 295

Quoted 4 lines

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

+
+	if (bitmap_equal(&cache_mask, cbm_tmp, MAX_CBM_LENGTH))
+		goto out;
+

Sigh, what's the point of this bitmap shuffling? Nothing as far as I can tell, because you first set all bits of maxmask and then AND cbmvalue.

All you achieve is, that you truncate cbmvalue to max_cbm bits. So if cbmvalue has any of the invalid bits set, you happily ignore that.

This whole bitmap usage is just making the code obfuscated.

    u64 max_mask = (1ULL << max_cbm) - 1;

    if (cbm_value & ~max_mask)
    	   return -EINVAL;

    if (cbm_value == (u64)ccmap[ir->clos].cache_mask)
    	   return 0;

No bitmap_set, bitmap_and nothing. Simple and readable, right?

The bitmap would only be necessary, IF max_cbm is > 64. But that's not the case and nothing in your code would survive ma_ cbm > 64. And as I said before CBM_LEN is 32bit for now and I serioulsy doubt, that it can ever go over 64bit simply because that would not fit into the mask MSRs.

So can you please get rid of that bitmap nonsense including the dynamic allocation and use simple and readable code constructs?

Just for the record:

    struct bla {
    	    u64			cbm_mask;
    };

versus:

    struct bla {
    	    unsigned long	*cbm_mask;
    };

plus

    bla.cbm_mask = kzalloc(sizeof(u64));

Is just utter waste of data and text space.

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 305

Quoted 6 lines

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

+	/*
+	 * 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);

So you drop the reference, before you make sure that you can get a new one?

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 313

Quoted 8 lines

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

+	if (cbm_search(cache_mask, &closid)) {
+		ir->clos = closid;
+		__clos_get(closid);
+	} else {
+		err = intel_rdt_alloc_closid(ir);
+		if (err)
+			goto out;

What happens if intel_rdt_alloc_closid() fails? ir->clos is unchanged, but you have no reference anymore. The next call to this function or anything else which calls __clos_put() will trigger the underflow warning and the warning will be completely useless because the wreckage happened before that.

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 334

Quoted 10 lines

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

+static inline bool intel_rdt_update_cpumask(int cpu)
+{
+	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;
+	}

You must be kidding.

wrong with having a bitmap of physical packages which have a CPU handling it?

	id =  topology_physical_package_id(cpu);
	if (!__test_and_set_bit(id, &package_map)) {
		cpumask_set_cpu(cpu, &rdt_cpumask);
		cbm_update_msrs(cpu);
	}

Four lines of code without loops and hoops. And here is a bitmap the proper data structure.

Quoted 10 lines

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

+static void intel_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;
+	}

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 371

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt.c +10 -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;
+		}
+	}

There is a better way than iterating over all online cpus ....

Quoted 1 line

arch/x86/kernel/cpu/intel_rdt.c out_err: · line 453

Quoted 9 lines

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

+static struct cftype rdt_files[] = {
+	{
+		.name = "cache_mask",
+		.seq_show = intel_cache_alloc_cbm_read,
+		.write_u64 = intel_cache_alloc_cbm_write,
+		.mode = 0666,
+	},
+	{ }	/* terminate */
+};

Where is the Documentation for that file? What's the content, format, error codes ....

Sigh.

tglx

Quoted 5 lines · Thomas Gleixner

Quoted 2 lines

+/*
+ * Mask of CPUs for writing CBM values. We only need one per-socket.

One mask? One CPU? One what? Please add comments which are understandable and useful for people who did NOT write that code.

when there is already code in the upstream code which has comments exactly consistent - when that made sense , this should ?

/*
* Mask of CPUs for reading CQM values. We only need one per-socket. */
static cpumask_t cqm_cpumask;

Quoted 9 lines

Quoted 9 lines

+/*
+ * cbm_update_msrs() - Updates all the existing IA32_L3_MASK_n MSRs
+ * which are one per CLOSid, except IA32_L3_MASK_0 on the current package.
+ * @cpu : the cpu on which the mask is updated.
+ */

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

+static inline void cbm_update_msrs(int cpu)
+{
+	int maxid = boot_cpu_data.x86_rdt_max_closid;

Quoted 8 lines

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 255

Quoted 6 lines

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

+	unsigned int i;
+
+	if (WARN_ON(cpu != smp_processor_id()))
+		return;
+
+	for (i = 1; i < maxid; i++) {

Lacks a comment why this starts with 1 and not with 0 as one would expect.

Its in the function comment just above - "except IA32_L3_MASK_0 on the current package."

Quoted 7 lines

Quoted 7 lines

+/*
+ * intel_cache_alloc_cbm_write() - Validates and writes the
+ * cache bit mask(cbm) to the IA32_L3_MASK_n
+ * and also store the same in the ccmap.
+ *
+ * CLOSids are reused for cgroups which have same bitmask.
+ * - This helps to use the scant CLOSids optimally.

Quoted 8 lines

Quoted 5 lines

+ * - This also implies that at context switch write
+ * to PQR-MSR is done only when a task with a
+ * different bitmask is scheduled in.
+ */
+static int intel_cache_alloc_cbm_write(struct cgroup_subsys_state *css,

Can you please drop these intel_ prefixes? They provide no value and just eat space.

well , i got ample comments which wanted me to me to specify intel as this feature is specific to intel.

the cache monitoring code has similar prefixes as well - is that pref specific

then ? I just dont want people to come back and ask to make it clear that things are intel specific

Quoted 20 lines

Quoted 20 lines

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

+				 struct cftype *cft, u64 cbmvalue)
+{
+	u32 max_cbm = boot_cpu_data.x86_rdt_max_cbm_len;
+	struct intel_rdt *ir = css_rdt(css);
+	unsigned long cache_mask, max_mask;
+	unsigned long *cbm_tmp;
+	unsigned int closid;
+	ssize_t err = 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);

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

+	cbm_tmp = &ccmap[ir->clos].cache_mask;

Quoted 8 lines

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 295

Quoted 4 lines

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

+
+	if (bitmap_equal(&cache_mask, cbm_tmp, MAX_CBM_LENGTH))
+		goto out;
+

Sigh, what's the point of this bitmap shuffling? Nothing as far as I can tell, because you first set all bits of maxmask and then AND cbmvalue.

Again , this contraditcs your earlier comment where you asked me to use the bit manipulation APIs to make it more *readable*. So do you want more readable now or is that not clear now ?

https://lkml.org/lkml/2014/11/19/1145

(quoted diff) +5 -0

+	/* Reset the least significant bit.*/
+	i = var & (var - 1);
+
+	if (i != (var & (var << 1)))
+		return false;

was replaced with the

	if (bitmap_weight(&var, maxcbm) < min_bitmask_len)
		return false;

	first_bit = find_next_bit(&var, maxcbm, 0);
	zero_bit = find_next_zero_bit(&var, maxcbm, first_bit);

	if (find_next_bit(&var, maxcbm, zero_bit) < maxcbm)
		return false;

3 lines replaced with more to make it more readable ! so does this come back then ?

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

+static inline bool cbm_minbits(unsigned long var)
+{
+
+	unsigned long i;

static inline bool cbm_iscontiguous(unsigned long var)

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

+{
+
+	unsigned long i;
+
+	/* Reset the least significant bit.*/
+	i = var & (var - 1);
+
+	/*
+	 * We would have a set of non-contiguous bits when
+	 * there is at least one zero
+	 * between the most significant 1 and least significant 1.
+	 * In the below '&' operation,(var <<1) would have zero in
+	 * at least 1 bit position in var apart from least
+	 * significant bit if it does not have contiguous bits.

Quoted 2 lines

All you achieve is, that you truncate cbmvalue to max_cbm bits. So if cbmvalue has any of the invalid bits set, you happily ignore that.

taskset ignores the extra bits. is that a problem here ?

Quoted 4 lines

Quoted 4 lines

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

+static inline bool intel_rdt_update_cpumask(int cpu)
+{
+	int phys_id = topology_physical_package_id(cpu);
+	struct cpumask *mask = &rdt_cpumask;

Quoted 8 lines

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 334

Quoted 6 lines

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

+	int i;
+
+	for_each_cpu(i, mask) {
+		if (phys_id == topology_physical_package_id(i))
+			return false;
+	}

You must be kidding.

the rapl and cqm use similar code. You want me to keep a seperate package mask for this code which not would be that frequent at all ?

for c-state transitions only idle_notifier gets called - so this is only when a new package is physically added. and we dont need anything for cache alloc for idle transitions. not really frequent ?

Quoted 10 lines · Vikas Shivappa

Quoted 5 lines · Thomas Gleixner

Quoted 2 lines

+/*
+ * Mask of CPUs for writing CBM values. We only need one per-socket.

One mask? One CPU? One what? Please add comments which are understandable and useful for people who did NOT write that code.

when there is already code in the upstream code which has comments exactly consistent - when that made sense , this should ?

Quoted 4 lines

/*
* Mask of CPUs for reading CQM values. We only need one per-socket. */
static cpumask_t cqm_cpumask;

And just because there is a lousy comment upstream it does not become better by copying it.

Quoted 19 lines

Quoted 18 lines

Quoted 7 lines

+/*
+ * cbm_update_msrs() - Updates all the existing IA32_L3_MASK_n MSRs
+ * which are one per CLOSid, except IA32_L3_MASK_0 on the current package.
+ * @cpu : the cpu on which the mask is updated.
+ */

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 255

Quoted 9 lines

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

+static inline void cbm_update_msrs(int cpu)
+{
+	int maxid = boot_cpu_data.x86_rdt_max_closid;
+	unsigned int i;
+
+	if (WARN_ON(cpu != smp_processor_id()))
+		return;
+
+	for (i = 1; i < maxid; i++) {

Lacks a comment why this starts with 1 and not with 0 as one would expect.

Quoted 2 lines

Its in the function comment just above - "except IA32_L3_MASK_0 on the current package."

No. That comment explains WHAT it does not WHY. Asided of that that comment is not correct KernelDoc format.

Quoted 17 lines

Quoted 16 lines

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 272

Quoted 13 lines

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

+/*
+ * intel_cache_alloc_cbm_write() - Validates and writes the
+ * cache bit mask(cbm) to the IA32_L3_MASK_n
+ * and also store the same in the ccmap.
+ *
+ * CLOSids are reused for cgroups which have same bitmask.
+ * - This helps to use the scant CLOSids optimally.
+ * - This also implies that at context switch write
+ * to PQR-MSR is done only when a task with a
+ * different bitmask is scheduled in.
+ */
+static int intel_cache_alloc_cbm_write(struct cgroup_subsys_state *css,

Can you please drop these intel_ prefixes? They provide no value and just eat space.

Quoted 2 lines

well , i got ample comments which wanted me to me to specify intel as this feature is specific to intel.

It's fine for public interfaces, but for functions which are only used inside a file which contains only intel specific code it's just silly.

Quoted 2 lines

the cache monitoring code has similar prefixes as well - is that pref specific

Sigh. There is so much crap in the kernel, you'll find an example for everything.

Quoted 3 lines

then ? I just dont want people to come back and ask to make it clear that things are intel specific

The file name tells everyone its INTEL specific, right? And following your argumentation you should have named your helper functions intel_get_closid() and intel_put_closid() as well.

If your code would have been in a proper shape otherwise, I would probably have just let it slip.

Quoted 1 line

taskset ignores the extra bits. is that a problem here ?

I do not care, what taskset does. I care what this code does.

It tells the user that: 0x100ffff is fine, even if the valid bits are just 0xffff.

And taskset is a different story due to cpu hotplug.

But for CAT the maskbits are defined by hardware and cannot change ever. So why would we allow to set invalid ones and claim that its correct?

If there is a reason to do so, then it needs a proper comment in the code and a proper explanation in Documentation/.....

Quoted 13 lines

Quoted 12 lines

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 334

Quoted 10 lines

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

+static inline bool intel_rdt_update_cpumask(int cpu)
+{
+	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;
+	}

You must be kidding.

Quoted 2 lines

the rapl and cqm use similar code. You want me to keep a seperate package mask for this code which not would be that frequent at all ?

You find for everything a place where you copied your stuff from without thinking about it, right?

Other people dessperately try to fix the cpu online times which are more and more interesting the larger the systems become. So it might be a good idea to come up with a proper fast implementation which can be used everywhere instead of blindly copying code.

Quoted 3 lines

for c-state transitions only idle_notifier gets called - so this is only when a new package is physically added. and we dont need anything for cache alloc for idle transitions. not really frequent ?

Crap. It's called for every cpu which comes online and goes offline, not for new packages.

Thanks,

tglx

Quoted 9 lines · Vikas Shivappa

Quoted 5 lines · Thomas Gleixner

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 334

Quoted 3 lines

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

+static inline bool intel_rdt_update_cpumask(int cpu)
+{
+	}

You must be kidding.

the rapl and cqm use similar code. You want me to keep a seperate package mask for this code which not would be that frequent at all ?

You find for everything a place where you copied your stuff from without thinking about it, right?

Other people dessperately try to fix the cpu online times which are more and more interesting the larger the systems become. So it might be a good idea to come up with a proper fast implementation which can be used everywhere instead of blindly copying code.

Quoted 4 lines

for c-state transitions only idle_notifier gets called - so this is only when a new package is physically added. and we dont need anything for cache alloc for idle transitions. not really frequent ?

Crap. It's called for every cpu which comes online and goes offline, not for new packages.

Thanks,

tglx

Quoted 15 lines · Thomas Gleixner

Quoted 10 lines · Vikas Shivappa

Quoted 5 lines · Thomas Gleixner

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 334

Quoted 3 lines

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

+static inline bool intel_rdt_update_cpumask(int cpu)
+{
+	}

You must be kidding.

the rapl and cqm use similar code. You want me to keep a seperate package mask
for this code which not would be that frequent at all ?

You find for everything a place where you copied your stuff from without thinking about it, right?

Quoted 4 lines

Other people dessperately try to fix the cpu online times which are more and more interesting the larger the systems become. So it might be a good idea to come up with a proper fast implementation which can be used everywhere instead of blindly copying code.

Ok , i can try to do this as a seperate patch after the cache allocation to

get a support for faster implementation for traversing package and cpus in the packages which can be used by everyone. we would need to start from scratch with having packagemask_t equivalent to cpumask_t. hope that is fair ?

Quoted 17 lines · Vikas Shivappa

Quoted 15 lines · Thomas Gleixner

Quoted 10 lines · Vikas Shivappa

Quoted 5 lines · Thomas Gleixner

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_css_free(struct cgroup_subsys_state *css) · line 334

Quoted 3 lines

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

+static inline bool intel_rdt_update_cpumask(int cpu)
+{
+	}

You must be kidding.

the rapl and cqm use similar code. You want me to keep a seperate package mask
for this code which not would be that frequent at all ?

You find for everything a place where you copied your stuff from without thinking about it, right?

Quoted 6 lines

Quoted 4 lines

Other people dessperately try to fix the cpu online times which are more and more interesting the larger the systems become. So it might be a good idea to come up with a proper fast implementation which can be used everywhere instead of blindly copying code.

Ok , i can try to do this as a seperate patch after the cache allocation to

Hell no. We do preparatory patches first. I'm not believing in 'can try' promises.

Quoted 3 lines

get a support for faster implementation for traversing package and cpus in the packages which can be used by everyone. we would need to start from scratch with having packagemask_t equivalent to cpumask_t. hope that is fair ?

Yes, that's what I want to see.

Quoted 25 lines · Thomas Gleixner

Quoted 17 lines · Vikas Shivappa

Quoted 15 lines · Thomas Gleixner

Quoted 10 lines · Vikas Shivappa

Quoted 5 lines · Thomas Gleixner

Review

Quoted 3 lines

(quoted diff) +3 -0

+static inline bool intel_rdt_update_cpumask(int cpu)
+{
+	}

You must be kidding.

the rapl and cqm use similar code. You want me to keep a seperate package mask
for this code which not would be that frequent at all ?

You find for everything a place where you copied your stuff from without thinking about it, right?

Quoted 6 lines

Quoted 4 lines

Other people dessperately try to fix the cpu online times which are more and more interesting the larger the systems become. So it might be a good idea to come up with a proper fast implementation which can be used everywhere instead of blindly copying code.

Ok , i can try to do this as a seperate patch after the cache allocation to

Quoted 8 lines

Hell no. We do preparatory patches first. I'm not believing in 'can try' promises.

Quoted 3 lines

get a support for faster implementation for traversing package and cpus in the packages which can be used by everyone. we would need to start from scratch with having packagemask_t equivalent to cpumask_t. hope that is fair ?

Yes, that's what I want to see.

I was kidding. You need two functionalities:

1) A way to figure out whether you already have a CPU taking care of the package to which the newly online CPU belongs.

That's something you need to track in your own code and I already gave you the 5 lines of code which can handle that. Remember?

id = topology_physical_package_id(cpu); if (!__test_and_set_bit(id, &package_map)) {
cpumask_set_cpu(cpu, &rdt_cpumask); cbm_update_msrs(cpu);
}

So you cannot add much infrastructure for that because you need to track your own state in the CAT relevant package bitmap.

2) A way to find out whether the to be offlined CPU is the last online CPU of a package. If it's not the last one, then you need a fast way to find the next online cpu which belongs to that package. If it's the last one you need to kill the cat.

The information is already available, so it's not rocket science. And it takes maximal 7 lines of code to implement it.

Q: Why 'maximal' 7?
A: Because I'm a lazy bastard and cannot be bothered to figure
out whether one of the lines is superfluous.
Q: Why can't I be bothered?
A: It's none of my problems and it actually does not matter much.

Here is the pseudo code:

do_magic_stuff(tmp, TCC, COM);
clr(c, tmp);
n = do_more_stuff(tmp);
if (notavailable(n))
kill_the_cat();
else
set(n, RCM);

Hint: One of the NNN placeholders resolves to topology_core_cpumask()

Now once you figured that out, you will notice that the above 5 lines of code to solve problem #1 can be simplified because you can avoid the package_map bitmap completely.

Sorry, no pseudo code for this. But you get more than one hint this time:
- Hint #1 still applies
- The logic is very similar to the above #2 pseudo code
- It takes maximal 6 lines of code to implement it

There is one caveat:

If Hint #1 cannot solve your problem, then you need to figure out why and then work with the people who are responsible for it to figure out how it can be resolved.

A few general hints:

- The line counts are neither including the conditionals which invoke that code nor the function body nor variable declarations. But they include braces,

All I care about is the logic itself. See the pseudo code example above.

- Please provide a solution for #2 and #1 before you bother me with another patch series.

Thanks,

tglx

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

+static inline void intel_rdt_sched_in(void)
+{
+	if (rdt_enabled())
+		__rdt_sched_in();
+}
 
+void __rdt_sched_in(void)
+{

arch/x86/include/asm/intel_rdt.h struct clos_cbm_map { · line 35

Quoted 9 lines · Vikas Shivappa

struct rdt_subsys_info {

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

 	/* Clos Bitmap to keep track of available CLOSids.*/

@@ -24,6 +30,11 @@ struct clos_cbm_map {
 	unsigned int clos_refcnt;
 };
 
+static inline bool rdt_enabled(void)
+{
+	return static_key_false(&rdt_enable_key);

So again, why this useless helper function for a single call site?

Quoted 15 lines

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

+static inline void intel_rdt_sched_in(void)
+{
+	if (rdt_enabled())
+		__rdt_sched_in();
+}
 
+void __rdt_sched_in(void)
+{
+	struct task_struct *task = current;
+	struct intel_rdt *ir;
+	unsigned int clos;
+
+	/*
+	 * This needs to be fixed
+	 * to cache the whole PQR instead of just CLOSid.

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_free_closid(unsigned int clos) · line 95

Quoted 3 lines

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

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

And why is this not fixed __BEFORE__ this patch? You can do the changes to struct intel_cqm_state in a seperate patch and then do the proper implementation from the beginning instead of providing a half broken variant which gets replaced in the next patch.

Thanks,

tglx

Quoted 12 lines · Thomas Gleixner

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_free_closid(unsigned int clos) · line 95

Quoted 6 lines · Vikas Shivappa

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

+	/*
+	 * 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.

And why is this not fixed __BEFORE__ this patch? You can do the changes to struct intel_cqm_state in a seperate patch and then do the proper implementation from the beginning instead of providing a half broken variant which gets replaced in the next patch.

Ok , can fix both items in your comments. Reason I had it seperately is that the cache affects both cmt and cache allocation patches.

Quoted 14 lines · Vikas Shivappa

Quoted 12 lines · Thomas Gleixner

arch/x86/kernel/cpu/intel_rdt.c static void intel_rdt_free_closid(unsigned int clos) · line 95

Quoted 6 lines · Vikas Shivappa

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

+	/*
+	 * 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.

And why is this not fixed __BEFORE__ this patch? You can do the changes to struct intel_cqm_state in a seperate patch and then do the proper implementation from the beginning instead of providing a half broken variant which gets replaced in the next patch.

Quoted 2 lines

Ok , can fix both items in your comments. Reason I had it seperately is that the cache affects both cmt and cache allocation patches.

And that's the wrong reason. Sure it affects both, but we first prepare the changes to the existing code and then build new stuff on top of it not the other way round. Building the roof before the basement is almost never a good idea.

Thanks,

tglx

Signed-off-by Vikas Shivappa <[email protected]>

Conflicts:
arch/x86/kernel/cpu/perf_event_intel_cqm.c

rcu_read_lock();

Quoted 4 lines · Vikas Shivappa

Signed-off-by Vikas Shivappa <[email protected]>

Conflicts:
arch/x86/kernel/cpu/perf_event_intel_cqm.c

And that's interesting for what?

Quoted 3 lines

arch/x86/include/asm/rdt_common.h new file +0 -0

arch/x86/include/asm/rdt_common.h line 9

Quoted 10 lines

arch/x86/include/asm/rdt_common.h new file +9 -0

@@ -0,0 +1,13 @@
+#ifndef _X86_RDT_H_
+#define _X86_RDT_H_
+
+#define MSR_IA32_PQR_ASSOC	0x0c8f
+
+struct intel_pqr_state {
+	raw_spinlock_t	lock;
+	int		rmid;
+	int		clos;

Those want to be u32. We have no type checkes there, but u32 is the proper data type for wrmsr.

arch/x86/include/asm/rdt_common.h line 12

Quoted 4 lines

arch/x86/include/asm/rdt_common.h new file +3 -0

+	int		cnt;
+};
+

What's wrong with having this in intel_rdt.h?

fan of sprinkling stuff all over the place so reading becomes a chasing game.

Quoted 6 lines

arch/x86/include/asm/rdt_common.h new file +2 -0

 {
 	struct task_struct *task = current;
 	struct intel_rdt *ir;
+	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
+	unsigned long flags;

arch/x86/include/asm/rdt_common.h

Quoted 1 line

arch/x86/include/asm/rdt_common.h new file +1 -0

+	raw_spin_lock_irqsave(&state->lock, flags);

finish_arch_switch() is called with interrupts disabled already ...

Quoted 2 lines

rcu_read_lock();

So we now have a spin_lock() and rcu_read_lock() and no explanation what is protecting what.

arch/x86/include/asm/rdt_common.h

Quoted 3 lines

arch/x86/include/asm/rdt_common.h new file +1 -1

 	ir = task_rdt(task);
-	if (ir->clos == clos) {
+	if (ir->clos == state->clos) {

And of course taking the spin lock for checking state->clos is complete and utter nonsense.

state->clos can only be changed by this code and the only reason why we need the lock is to protect against concurrent modification of state->rmid.

So the check for ir->clos == state->clos can be done lockless.

And I seriously doubt, that state->lock is necessary at all.

Q: What is it protecting?
A: state->clos, state->rmid, state->cnt

Q: What's the context?
A: Per cpu context. The per cpu pqr state is NEVER modified from a
remote cpu.

Q: What is the lock for?
A: Nothing.

Q: Why?
A: Because interrupt disabled regions protect per cpu state perfectly
fine and there is is no memory ordering issue which would require a lock or barrier either.

Peter explained it to you several times already that context switch is one the most sensitive hot pathes where we care about every cycle. But you just go ahead and mindlessly create pointless overhead.

arch/x86/include/asm/rdt_common.h

Quoted 4 lines

arch/x86/include/asm/rdt_common.h new file +4 -0

+	/*
+	 * PQR has closid in high 32 bits and CQM-RMID
+	 * in low 10 bits. Rewrite the exsting rmid from
+	 * software cache.

This comment wouldnt be necessary if you would have proper documented struct pqr_state.

Quoted 8 lines

arch/x86/include/asm/rdt_common.h new file +5 -0

+	 */
+	wrmsr(MSR_IA32_PQR_ASSOC, state->rmid, ir->clos);
+	state->clos = ir->clos;
 	rcu_read_unlock();
+	raw_spin_unlock_irqrestore(&state->lock, flags);
+
 }

arch/x86/include/asm/rdt_common.h

Quoted 2 lines

arch/x86/include/asm/rdt_common.h new file +1 -1

-static DEFINE_PER_CPU(struct intel_cqm_state, cqm_state);
+DEFINE_PER_CPU(struct intel_pqr_state, pqr_state);

With CONFIG_PERF=n and CONFIG_CGROUP_RDT=y the linker will fail.

Thanks,

tglx

Quoted 4 lines · Thomas Gleixner

arch/x86/kernel/cpu/perf_event_intel_cqm.c line 19

Quoted 2 lines

arch/x86/kernel/cpu/perf_event_intel_cqm.c +1 -1

-static DEFINE_PER_CPU(struct intel_cqm_state, cqm_state);
+DEFINE_PER_CPU(struct intel_pqr_state, pqr_state);

With CONFIG_PERF=n and CONFIG_CGROUP_RDT=y the linker will fail.

copy from Makefile below -
obj-$(CONFIG_CPU_SUP_INTEL) += perf_event_intel_rapl.o
perf_event_intel_cqm.o

should work with CONFIG_PERF_EVENTS=n and CGROUP_RDT=y ?

Quoted 6 lines · Vikas Shivappa

Quoted 4 lines · Thomas Gleixner

arch/x86/kernel/cpu/perf_event_intel_cqm.c line 19

Quoted 2 lines

arch/x86/kernel/cpu/perf_event_intel_cqm.c +1 -1

-static DEFINE_PER_CPU(struct intel_cqm_state, cqm_state);
+DEFINE_PER_CPU(struct intel_pqr_state, pqr_state);

With CONFIG_PERF=n and CONFIG_CGROUP_RDT=y the linker will fail.

Quoted 5 lines

copy from Makefile below -
obj-$(CONFIG_CPU_SUP_INTEL) += perf_event_intel_rapl.o
perf_event_intel_cqm.o

should work with CONFIG_PERF_EVENTS=n and CGROUP_RDT=y ?

Groan. Did you try to compile it? Certainly not.

Simply because the whole section which contains perf_event* object files is conditional on

ifdef CONFIG_PERF_EVENTS

I'm starting to get really grumpy and tired of your attitude.

Thanks,

tglx

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines · Vikas Shivappa

Quoted 4 lines · Thomas Gleixner

arch/x86/kernel/cpu/perf_event_intel_cqm.c line 19

Quoted 2 lines

arch/x86/kernel/cpu/perf_event_intel_cqm.c +1 -1

-static DEFINE_PER_CPU(struct intel_cqm_state, cqm_state);
+DEFINE_PER_CPU(struct intel_pqr_state, pqr_state);

With CONFIG_PERF=n and CONFIG_CGROUP_RDT=y the linker will fail.

Quoted 7 lines

Quoted 5 lines

copy from Makefile below -
obj-$(CONFIG_CPU_SUP_INTEL) += perf_event_intel_rapl.o
perf_event_intel_cqm.o

should work with CONFIG_PERF_EVENTS=n and CGROUP_RDT=y ?

Groan. Did you try to compile it? Certainly not.

of course I did compile successfully after I changed PERF_EVENTS=n and RDT=y in the .config.
sorry, my reason was wrong though.

What I had not noticed was the .config is simply added to enabling CONFIG_PERF_EVENTS=y even though i disable it by editing .config. Thats because x86 is selecting it.
So PERF_EVENTS cant be disabled on x86 and RDT is only in x86 && SUP_INTEL.

How did you configure CONFIG_PERF_EVENTS=n and CGROUP_RDT=y and see the error

Quoted 5 lines

Quoted 5 lines

copy from Makefile below -
obj-$(CONFIG_CPU_SUP_INTEL) += perf_event_intel_rapl.o
perf_event_intel_cqm.o

should work with CONFIG_PERF_EVENTS=n and CGROUP_RDT=y ?

So PERF_EVENTS cant be disabled on x86 and RDT is only in x86 && SUP_INTEL.

Quoted 4 lines · Vikas Shivappa

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Vikas Shivappa

On Fri, 15 May 2015, Thomas Gleixner wrote:

Quoted 5 lines

sorry, my reason was wrong though.

So PERF_EVENTS cant be disabled on x86 and RDT is only in x86 && SUP_INTEL.

That should have been your answer to #1, right?

Thanks,

tglx

This patch adds a cgroup subsystem to support the new Cache Allocation feature found in future Intel Xeon Intel processors. Cache Allocation is a sub-feature with in Resource Director Technology(RDT) feature. RDT provides support to control sharing of platform resources like L3 cache.

Cache Allocation Technology provides a way for the Software (OS/VMM) to restrict L3 cache allocation to a defined 'subset' of cache which may be overlapping with other 'subsets'. This feature is used when allocating a line in cache ie when pulling new data into the cache. The programming of the h/w is done via programming MSRs. The patch series only supports to perform L3 cache allocation. However note that some of the datastructures and framework which is common to RDT are also implemented but its currently used only by the cache allocation sub-feature.

In todays new processors the number of cores is continuously increasing which in turn increase the number of threads or workloads that can simultaneously be run. When multi-threaded
applications run concurrently, they compete for shared
resources including L3 cache. At times, this L3 cache resource contention may result in inefficient space utilization. For example a higher priority thread may end up with lesser L3 cache resource or a cache sensitive app may not get optimal cache occupancy thereby degrading the performance.
Cache Allocation kernel patch helps provides a framework for sharing L3 cache so that users can allocate the resource according to set requirements.

More information about the feature can be found in the Intel SDM, Volume 3 section 17.15. SDM does not yet use the 'RDT' term yet and it is planned to be changed at a later time.

*All the patches will apply on 4.1-rc0*.

Changes in V7: (Thanks to feedback from PeterZ and Matt and following discussions)
- changed lot of naming to reflect the data structures which are common to RDT and specific to Cache allocation.
- removed all usage of 'cat'. replace with more friendly cache allocation
- fixed lot of convention issues (whitespace, return paradigm etc)
- changed the scheduling hook for RDT to not use a inline.
- removed adding new scheduling hook and just reused the existing one similar to perf hook.

Changes in V6:
- rebased to 4.1-rc1 which has the CMT(cache monitoring) support included.
- (Thanks to Marcelo's feedback).Fixed support for hot cpu handling for IA32_L3_QOS MSRs. Although during deep C states the MSR need not be restored this is needed when physically a new package is added.
-some other coding convention changes including renaming to cache_mask using a refcnt to track the number of cgroups using a closid in clos_cbm map. -1b cbm support for non-hsw SKUs. HSW is an exception which needs the cache bit masks to be at least 2 bits.

Changes in v5:
- Added support to propagate the cache bit mask update for each package.
- Removed the cache bit mask reference in the intel_rdt structure as there was no need for that and we already maintain a separate closid<->cbm mapping.
- Made a few coding convention changes which include adding the assertion while freeing the CLOSID.

Changes in V4:
- Integrated with the latest V5 CMT patches.
- Changed naming of cgroup to rdt(resource director technology) from cat(cache allocation technology). This was done as the RDT is the umbrella term for platform shared resources allocation. Hence in future it would be easier to add resource allocation to the same cgroup
- Naming changes also applied to a lot of other data structures/APIs.
- Added documentation on cgroup usage for cache allocation to address a lot of questions from various academic and industry regarding cache allocation usage.

Changes in V3:
- Implements a common software cache for IA32_PQR_MSR
- Implements support for hsw Cache Allocation enumeration. This does not use the brand strings like earlier version but does a probe test. The probe test is done only on hsw family of processors
- Made a few coding convention, name changes
- Check for lock being held when ClosID manipulation happens

Changes in V2:
- Removed HSW specific enumeration changes. Plan to include it later as a separate patch.
- Fixed the code in prep_arch_switch to be specific for x86 and removed x86 defines.
- Fixed cbm_write to not write all 1s when a cgroup is freed.
- Fixed one possible memory leak in init.
- Changed some of manual bitmap
manipulation to use the predefined bitmap APIs to make code more readable
- Changed name in sources from cqe to cat
- Global cat enable flag changed to static_key and disabled cgroup early_init

[PATCH 1/7] x86/intel_rdt: Intel Cache Allocation detection [PATCH 2/7] x86/intel_rdt: Adds support for Class of service management [PATCH 3/7] x86/intel_rdt: Add support for cache bit mask management [PATCH 4/7] x86/intel_rdt: Implement scheduling support for Intel RDT [PATCH 5/7] x86/intel_rdt: Software Cache for IA32_PQR_MSR [PATCH 6/7] x86/intel_rdt: Intel haswell Cache Allocation enumeration [PATCH 7/7] x86/intel_rdt: Add Cache Allocation documentation and user guide