x86/cqm3: Resctrl based cqm

static void
mba_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r); static void

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

@@ -230,7 +236,7 @@ static bool rdt_get_mem_config(struct rdt_resource *r)
 	return true;
 }
 
-static void rdt_get_cache_config(int idx, struct rdt_resource *r)
+static void rdt_get_cache_alloc_config(int idx, struct rdt_resource *r)
 {
 	union cpuid_0x10_1_eax eax;
 	union cpuid_0x10_x_edx edx;

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

 		kfree(d);
 		return;
 	}

@@ -513,34 +519,39 @@ static __init void rdt_init_padding(void)
 
 static __init bool get_rdt_resources(void)
 {
-	bool ret = false;
-
 	if (cache_alloc_hsw_probe())

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

+/*
+ * Event IDs are used to program IA32_QM_EVTSEL before reading event
+ * counter from IA32_QM_CTR
+ */
+#define QOS_L3_OCCUP_EVENT_ID		0x01
+#define QOS_L3_MBM_TOTAL_EVENT_ID	0x02
+#define QOS_L3_MBM_LOCAL_EVENT_ID	0x03
+
+/**
+ * struct mon_evt - Entry in the event list of a resource
+ * @evtid:		event id
+ * @name:		name of the event

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +15 -0

+};
+
+/**
+ * @rmid_free_lru    A least recently used list of free RMIDs
+ *     These RMIDs are guaranteed to have an occupancy less than the
+ *     threshold occupancy
+ */
+static struct list_head	rmid_free_lru;
+
+/**
+ * @rmid_limbo_lru       list of currently unused but (potentially)
+ *     dirty RMIDs.
+ *     This list contains RMIDs that no one is currently using but that
+ *     may have a occupancy value > intel_cqm_threshold. User can change
+ *     the threshold occupancy value.

+ * resource monitoring is enabled.
+ */

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +15 -0

+int rdt_mon_features;
+
+/*
+ * This is the threshold cache occupancy at which we will consider an
+ * RMID available for re-allocation.
+ */
+unsigned int intel_cqm_threshold;
+
+static inline struct rmid_entry *__rmid_entry(u32 rmid)
+{
+	struct rmid_entry *entry;
+
+	entry = &rmid_ptrs[rmid];
+	WARN_ON(entry->rmid != rmid);
+

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +10 -0

+		entry = &rmid_ptrs[i];
+		INIT_LIST_HEAD(&entry->list);
+
+		entry->rmid = i;
+		list_add_tail(&entry->list, &rmid_free_lru);
+	}
+
+	/*
+	 * RMID 0 is special and is always allocated. It's used for all
+	 * tasks that are not monitored.

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +24 -0

+}
+
+void rdt_get_mon_l3_config(struct rdt_resource *r)
+{
+	int ret;
+
+	r->mon_scale = boot_cpu_data.x86_cache_occ_scale;
+	r->num_rmid = boot_cpu_data.x86_cache_max_rmid + 1;
+
+	/*
+	 * A reasonable upper limit on the max threshold is the number
+	 * of lines tagged per RMID if all RMIDs have the same number of
+	 * lines tagged in the LLC.
+	 *
+	 * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
+	 */
+	intel_cqm_threshold = boot_cpu_data.x86_cache_size * 1024 / r->num_rmid;
+
+	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */
+	intel_cqm_threshold /= r->mon_scale;
+
+	ret = dom_data_init(r);
+	if (ret)
+		goto out;

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

Quoted 5 lines · Vikas Shivappa

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

+/*
+ * Global boolean for rdt_alloc which is true if any
+ * resource allocation is enabled.
+ */
+bool rdt_alloc_enabled;

That should be rdt_alloc_capable. It's not enabled at probe time. Probing merily detects the capability. That mirrors the capable/enabled bits in the rdt resource struct.

Quoted 13 lines

static void
mba_wrmsr(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r); static void

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

@@ -230,7 +236,7 @@ static bool rdt_get_mem_config(struct rdt_resource *r)
 	return true;
 }
 
-static void rdt_get_cache_config(int idx, struct rdt_resource *r)
+static void rdt_get_cache_alloc_config(int idx, struct rdt_resource *r)
 {
 	union cpuid_0x10_1_eax eax;
 	union cpuid_0x10_x_edx edx;

arch/x86/kernel/cpu/intel_rdt.c static void domain_add_cpu(int cpu, struct rdt_resource *r) · line 431

Quoted 5 lines

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

@@ -422,7 +428,7 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
 
 	d->id = id;
 
-	if (domain_setup_ctrlval(r, d)) {
+	if (r->alloc_capable && domain_setup_ctrlval(r, d)) {

This should be done in the name space cleanup patch or in a separate one.

Quoted 10 lines

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

 		kfree(d);
 		return;
 	}

@@ -513,34 +519,39 @@ static __init void rdt_init_padding(void)
 
 static __init bool get_rdt_resources(void)
 {
-	bool ret = false;
-
 	if (cache_alloc_hsw_probe())

arch/x86/kernel/cpu/intel_rdt.c static __init void rdt_init_padding(void) · line 530

Quoted 10 lines

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

-		return true;
+		rdt_alloc_enabled = true;
 
-	if (!boot_cpu_has(X86_FEATURE_RDT_A))
+	if ((!rdt_alloc_enabled && !boot_cpu_has(X86_FEATURE_RDT_A)) &&
+	    !boot_cpu_has(X86_FEATURE_CQM))
 		return false;
 
+	if (boot_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
+		rdt_mon_features |= (1 << QOS_L3_OCCUP_EVENT_ID);

Instead of artificially cramming the CQM bits into this function, it would be cleaner to leave that function alone, rename it to

get_rdt_alloc_resources()

and have a new function

get_rdt_mon_resources()

and handle the aggregation at the call site.

	rdt_alloc_capable = get_rdt_alloc_resources();
	rdt_mon_capable = get_rdt_mon_resources();

	if (!rdt_alloc_capable && !rdt_mon_capable)
		return -ENODEV;

I'd make both variables boolean and have rdt_mon_features as a separate one, which carries the actual available feature bits. This is neither hotpath nor are we in a situation where we need to spare the last 4byte of memory. Clean separation of code and functionality is more important.

Quoted 12 lines

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

+/*
+ * Event IDs are used to program IA32_QM_EVTSEL before reading event
+ * counter from IA32_QM_CTR
+ */
+#define QOS_L3_OCCUP_EVENT_ID		0x01
+#define QOS_L3_MBM_TOTAL_EVENT_ID	0x02
+#define QOS_L3_MBM_LOCAL_EVENT_ID	0x03
+
+/**
+ * struct mon_evt - Entry in the event list of a resource
+ * @evtid:		event id
+ * @name:		name of the event

arch/x86/kernel/cpu/intel_rdt.h line 36

Quoted 10 lines

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

+ */
+struct mon_evt {
+	u32			evtid;
+	char			*name;
+	struct list_head	list;
+};
+
+extern unsigned int intel_cqm_threshold;
+extern bool rdt_alloc_enabled;
+extern int rdt_mon_features;

Please do not use 'int' for variables which contain bit flags. unsigned int is the proper choice here.

arch/x86/kernel/cpu/intel_rdt_monitor.c line 39

Quoted 4 lines

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +4 -0

+struct rmid_entry {
+	u32 rmid;
+	enum rmid_recycle_state state;
+	struct list_head list;

Please make it tabular as you did with mon_evt and other structs.

Quoted 15 lines

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +15 -0

+};
+
+/**
+ * @rmid_free_lru    A least recently used list of free RMIDs
+ *     These RMIDs are guaranteed to have an occupancy less than the
+ *     threshold occupancy
+ */
+static struct list_head	rmid_free_lru;
+
+/**
+ * @rmid_limbo_lru       list of currently unused but (potentially)
+ *     dirty RMIDs.
+ *     This list contains RMIDs that no one is currently using but that
+ *     may have a occupancy value > intel_cqm_threshold. User can change
+ *     the threshold occupancy value.

arch/x86/kernel/cpu/intel_rdt_monitor.c line 64

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +10 -0

+ */
+static struct list_head	rmid_limbo_lru;
+
+/**
+ * @rmid_entry - The entry in the limbo and free lists.
+ */
+static struct rmid_entry	*rmid_ptrs;
+
+/*
+ * Global boolean for rdt_monitor which is true if any

Boolean !?!?!

Quoted 17 lines

+ * resource monitoring is enabled.
+ */

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +15 -0

+int rdt_mon_features;
+
+/*
+ * This is the threshold cache occupancy at which we will consider an
+ * RMID available for re-allocation.
+ */
+unsigned int intel_cqm_threshold;
+
+static inline struct rmid_entry *__rmid_entry(u32 rmid)
+{
+	struct rmid_entry *entry;
+
+	entry = &rmid_ptrs[rmid];
+	WARN_ON(entry->rmid != rmid);
+

arch/x86/kernel/cpu/intel_rdt_monitor.c line 91

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +10 -0

+	return entry;
+}
+
+static int dom_data_init(struct rdt_resource *r)
+{
+	struct rmid_entry *entry = NULL;
+	int i = 0, nr_rmids;
+
+	INIT_LIST_HEAD(&rmid_free_lru);
+	INIT_LIST_HEAD(&rmid_limbo_lru);

You can spare that by declaring the list head with

static LIST_HEAD(rmid_xxx_lru);

arch/x86/kernel/cpu/intel_rdt_monitor.c line 98

Quoted 7 lines

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +7 -0

+
+	nr_rmids = r->num_rmid;
+	rmid_ptrs = kcalloc(nr_rmids, sizeof(struct rmid_entry), GFP_KERNEL);
+	if (!rmid_ptrs)
+		return -ENOMEM;
+
+	for (; i < nr_rmids; i++) {

Please initialize i in the for() construct. It's really bad to read, because the missing initialization statement makes one look for a special initialization magic just to figure out that it's simply i = 0.

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +10 -0

+		entry = &rmid_ptrs[i];
+		INIT_LIST_HEAD(&entry->list);
+
+		entry->rmid = i;
+		list_add_tail(&entry->list, &rmid_free_lru);
+	}
+
+	/*
+	 * RMID 0 is special and is always allocated. It's used for all
+	 * tasks that are not monitored.

arch/x86/kernel/cpu/intel_rdt_monitor.c line 118

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +10 -0

+	 */
+	entry = __rmid_entry(0);
+	list_del(&entry->list);
+
+	return 0;
+}
+
+static struct mon_evt llc_occupancy_event = {
+	.name = "llc_occupancy",
+	.evtid = QOS_L3_OCCUP_EVENT_ID,

Tabluar...

arch/x86/kernel/cpu/intel_rdt_monitor.c line 126

Quoted 8 lines

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +8 -0

+};
+
+static void l3_mon_evt_init(struct rdt_resource *r)
+{
+	INIT_LIST_HEAD(&r->evt_list);
+
+	if (rdt_mon_features & (1 << QOS_L3_OCCUP_EVENT_ID))
+		list_add_tail(&llc_occupancy_event.list, &r->evt_list);

What's that list for? Why don't you have that event as a member of the L3 rdt resource and control it via r->mon_capable/enabled?

Quoted 25 lines

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +24 -0

+}
+
+void rdt_get_mon_l3_config(struct rdt_resource *r)
+{
+	int ret;
+
+	r->mon_scale = boot_cpu_data.x86_cache_occ_scale;
+	r->num_rmid = boot_cpu_data.x86_cache_max_rmid + 1;
+
+	/*
+	 * A reasonable upper limit on the max threshold is the number
+	 * of lines tagged per RMID if all RMIDs have the same number of
+	 * lines tagged in the LLC.
+	 *
+	 * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
+	 */
+	intel_cqm_threshold = boot_cpu_data.x86_cache_size * 1024 / r->num_rmid;
+
+	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */
+	intel_cqm_threshold /= r->mon_scale;
+
+	ret = dom_data_init(r);
+	if (ret)
+		goto out;

arch/x86/kernel/cpu/intel_rdt_monitor.c line 160

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt_monitor.c new file +10 -0

+
+	l3_mon_evt_init(r);
+
+	r->mon_capable = true;
+	r->mon_enabled = true;
+
+	return;
+out:
+	kfree(rmid_ptrs);
+	rdt_mon_features = 0;

This is silly. if dom_data_init() fails, then it failed because it was unable to allocate rmid_ptrs. .....

Also clearing rdt_mod_features here is conceptually wrong. Make that function return int, i.e. the failure value, and clear rdt_mon_capable at the call site in case of error.

Thanks,

tglx

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

+
+	/*
+	 * Aside from the ERROR and UNAVAIL bits, the return value is the
+	 * count for this @eventid tagged with @rmid.
+	 */
+	return val;
+}
+
+/*

arch/x86/kernel/cpu/intel_rdt_monitor.c +23 -0

+		val = __rmid_read(entry->rmid, QOS_L3_OCCUP_EVENT_ID);
+		if (val > intel_cqm_threshold)
+			entry->state = RMID_DIRTY;
+	}
+}
+
+/*
+ * Scan the limbo list and move all entries that are below the
+ * intel_cqm_threshold to the free list.
+ * Return "true" if the limbo list is empty, "false" if there are
+ * still some RMIDs there.
+ */
+static bool try_freeing_limbo_rmid(void)
+{
+	struct rmid_entry *entry, *tmp;
+	struct rdt_resource *r;
+	cpumask_var_t cpu_mask;
+	struct rdt_domain *d;
+	bool ret = true;
+
+	if (list_empty(&rmid_limbo_lru))
+		return ret;
+

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

+		 */
+		if (entry->state == RMID_DIRTY) {
+			entry->state = RMID_CHECK;
+			ret = false;
+			continue;
+		}
+		list_del(&entry->list);
+		list_add_tail(&entry->list, &rmid_free_lru);
+	}
+
+	free_cpumask_var(cpu_mask);

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

+void free_rmid(u32 rmid)
+{
+	struct rmid_entry *entry;
+
+	lockdep_assert_held(&rdtgroup_mutex);

Thinking a bit more about that limbo mechanics.

In case that a RMID was never used on a particular package, the state check forces an IPI on all packages unconditionally. That's suboptimal at least.

We know on which package a given RMID was used, so we could restrict the checks to exactly these packages, but I'm not sure it's worth the trouble. We might at least document that and explain why this is implemented in that way.

	if (static_branch_likely(&rdt_mon_enable_key)) {
		if (unlikely(current->rmid)) {
			newstate.rmid = current->rmid;
			__set_bit(newstate.rmid, this_cpu_ptr(rmid_bitmap));

		}
	}

Now in rmid_free() we can collect that information:

	cpumask_clear(&tmpmask);
	cpumask_clear(rmid_entry->mask);

	cpus_read_lock();
	for_each_online_cpu(cpu) {
		if (test_and_clear_bit(rmid, per_cpu_ptr(cpu, rmid_bitmap)))

			cpumask_set(cpu, tmpmask);
	}

	for_each_domain(d, resource) {
		cpu = cpumask_any_and(d->cpu_mask, tmpmask);
		if (cpu < nr_cpu_ids)
			cpumask_set(cpu, rmid_entry->mask);
	}

	list_add(&rmid_entry->list, &limbo_list);

	for_each_cpu(cpu, rmid_entry->mask)
		schedule_delayed_work_on(cpu, rmid_work);
	cpus_read_unlock();

The work function:

    	boot resched = false;

    	list_for_each_entry(rme, limbo_list,...) {
		if (!cpumask_test_cpu(cpu, rme->mask))
			continue;

		if (!rmid_is_reusable(rme)) {
			resched = true;
			continue;
		}

		cpumask_clear_cpu(cpu, rme->mask);
		if (!cpumask_empty(rme->mask))
			continue;

		/* Ready for reuse */
		list_del(rme->list);
		list_add(&rme->list, &free_list);
	}	

The alloc function then becomes:

	if (list_empty(&free_list))
		return list_empty(&limbo_list) ? -ENOSPC : -EBUSY;

The switch_to() covers the task rmids. The per cpu default rmids can be marked at the point where they are installed on a CPU in the per cpu rmid_bitmap. The free path is the same for per task and per cpu.

Another thing which needs some thought it the CPU hotplug code. We need to make sure that pending work which is scheduled on an outgoing CPU is moved in the offline callback to a still online CPU of the same domain and not moved to some random CPU by the workqueue hotplug code.

There is another subtle issue. Assume a RMID is freed. The limbo stuff is scheduled on all domains which have online CPUs.

Now the last CPU of a domain goes offline before the threshold for clearing the domain CPU bit in the rme->mask is reached.

So we have two options here:

1) Clear the bit unconditionally when the last CPU of a domain goes offline.

2) Arm a timer which clears the bit after a grace period

#1 The RMID might become available for reuse right away because all other
domains have not used it or have cleared their bits already.

If one of the CPUs of that domain comes online again and is associated to that reused RMID again, then the counter content might still contain leftovers from the previous usage.

#2 Prevents #1 but has it's own issues vs. serialization and coordination
with CPU hotplug.

I'd say we go for #1 as the simplest solution, document it and if really the need arises revisit it later.

Thanks,

tglx

Quoted 5 lines · Thomas Gleixner

	if (static_branch_likely(&rdt_mon_enable_key)) {
		if (unlikely(current->rmid)) {
			newstate.rmid = current->rmid;
			__set_bit(newstate.rmid, this_cpu_ptr(rmid_bitmap));

Non atomic op

Quoted 5 lines

		}
	}

Now in rmid_free() we can collect that information:

Quoted 6 lines

	cpumask_clear(&tmpmask);
	cpumask_clear(rmid_entry->mask);

	cpus_read_lock();
	for_each_online_cpu(cpu) {
		if (test_and_clear_bit(rmid, per_cpu_ptr(cpu, rmid_bitmap)))

atomic op

Quoted 6 lines

			cpumask_set(cpu, tmpmask);
	}

Another thing which needs some thought it the CPU hotplug code. We need to make sure that pending work which is scheduled on an outgoing CPU is moved in the offline callback to a still online CPU of the same domain and not moved to some random CPU by the workqueue hotplug code.

just flush the workqueue for that CPU? That's what the workqueue core _should_ do in any case. And that also covers the case where @cpu is the last in the set of CPUs we could run on.

Quoted 6 lines

There is another subtle issue. Assume a RMID is freed. The limbo stuff is scheduled on all domains which have online CPUs.

Now the last CPU of a domain goes offline before the threshold for clearing the domain CPU bit in the rme->mask is reached.

Quoted 4 lines

So we have two options here:

1) Clear the bit unconditionally when the last CPU of a domain goes offline.

Arguably this. This is cache level stuff, that means this is the last CPU of a cache, so just explicitly kill the _entire_ cache and insta mark everything good again; WBINVD ftw.

Quoted 5 lines

2) Arm a timer which clears the bit after a grace period

#1 The RMID might become available for reuse right away because all other
domains have not used it or have cleared their bits already.

Quoted 3 lines

If one of the CPUs of that domain comes online again and is associated to that reused RMID again, then the counter content might still contain leftovers from the previous usage.

Not if we kill the cache on offline -- also, if all CPUs have been offline, its not too weird to expect something like a package idle state to have happened and shot down the caches anyway.

Quoted 9 lines · Thomas Gleixner

Thinking a bit more about that limbo mechanics.

In case that a RMID was never used on a particular package, the state check forces an IPI on all packages unconditionally. That's suboptimal at least.

We know on which package a given RMID was used, so we could restrict the checks to exactly these packages, but I'm not sure it's worth the trouble. We might at least document that and explain why this is implemented in that way.

Second thoughts on that. The allocation logic is:

Quoted 9 lines

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

+       if (list_empty(&rmid_free_lru)) {
+               ret = try_freeing_limbo_rmid();
+               if (list_empty(&rmid_free_lru))
+                       return ret ? -ENOSPC : -EBUSY;
+       }
+
+       entry = list_first_entry(&rmid_free_lru,
+                                struct rmid_entry, list);
+       list_del(&entry->list);

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+       return entry->rmid;

That means, the free list is used as the primary source. One of my boxes has 143 RMIDs. So it only takes 142 mkdir/rmdir invocations to move all RMIDs to the limbo list. On the next mkdir invocation the allocation goes into the limbo path and the SMP function call has to walk the list with 142 entries on ALL online domains whether they used the RMID or not!

That's bad enough already and the number of RMIDs will not become smaller; it doubled from HSW to BDW ...

The HPC and RT folks will love you for that - NOT!

So this needs to be solved differently.

Let's have a look at the context switch path first. That's the most sensitive part of it.

	if (static_branch_likely(&rdt_mon_enable_key)) {
		if (current->rmid)
			newstate.rmid = current->rmid;
	}

That's optimized for the !monitoring case. So we can really penalize the per task monitoring case.

	if (static_branch_likely(&rdt_mon_enable_key)) {
		if (unlikely(current->rmid)) {
			newstate.rmid = current->rmid;
			__set_bit(newstate.rmid, this_cpu_ptr(rmid_bitmap));
		}
	}

Now in rmid_free() we can collect that information:

	cpumask_clear(&tmpmask);
	cpumask_clear(rmid_entry->mask);

	cpus_read_lock();
	for_each_online_cpu(cpu) {
		if (test_and_clear_bit(rmid, per_cpu_ptr(cpu, rmid_bitmap)))
			cpumask_set(cpu, tmpmask);
	}

	for_each_domain(d, resource) {
		cpu = cpumask_any_and(d->cpu_mask, tmpmask);
		if (cpu < nr_cpu_ids)
			cpumask_set(cpu, rmid_entry->mask);

	}

	list_add(&rmid_entry->list, &limbo_list);

	for_each_cpu(cpu, rmid_entry->mask)
		schedule_delayed_work_on(cpu, rmid_work);
	cpus_read_unlock();

The work function:

   	boot resched = false;

   	list_for_each_entry(rme, limbo_list,...) {
		if (!cpumask_test_cpu(cpu, rme->mask))
			continue;

		if (!rmid_is_reusable(rme)) {
			resched = true;
			continue;
		}

		cpumask_clear_cpu(cpu, rme->mask);
		if (!cpumask_empty(rme->mask))
			continue;

		/* Ready for reuse */
		list_del(rme->list);
		list_add(&rme->list, &free_list);
	}

The alloc function then becomes:

	if (list_empty(&free_list))
		return list_empty(&limbo_list) ? -ENOSPC : -EBUSY;

The switch_to() covers the task rmids. The per cpu default rmids can be marked at the point where they are installed on a CPU in the per cpu rmid_bitmap. The free path is the same for per task and per cpu.

Another thing which needs some thought it the CPU hotplug code. We need to make sure that pending work which is scheduled on an outgoing CPU is moved in the offline callback to a still online CPU of the same domain and not moved to some random CPU by the workqueue hotplug code.

There is another subtle issue. Assume a RMID is freed. The limbo stuff is scheduled on all domains which have online CPUs.

Now the last CPU of a domain goes offline before the threshold for clearing the domain CPU bit in the rme->mask is reached.

So we have two options here:

1) Clear the bit unconditionally when the last CPU of a domain goes offline.

2) Arm a timer which clears the bit after a grace period

#1 The RMID might become available for reuse right away because all other
domains have not used it or have cleared their bits already.

If one of the CPUs of that domain comes online again and is associated to that reused RMID again, then the counter content might still contain leftovers from the previous usage.

#2 Prevents #1 but has it's own issues vs. serialization and coordination
with CPU hotplug.

I'd say we go for #1 as the simplest solution, document it and if really the need arises revisit it later.

Thanks,

tglx

Quoted 5 lines · Thomas Gleixner

That means, the free list is used as the primary source. One of my boxes has 143 RMIDs. So it only takes 142 mkdir/rmdir invocations to move all RMIDs to the limbo list. On the next mkdir invocation the allocation goes into the limbo path and the SMP function call has to walk the list with 142 entries on ALL online domains whether they used the RMID or not!

Would it be better if we do this in the MBM 1s overflow timer delayed_work? That is not in the interupt context. So we do a periodic flush of the limbo list and then mkdir fails with -EBUSY if list_empty(&free_list) && !list_empty(&limbo_list).

To improve that -
We may also include the optimization Tony suggested to skip the checks for RMIDs which are already checked to be < threshold (however that needs a domain mask like I mention below but may be we can just check the list here).

Quoted 5 lines

	for_each_domain(d, resource) {
		cpu = cpumask_any_and(d->cpu_mask, tmpmask);
		if (cpu < nr_cpu_ids)
			cpumask_set(cpu, rmid_entry->mask);

When this cpu goes offline - the rmid_entry->mask needs an update. Otherwise, the work function would return true for

             if (!cpumask_test_cpu(cpu, rme->mask))

since the work may have been moved to a different cpu.

So we really need a package mask ? or really a per-domain mask and for that we dont know the max domain number(which is why we use a list..)

In case that a RMID was never used on a particular package, the state check forces an IPI on all packages unconditionally. That's suboptimal at least.

We know on which package a given RMID was used, so we could restrict the checks to exactly these packages, but I'm not sure it's worth the trouble. We might at least document that and explain why this is implemented in that way.

Quoted 7 lines

In case that a RMID was never used on a particular package, the state check forces an IPI on all packages unconditionally. That's suboptimal at least.

We know on which package a given RMID was used, so we could restrict the checks to exactly these packages, but I'm not sure it's worth the trouble. We might at least document that and explain why this is implemented in that way.

We only allocate RMIDs when a user makes a directory. I don't think we should consider options that slow down context switch in order to keep track of which packages were used just to make mkdir(2) a bit faster in the case where we need to check the limbo list.

We could make the check of the limbo list less costly by using a bitmask to keep track of which packages have already found that the llc_occupancy is below the threshold. But I'd question whether the extra complexity in the code was really worth it.

arch/x86/kernel/cpu/intel_rdt.c struct rdt_resource rdt_resources_all[] = { · line 86

Quoted 6 lines · Vikas Shivappa

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

@@ -82,6 +82,7 @@ struct rdt_resource rdt_resources_all[] = {
 		},
 		.parse_ctrlval		= parse_cbm,
 		.format_str		= "%d=%0*x",
+		.fflags			= RFTYPE_RES_CACHE,
 	},

Can you please convert that array to use explicit array member initializers? I've noticed this back when I reviewed the intial RDT implementation, but it somehow escaped. i.e.:

     [RESOURCE_ID] =
     {
	.....
     }

Thanks,

tglx

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +11 -21

-	struct rdtgroup *parent, *rdtgrp;
+	struct rdtgroup *prgrp, *rdtgrp;
 	struct kernfs_node *kn;
-	int ret, closid;
-
-	/* Only allow mkdir in the root directory */
-	if (parent_kn != rdtgroup_default.kn)
-		return -EPERM;
-
-	/* Do not accept '\n' to avoid unparsable situation. */
-	if (strchr(name, '\n'))
-		return -EINVAL;
+	uint fshift = 0;
+	int ret;
 
-	parent = rdtgroup_kn_lock_live(parent_kn);
-	if (!parent) {
+	prgrp = rdtgroup_kn_lock_live(prkn);
+	if (!prgrp) {
 		ret = -ENODEV;
 		goto out_unlock;
 	}
 
-	ret = closid_alloc();
-	if (ret < 0)
-		goto out_unlock;
-	closid = ret;
-
 	/* allocate the rdtgroup. */
 	rdtgrp = kzalloc(sizeof(*rdtgrp), GFP_KERNEL);
 	if (!rdtgrp) {
 		ret = -ENOSPC;
-		goto out_closid_free;
+		goto out_unlock;
 	}
-	rdtgrp->closid = closid;
-	list_add(&rdtgrp->rdtgroup_list, &rdt_all_groups);
+	*r = rdtgrp;
+	rdtgrp->parent = prgrp;
+	rdtgrp->type = rtype;
+	INIT_LIST_HEAD(&rdtgrp->crdtgrp_list);
 
 	/* kernfs creates the directory for rdtgrp */
-	kn = kernfs_create_dir(parent->kn, name, mode, rdtgrp);
+	kn = kernfs_create_dir(pkn, name, mode, rdtgrp);
 	if (IS_ERR(kn)) {
 		ret = PTR_ERR(kn);
 		goto out_cancel_ref;

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

 	if (ret)
 		goto out_destroy;
 
+	if (rdt_mon_features) {

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

+	return 0;
 
 out_destroy:
 	kernfs_remove(rdtgrp->kn);
 out_cancel_ref:
-	list_del(&rdtgrp->rdtgroup_list);
 	kfree(rdtgrp);
-out_closid_free:

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

+	kfree(rgrp);
+}
 
+static int rdtgroup_mkdir(struct kernfs_node *pkn, const char *name,
+			  umode_t mode)
+{
+	/* Do not accept '\n' to avoid unparsable situation. */
+	if (strchr(name, '\n'))
+		return -EINVAL;
+
+	/*
+	 * We don't allow rdtgroup ctrl_mon directories to be created anywhere
+	 * except the root directory and dont allow rdtgroup monitor

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static void rdt_kill_sb(struct super_block *sb) · line 1123

Quoted 5 lines · Vikas Shivappa

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

+/*
+ * Common code for ctrl_mon and monitor group mkdir.
+ * The caller needs to unlock the global mutex upon success.
+ */
+static int mkdir_rdt_common(struct kernfs_node *pkn, struct kernfs_node *prkn,

pkn and prkn are horrible to distinguish. What's wrong with keeping *parent_kn and have *kn as the new thing?

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static void rdt_kill_sb(struct super_block *sb) · line 1126

Quoted 4 lines

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

+			    const char *name, umode_t mode,
+			    enum rdt_group_type rtype, struct rdtgroup **r)
 {

Can you please split out that mkdir_rdt_common() change into a separate patch? It can be done as a preparatory stand alone change just for the existing rdt group code. Then the monitoring add ons come on top of it.

Quoted 49 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +11 -21

-	struct rdtgroup *parent, *rdtgrp;
+	struct rdtgroup *prgrp, *rdtgrp;
 	struct kernfs_node *kn;
-	int ret, closid;
-
-	/* Only allow mkdir in the root directory */
-	if (parent_kn != rdtgroup_default.kn)
-		return -EPERM;
-
-	/* Do not accept '\n' to avoid unparsable situation. */
-	if (strchr(name, '\n'))
-		return -EINVAL;
+	uint fshift = 0;
+	int ret;
 
-	parent = rdtgroup_kn_lock_live(parent_kn);
-	if (!parent) {
+	prgrp = rdtgroup_kn_lock_live(prkn);
+	if (!prgrp) {
 		ret = -ENODEV;
 		goto out_unlock;
 	}
 
-	ret = closid_alloc();
-	if (ret < 0)
-		goto out_unlock;
-	closid = ret;
-
 	/* allocate the rdtgroup. */
 	rdtgrp = kzalloc(sizeof(*rdtgrp), GFP_KERNEL);
 	if (!rdtgrp) {
 		ret = -ENOSPC;
-		goto out_closid_free;
+		goto out_unlock;
 	}
-	rdtgrp->closid = closid;
-	list_add(&rdtgrp->rdtgroup_list, &rdt_all_groups);
+	*r = rdtgrp;
+	rdtgrp->parent = prgrp;
+	rdtgrp->type = rtype;
+	INIT_LIST_HEAD(&rdtgrp->crdtgrp_list);
 
 	/* kernfs creates the directory for rdtgrp */
-	kn = kernfs_create_dir(parent->kn, name, mode, rdtgrp);
+	kn = kernfs_create_dir(pkn, name, mode, rdtgrp);
 	if (IS_ERR(kn)) {
 		ret = PTR_ERR(kn);
 		goto out_cancel_ref;

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name, · line 1170

Quoted 6 lines

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

@@ -1138,27 +1166,138 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
 	if (ret)
 		goto out_destroy;
 
-	ret = rdtgroup_add_files(kn, RF_CTRL_BASE);
+	fshift = 1 << (RF_CTRLSHIFT + rtype);
+	ret = rdtgroup_add_files(kn, RFTYPE_BASE | fshift);

I'd rather make this:

  	files = RFTYPE_BASE | (1U << (RF_CTRLSHIFT + rtype));
	ret = rdtgroup_add_files(kn, files);

Quoted 4 lines

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

 	if (ret)
 		goto out_destroy;
 
+	if (rdt_mon_features) {

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name, · line 1148

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +6 -2

+		ret = alloc_rmid();
+		if (ret < 0)
+			return ret;
+
+		rdtgrp->rmid = ret;
+	}
 	kernfs_activate(kn);
 
-	ret = 0;
-	goto out_unlock;

What unlocks prkn now? The caller, right? Please add a comment ...

Quoted 12 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +5 -2

+	return 0;
 
 out_destroy:
 	kernfs_remove(rdtgrp->kn);
 out_cancel_ref:
-	list_del(&rdtgrp->rdtgroup_list);
 	kfree(rdtgrp);
-out_closid_free:
+out_unlock:
+	rdtgroup_kn_unlock(prkn);
+	return ret;
+}

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name, · line 1198

Quoted 6 lines

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

+
+static void mkdir_rdt_common_clean(struct rdtgroup *rgrp)
+{
+	kernfs_remove(rgrp->kn);
+	if (rgrp->rmid)
+		free_rmid(rgrp->rmid);

Please put that conditonal into free_rmid().

Quoted 14 lines

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

+	kfree(rgrp);
+}
 
+static int rdtgroup_mkdir(struct kernfs_node *pkn, const char *name,
+			  umode_t mode)
+{
+	/* Do not accept '\n' to avoid unparsable situation. */
+	if (strchr(name, '\n'))
+		return -EINVAL;
+
+	/*
+	 * We don't allow rdtgroup ctrl_mon directories to be created anywhere
+	 * except the root directory and dont allow rdtgroup monitor

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name, · line 1298

Quoted 10 lines

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

+	 * directories to be created anywhere execept inside mon_groups
+	 * directory.
+	 */
+	if (rdt_alloc_enabled && pkn == rdtgroup_default.kn)
+		return rdtgroup_mkdir_ctrl_mon(pkn, pkn, name, mode);
+	else if (rdt_mon_features &&
+		 !strcmp(pkn->name, "mon_groups"))
+		return rdtgroup_mkdir_mon(pkn, pkn->parent, name, mode);
+	else
+		return -EPERM;

TBH, this is really convoluted (including the comment).

	/*
	 * If the parent directory is the root directory and RDT
	 * allocation is supported, add a control and monitoring
	 * subdirectory.
	 */
	if (rdt_alloc_capable && parent_kn == rdtgroup_default.kn)
		return rdtgroup_mkdir_ctrl_mon(...);

	/*
	 * If the parent directory is a monitoring group and RDT
	 * monitoring is supported, add a monitoring subdirectory.
	 */
	 if (rdt_mon_capable && is_mon_group(parent_kn))
		return rdtgroup_mkdir_mon(...);

	 return -EPERM;

Note, that I did not use strcmp(parent_kn->name) because that's simply not sufficient. What prevents a user from doing:

# mkdir /sys/fs/resctrl/mon_group/mon_group
# mkdir /sys/fs/resctrl/mon_group/mon_group/foo

You need a better way to distignuish that than strcmp(). You probably want to prevent creating subdirectories named "mon_group" as well.

Thanks,

tglx

include/linux/sched.h struct task_struct { · line 869

Quoted 5 lines · Vikas Shivappa

include/linux/sched.h +1 -0

@@ -866,6 +866,7 @@ struct task_struct {
 #endif
 #ifdef CONFIG_INTEL_RDT
 	int				closid;
+	u32				rmid;

Can you please make a preparatory change which makes closid an u32 as well? We should have done that in the first place, but in hindsight we are always smarter...

Thanks,

tglx

arch/x86/kernel/cpu/intel_rdt.h struct mon_evt { · line 40

Quoted 9 lines · Vikas Shivappa

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

index fdf3654..fec8ba9 100644
@@ -37,6 +37,8 @@ struct mon_evt {
 extern bool rdt_alloc_enabled;
 extern int rdt_mon_features;
 
+DECLARE_PER_CPU_READ_MOSTLY(int, cpu_rmid);

u32

arch/x86/kernel/cpu/intel_rdt.h struct mon_evt { · line 42

Quoted 3 lines

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

+DEFINE_PER_CPU_READ_MOSTLY(int, cpu_rmid);
 static inline struct rmid_entry *__rmid_entry(u32 rmid)

Bah. Please add a new line between the DEFINE... and the function.

But that whole thing is wrong. The per cpu default closid and rmid want to be in a single place, not in two distinct per cpu variables.

struct rdt_cpu_default {
      u32	rmid;
      u32	closid;
};

DEFINE_PER_CPU_READ_MOSTLY(struct rdt_cpu_default, rdt_cpu_default);

or something like this. That way it's guaranteed that the context switch code touches a single cache line for the per cpu defaults.

Thanks,

tglx

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +1 -4

 	cpumask_var_t tmpmask, newmask;
-	struct rdtgroup *rdtgrp, *r;
+	struct list_head *llist;
 	int ret;
 
-	if (!buf)
-		return -EINVAL;
-
 	if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL))
 		return -ENOMEM;
 	if (!zalloc_cpumask_var(&newmask, GFP_KERNEL)) {

@@ -233,10 +235,89 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
 		return -ENOMEM;

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

+		list_for_each_entry(cr, llist, crdtgrp_list) {
+			if (cr == rdtgrp)
+				continue;
+			cpumask_andnot(&cr->cpu_mask, &cr->cpu_mask, tmpmask);
+		}
+		update_closid_rmid(tmpmask, rdtgrp);

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static void rdt_update_cpu_closid(void *closid) · line 224

Quoted 5 lines · Vikas Shivappa

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +3 -2

-static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
-				   char *buf, size_t nbytes, loff_t off)
+static ssize_t cpus_mon_write(struct kernfs_open_file *of,
+			      char *buf, size_t nbytes,
+			      struct rdtgroup *rdtgrp)

Again. Please make the split of rdtgroup_cpus_write() as a seperate preparatory change first and just move the guts of the existing write function out into cpus_ctrl_write() and then add the mon_write stuff as an extra patch.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static void rdt_update_cpu_closid(void *closid) · line 226

Quoted 2 lines

{

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

+	struct rdtgroup *pr = rdtgrp->parent, *cr;

*pr and *cr really suck.

Quoted 13 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +1 -4

 	cpumask_var_t tmpmask, newmask;
-	struct rdtgroup *rdtgrp, *r;
+	struct list_head *llist;
 	int ret;
 
-	if (!buf)
-		return -EINVAL;
-
 	if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL))
 		return -ENOMEM;
 	if (!zalloc_cpumask_var(&newmask, GFP_KERNEL)) {

@@ -233,10 +235,89 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
 		return -ENOMEM;

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, · line 241

Quoted 10 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +4 -4

 	}
 
-	rdtgrp = rdtgroup_kn_lock_live(of->kn);
-	if (!rdtgrp) {
-		ret = -ENOENT;
-		goto unlock;
+	if (is_cpu_list(of))
+		ret = cpulist_parse(buf, newmask);
+	else
+		ret = cpumask_parse(buf, newmask);

The cpuask allocation and parsing of the user buffer can be done in the common code. No point in duplicating that.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, · line 251

Quoted 10 lines

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

+
+	if (ret)
+		goto out;
+
+	/* check that user didn't specify any offline cpus */
+	cpumask_andnot(tmpmask, newmask, cpu_online_mask);
+	if (cpumask_weight(tmpmask)) {
+		ret = -EINVAL;
+		goto out;
+	}

Common code.

Quoted 10 lines

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

+	/* Check whether cpus belong to parent ctrl group */
+	cpumask_andnot(tmpmask, newmask, &pr->cpu_mask);
+	if (cpumask_weight(tmpmask)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Check whether cpus are dropped from this group */
+	cpumask_andnot(tmpmask, &rdtgrp->cpu_mask, newmask);

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, · line 264

Quoted 3 lines

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

+	if (cpumask_weight(tmpmask)) {
+		/* Give any dropped cpus to parent rdtgroup */
+		cpumask_or(&pr->cpu_mask, &pr->cpu_mask, tmpmask);

This does not make any sense. The check above verifies that all cpus in newmask belong to the parent->cpu_mask. If they don't then you return -EINVAL, but here you give them back to parent->cpu_mask. How is that supposed to work? You never get into this code path!

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, · line 264

Quoted 1 line

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

+		cpumask_or(&pr->cpu_mask, &pr->cpu_mask, tmpmask);

So you need a seperate mask in the parent rdtgroup to store the CPUs which are valid in any monitoring group which belongs to it. So the logic becomes:

	/*
	 * Check whether the CPU mask is a subset of the CPUs
	 * which belong to the parent group.
	 */
	cpumask_andnot(tmpmask, newmask, parent->cpus_valid_mask);
	if (cpumask_weight(tmpmask))
		return -EINVAL;

When CAT is not available, then parent->cpus_valid_mask is a pointer to cpu_online_mask. When CAT is enabled, then parent->cpus_valid_mask is a pointer to the CAT group cpu mask.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, · line 274

Quoted 11 lines

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

+		update_closid_rmid(tmpmask, pr);
+	}
+
+	/*
+	 * If we added cpus, remove them from previous group that owned them
+	 * and update per-cpu rmid
+	 */
+	cpumask_andnot(tmpmask, newmask, &rdtgrp->cpu_mask);
+	if (cpumask_weight(tmpmask)) {
+		llist = &pr->crdtgrp_list;

llist is a bad name. We have a facility llist, i.e. lockless list. head ?

Quoted 8 lines

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

+		list_for_each_entry(cr, llist, crdtgrp_list) {
+			if (cr == rdtgrp)
+				continue;
+			cpumask_andnot(&cr->cpu_mask, &cr->cpu_mask, tmpmask);
+		}
+		update_closid_rmid(tmpmask, rdtgrp);
+	}

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, · line 300

Quoted 8 lines

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

+static void cpumask_rdtgrp_clear(struct rdtgroup *r, struct cpumask *m)
+{
+	struct rdtgroup *cr;
+
+	cpumask_andnot(&r->cpu_mask, &r->cpu_mask, m);
+	/* update the child mon group masks as well*/
+	list_for_each_entry(cr, &r->crdtgrp_list, crdtgrp_list)
+		cpumask_and(&cr->cpu_mask, &r->cpu_mask, &cr->cpu_mask);

That's equally wrong. See above.

Thanks,

tglx

Quoted 9 lines · Thomas Gleixner

Quoted 9 lines

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

+	/* Check whether cpus belong to parent ctrl group */
+	cpumask_andnot(tmpmask, newmask, &pr->cpu_mask);
+	if (cpumask_weight(tmpmask)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Check whether cpus are dropped from this group */
+	cpumask_andnot(tmpmask, &rdtgrp->cpu_mask, newmask);

Quoted 8 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, · line 264

Quoted 3 lines

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

+	if (cpumask_weight(tmpmask)) {
+		/* Give any dropped cpus to parent rdtgroup */
+		cpumask_or(&pr->cpu_mask, &pr->cpu_mask, tmpmask);

This does not make any sense. The check above verifies that all cpus in newmask belong to the parent->cpu_mask. If they don't then you return -EINVAL, but here you give them back to parent->cpu_mask. How is that supposed to work? You never get into this code path!

The parent->cpu_mask always is the parent->cpus_valid_mask if i understand right. With monitor group, the cpu is present is always present in "one" ctrl_mon group and one mon_group. And the mon group can have only cpus in its parent. May be it needs a comment? (its explaind in the documentation patch).

# mkdir /sys/fs/resctrl/p1
# mkdir /sys/fs/resctrl/p1/mon_groups/m1
# echo 5-10 > /sys/fs/resctr/p1/cpus_list
Say p1 has RMID 2
cpus 5-10 have RMID 2

Quoted 19 lines · Shivappa Vikas

Quoted 9 lines · Thomas Gleixner

Quoted 9 lines

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

+	/* Check whether cpus belong to parent ctrl group */
+	cpumask_andnot(tmpmask, newmask, &pr->cpu_mask);
+	if (cpumask_weight(tmpmask)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Check whether cpus are dropped from this group */
+	cpumask_andnot(tmpmask, &rdtgrp->cpu_mask, newmask);

Quoted 8 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, · line 264

Quoted 3 lines

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

+	if (cpumask_weight(tmpmask)) {
+		/* Give any dropped cpus to parent rdtgroup */
+		cpumask_or(&pr->cpu_mask, &pr->cpu_mask, tmpmask);

This does not make any sense. The check above verifies that all cpus in newmask belong to the parent->cpu_mask. If they don't then you return -EINVAL, but here you give them back to parent->cpu_mask. How is that supposed to work? You never get into this code path!

Quoted 4 lines

The parent->cpu_mask always is the parent->cpus_valid_mask if i understand right. With monitor group, the cpu is present is always present in "one" ctrl_mon group and one mon_group. And the mon group can have only cpus in its parent. May be it needs a comment? (its explaind in the documentation patch).

Sigh, the code needs to be written in a way that it is halfways obvious what's going on.

Quoted 5 lines

# mkdir /sys/fs/resctrl/p1
# mkdir /sys/fs/resctrl/p1/mon_groups/m1
# echo 5-10 > /sys/fs/resctr/p1/cpus_list
Say p1 has RMID 2
cpus 5-10 have RMID 2

So what you say, is that parent is always the resource control group itself.

Can we please have a proper distinction in the code? I tripped over that ambigiousities several times.

The normal meaning of parent->child relations is that both have the same type. While this is the case at the implementation detail level (both are type struct rdtgroup), from a conceptual level they are different:

parent is a resource group and child is a monitoring group

That should be expressed in the code, at the very least by variable naming, so it becomes immediately clear that this operates on two different entities.

The proper solution is to have different data types or at least embedd the monitoring bits in a seperate entity inside of struct rdtgroup.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static void rdt_update_cpu_closid(void *closid) · line 226

Quoted 2 lines

{

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

+	struct rdtgroup *pr = rdtgrp->parent, *cr;

*pr and *cr really suck.