x86/resctrl: Fix a few issues in moving a task to a resource group

v1 submission at:
https://lore.kernel.org/lkml/[email protected]/

Changes since v1:
* Collect Reviewed-by tags.
* Stop using task_struct->on_cpu and use existing task_curr() helper instead. (James) This has two consequences:
* The reason task_struct->on_cpu is no longer used is because it may trigger unnecessary IPI. Add a new fix patch that replaces task_struct->on_cpu in existing usages with task_curr(). (James)
* The original first patch that introduced some helpers that used task_struct->on_cpu is removed. task_curr() is used directly in new version without additional helpers.
* Improve subject line of original patch #2 (now patch #1). (James)
* Use IS_ENABLED() instead of #ifdef to check for Kconfig symbol. (James)
* Remove check for impossible condition. Resource groups are only created of type RDTCTRL_GROUP or RDTMON_GROUP, there is no purpose to check for a third group type. This unnecessary check was added in this series but unrelated to this fix, removing this check maintains original behavior. This is a change that merges feedback from both James and Valentin about this snippet.
* Re-evaluation of memory barriers by Valentin is complete:
* Include barrier() after closid/rmid are written before using task_curr() to decide if task needs interrupting. (Valentin)
* Add new patch from Valentin that prevents load/store tearing if a task is moved while it is scheduled. (Valentin)
* Fixup subject line x86/intel_rdt -> x86/resctrl. (Reinette)
* Highlighting an update to cover letter below: Patch 3 and Patch 4 from this version do not meet the criteria in stable-kernel-rules.rst of "It must fix a real bug that bothers people ..." and thus do not contain a Fixes tag nor are they Cc'd to stable.

---

When a user writes a task id to the resctrl "tasks" file the task will be moved to the resource group to which the destination "tasks" file belongs. Primarily this includes updating the task's closid and rmid in its task_struct and, for a running task, setting these new values in the PQR_ASSOC register that reflects the active closid and rmid on a CPU.

Moving a task to a new resource group is currently accomplished by updating its task_struct and queueing the work that updates the PQR_ASSOC register. This queued work will be run as soon as possible if the task is running or if the task is not running the queued work will be run when the task exits the kernel and returns to user mode (task_work_add(...,..., TWA_RESUME)).

Updating the PQR_ASSOC MSR as soon as possible on the CPU a moved task is running is the right thing to do. Queueing work for a task that is not running is unnecessary (the PQR_ASSOC MSR is already updated when the task is scheduled in) and causing system resource waste with the way in which it is implemented: Work to update the PQR_ASSOC register is queued every time the user writes a task id to the "tasks" file, even if the task already belongs to the resource group. This could result in multiple pending work items associated with a single task even if they are all identical and even though only a single update with most recent values is needed. Specifically, even if a task is moved between different resource groups while it is sleeping, then it is only the last move that is relevant but yet a work item is queued during each move.
This unnecessary queueing of work items could result in significant system resource waste, especially on tasks sleeping for a long time. For example, as demonstrated by Shakeel Butt in [1] writing the same task id to the "tasks" file can quickly consume significant memory. The same problem (wasted system resources) occurs when moving a task between different resource groups. As pointed out by Valentin Schneider in [2] there is an additional issue with the way in which the queueing of work is done in that the task_struct update is currently done after the work is queued, resulting in a race with the register update possibly done before the data needed by the update is available.

This series fixes all the above issues related to the queueing of the updates to the PQR_ASSOC register.

There is already a way in which resctrl moves tasks that can guide towards a solution. Within resctrl tasks are also moved between resource groups when a resource group is removed, also an action initiated by the user with "rmdir" of the resource group directory. In this case resctrl moves all tasks belonging to the removed group to the default resource group (in the case of a control group) or the parent resource group (in the case of a monitor resource group). These task moves are handled synchronously by resctrl with an immediate update of the PQR_ASSOC register on the CPU the task is currently running.

This fix follows the same update mechanism used as when resource groups are removed. The task's closid and rmid is updated in its task_struct as before. Instead of queueing work to update the PQR_ASSOC register with the new values this update is done immediately on the CPU where the task is currently running. If the task is not running there is no action since the register will be updated when the task is scheduled in.

* Patch 1 updates the PQR_ASSOC MSR in synchronous way instead of in a callback.
* Patch 2 fixes the issue of unnecessary work when a task move is not needed (when user writes a task id to a "tasks" file to which it already belongs) by adding sanity checking to avoid costly move operations in the same resource group.
* Patch 3 replaces remaining usages of task_struct->on_cpu with * task_curr().
* Patch 4 prevents load/store tearing if a task is moved while
it is scheduled. (Valentin)

Patch 3 and Patch 4 do not meet the criteria in stable-kernel-rules.rst of "It must fix a real bug that bothers people ..." and thus do not contain a Fixes tag nor are they submitted to stable.

[1]: https://lore.kernel.org/lkml/CALvZod7E9zzHwenzf7objzGKsdBmVwTgEJ0nPgs0LUFU3SN5Pw@mail.gmail.com/
[2]: https://lore.kernel.org/lkml/[email protected]

Fenghua Yu (2):
x86/resctrl: Use IPI instead of task_work_add() to update PQR_ASSOC
MSR
x86/resctrl: Don't move a task to the same resource group

Reinette Chatre (1):
x86/resctrl: Use task_curr() instead of task_struct->on_cpu to prevent
unnecessary IPI

Valentin Schneider (1):
x86/resctrl: Apply READ_ONCE/WRITE_ONCE to task_struct .rmid & .closid

 arch/x86/include/asm/resctrl.h         |  11 +-
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 143 +++++++++++--------------
 2 files changed, 70 insertions(+), 84 deletions(-)

base-commit: 2c85ebc57b3e1817b6ce1a6b703928e113a90442

2.26.2

From: Fenghua Yu <[email protected]>

Currently when moving a task to a resource group the PQR_ASSOC MSR is updated with the new closid and rmid in an added task callback. If the task is running the work is run as soon as possible. If the task is not running the work is executed later in the kernel exit path when the kernel returns to the task again.

Updating the PQR_ASSOC MSR as soon as possible on the CPU a moved task is running is the right thing to do. Queueing work for a task that is not running is unnecessary (the PQR_ASSOC MSR is already updated when the task is scheduled in) and causing system resource waste with the way in which it is implemented: Work to update the PQR_ASSOC register is queued every time the user writes a task id to the "tasks" file, even if the task already belongs to the resource group. This could result in multiple pending work items associated with a single task even if they are all identical and even though only a single update with most recent values is needed. Specifically, even if a task is moved between different resource groups while it is sleeping then it is only the last move that is relevant but yet a work item is queued during each move. This unnecessary queueing of work items could result in significant system resource waste, especially on tasks sleeping for a long time. For example, as demonstrated by Shakeel Butt in [1] writing the same task id to the "tasks" file can quickly consume significant memory. The same problem (wasted system resources) occurs when moving a task between different resource groups.

As pointed out by Valentin Schneider in [2] there is an additional issue with the way in which the queueing of work is done in that the task_struct update is currently done after the work is queued, resulting in a race with the register update possibly done before the data needed by the update is available.

To solve these issues, the PQR_ASSOC MSR is updated in a synchronous way right after the new closid and rmid are ready during the task movement, only if the task is running. If a moved task is not running nothing is done since the PQR_ASSOC MSR will be updated next time the task is scheduled. This is the same way used to update the register when tasks are moved as part of resource group removal.

[1] https://lore.kernel.org/lkml/CALvZod7E9zzHwenzf7objzGKsdBmVwTgEJ0nPgs0LUFU3SN5Pw@mail.gmail.com/
[2] https://lore.kernel.org/lkml/[email protected]

---
V1->V2:
* Add Reviewed-by tags.
* Use task_curr() instead of task_struct->on_cpu (James)
* Fixup subject line (James)
* Remove unnecessary check for non-existent resource group type. (James and Valentin)
* Include barrier() after closid/rmid written. (Valentin)

 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 118 ++++++++++---------------
 1 file changed, 49 insertions(+), 69 deletions(-)

index f3418428682b..c5937a12d731 100644
@@ -525,89 +525,69 @@ static void rdtgroup_remove(struct rdtgroup *rdtgrp)
 	kfree(rdtgrp);
 }
 
-struct task_move_callback {
-	struct callback_head	work;
-	struct rdtgroup		*rdtgrp;
-};
-
-static void move_myself(struct callback_head *head)
+static void _update_task_closid_rmid(void *task)
 {
-	struct task_move_callback *callback;
-	struct rdtgroup *rdtgrp;
-
-	callback = container_of(head, struct task_move_callback, work);
-	rdtgrp = callback->rdtgrp;
-
 	/*
-	 * If resource group was deleted before this task work callback
-	 * was invoked, then assign the task to root group and free the
-	 * resource group.
+	 * If the task is still current on this CPU, update PQR_ASSOC MSR.
+	 * Otherwise, the MSR is updated when the task is scheduled in.
 	 */
-	if (atomic_dec_and_test(&rdtgrp->waitcount) &&
-	    (rdtgrp->flags & RDT_DELETED)) {
-		current->closid = 0;
-		current->rmid = 0;
-		rdtgroup_remove(rdtgrp);
-	}
-
-	if (unlikely(current->flags & PF_EXITING))
-		goto out;
-
-	preempt_disable();
-	/* update PQR_ASSOC MSR to make resource group go into effect */
-	resctrl_sched_in();
-	preempt_enable();
+	if (task == current)
+		resctrl_sched_in();
+}
 
-out:
-	kfree(callback);
+#ifdef CONFIG_SMP
+static void update_task_closid_rmid(struct task_struct *t)
+{
+	if (task_curr(t))
+		smp_call_function_single(task_cpu(t), _update_task_closid_rmid,
+					 t, 1);
 }
+#else
+static void update_task_closid_rmid(struct task_struct *t)
+{
+	_update_task_closid_rmid(t);
+}
+#endif
 
 static int __rdtgroup_move_task(struct task_struct *tsk,
 				struct rdtgroup *rdtgrp)
 {
-	struct task_move_callback *callback;
-	int ret;
-
-	callback = kzalloc(sizeof(*callback), GFP_KERNEL);
-	if (!callback)
-		return -ENOMEM;
-	callback->work.func = move_myself;
-	callback->rdtgrp = rdtgrp;
-
 	/*
-	 * Take a refcount, so rdtgrp cannot be freed before the
-	 * callback has been invoked.
+	 * Set the task's closid/rmid before the PQR_ASSOC MSR can be
+	 * updated by them.
+	 *
+	 * For ctrl_mon groups, move both closid and rmid.
+	 * For monitor groups, can move the tasks only from
+	 * their parent CTRL group.
 	 */
-	atomic_inc(&rdtgrp->waitcount);
-	ret = task_work_add(tsk, &callback->work, TWA_RESUME);
-	if (ret) {
-		/*
-		 * Task is exiting. Drop the refcount and free the callback.
-		 * No need to check the refcount as the group cannot be
-		 * deleted before the write function unlocks rdtgroup_mutex.
-		 */
-		atomic_dec(&rdtgrp->waitcount);
-		kfree(callback);
-		rdt_last_cmd_puts("Task exited\n");
-	} else {
-		/*
-		 * For ctrl_mon groups move both closid and rmid.
-		 * For monitor groups, can move the tasks only from
-		 * their parent CTRL group.
-		 */
-		if (rdtgrp->type == RDTCTRL_GROUP) {
-			tsk->closid = rdtgrp->closid;
+
+	if (rdtgrp->type == RDTCTRL_GROUP) {
+		tsk->closid = rdtgrp->closid;
+		tsk->rmid = rdtgrp->mon.rmid;
+	} else if (rdtgrp->type == RDTMON_GROUP) {
+		if (rdtgrp->mon.parent->closid == tsk->closid) {
 			tsk->rmid = rdtgrp->mon.rmid;
-		} else if (rdtgrp->type == RDTMON_GROUP) {
-			if (rdtgrp->mon.parent->closid == tsk->closid) {
-				tsk->rmid = rdtgrp->mon.rmid;
-			} else {
-				rdt_last_cmd_puts("Can't move task to different control group\n");
-				ret = -EINVAL;
-			}
+		} else {
+			rdt_last_cmd_puts("Can't move task to different control group\n");
+			return -EINVAL;
 		}
 	}
-	return ret;
+
+	/*
+	 * Ensure the task's closid and rmid are written before determining if
+	 * the task is current that will decide if it will be interrupted.
+	 */
+	barrier();
+
+	/*
+	 * By now, the task's closid and rmid are set. If the task is current
+	 * on a CPU, the PQR_ASSOC MSR needs to be updated to make the resource
+	 * group go into effect. If the task is not current, the MSR will be
+	 * updated when the task is scheduled in.
+	 */
+	update_task_closid_rmid(tsk);
+
+	return 0;
 }
 
 static bool is_closid_match(struct task_struct *t, struct rdtgroup *r)

2.26.2

Hi Borislav,

There is no particular reason. What you propose is much more readable. Thank you very much.

Reinette

From: Fenghua Yu <[email protected]>

Shakeel Butt reported in [1] that a user can request a task to be moved to a resource group even if the task is already in the group. It just wastes time to do the move operation which could be costly to send IPI to a different CPU.

Add a sanity check to ensure that the move operation only happens when the task is not already in the resource group.

[1] https://lore.kernel.org/lkml/CALvZod7E9zzHwenzf7objzGKsdBmVwTgEJ0nPgs0LUFU3SN5Pw@mail.gmail.com/

---
V1->V2:
* No changes

 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 7 +++++++
 1 file changed, 7 insertions(+)

index c5937a12d731..4042e1eb4f5d 100644
@@ -552,6 +552,13 @@ static void update_task_closid_rmid(struct task_struct *t)
 static int __rdtgroup_move_task(struct task_struct *tsk,
 				struct rdtgroup *rdtgrp)
 {
+	/* If the task is already in rdtgrp, no need to move the task. */
+	if ((rdtgrp->type == RDTCTRL_GROUP && tsk->closid == rdtgrp->closid &&
+	     tsk->rmid == rdtgrp->mon.rmid) ||
+	    (rdtgrp->type == RDTMON_GROUP && tsk->rmid == rdtgrp->mon.rmid &&
+	     tsk->closid == rdtgrp->mon.parent->closid))
+		return 0;
+
 	/*
 	 * Set the task's closid/rmid before the PQR_ASSOC MSR can be
 	 * updated by them.

2.26.2

James reported in [1] that there could be two tasks running on the same CPU with task_struct->on_cpu set. Using task_struct->on_cpu as a test if a task is running on a CPU may thus match the old task for a CPU while the scheduler is running and IPI it unnecessarily.

task_curr() is the correct helper to use. While doing so move the #ifdef check of the CONFIG_SMP symbol to be a C conditional used to determine if this helper should be used to ensure the code is always checked for correctness by the compiler.

[1] https://lore.kernel.org/lkml/[email protected]

---
V1->V2:
* New patch in series

 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

index 4042e1eb4f5d..9bd36210d220 100644
@@ -2319,19 +2319,15 @@ static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to,
 			t->closid = to->closid;
 			t->rmid = to->mon.rmid;
 
-#ifdef CONFIG_SMP
 			/*
-			 * This is safe on x86 w/o barriers as the ordering
-			 * of writing to task_cpu() and t->on_cpu is
-			 * reverse to the reading here. The detection is
-			 * inaccurate as tasks might move or schedule
-			 * before the smp function call takes place. In
-			 * such a case the function call is pointless, but
+			 * If the task is on a CPU, set the CPU in the mask.
+			 * The detection is inaccurate as tasks might move or
+			 * schedule before the smp function call takes place.
+			 * In such a case the function call is pointless, but
 			 * there is no other side effect.
 			 */
-			if (mask && t->on_cpu)
+			if (IS_ENABLED(CONFIG_SMP) && mask && task_curr(t))
 				cpumask_set_cpu(task_cpu(t), mask);
-#endif
 		}
 	}
 	read_unlock(&tasklist_lock);

2.26.2

From: Valentin Schneider <[email protected]>

A CPU's current task can have its {closid, rmid} fields read locally while they are being concurrently written to from another CPU. This can happen anytime __resctrl_sched_in() races with either __rdtgroup_move_task() or rdt_move_group_tasks().

Prevent load / store tearing for those accesses by giving them the READ_ONCE() / WRITE_ONCE() treatment.

---
V1->V2:
* Subject line prefix x86/intel_rdt -> x86/resctrl. (Reinette)

 arch/x86/include/asm/resctrl.h         | 11 +++++++----
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 10 +++++-----
 2 files changed, 12 insertions(+), 9 deletions(-)

index 07603064df8f..d60ed0668a59 100644
@@ -56,19 +56,22 @@ static void __resctrl_sched_in(void)
 	struct resctrl_pqr_state *state = this_cpu_ptr(&pqr_state);
 	u32 closid = state->default_closid;
 	u32 rmid = state->default_rmid;
+	u32 tmp;
 
 	/*
 	 * If this task has a closid/rmid assigned, use it.
 	 * Else use the closid/rmid assigned to this cpu.
 	 */
 	if (static_branch_likely(&rdt_alloc_enable_key)) {
-		if (current->closid)
-			closid = current->closid;
+		tmp = READ_ONCE(current->closid);
+		if (tmp)
+			closid = tmp;
 	}
 
 	if (static_branch_likely(&rdt_mon_enable_key)) {
-		if (current->rmid)
-			rmid = current->rmid;
+		tmp = READ_ONCE(current->rmid);
+		if (tmp)
+			rmid = tmp;
 	}
 
 	if (closid != state->cur_closid || rmid != state->cur_rmid) {

index 9bd36210d220..5aeb4fb91228 100644
@@ -569,11 +569,11 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
 	 */
 
 	if (rdtgrp->type == RDTCTRL_GROUP) {
-		tsk->closid = rdtgrp->closid;
-		tsk->rmid = rdtgrp->mon.rmid;
+		WRITE_ONCE(tsk->closid, rdtgrp->closid);
+		WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
 	} else if (rdtgrp->type == RDTMON_GROUP) {
 		if (rdtgrp->mon.parent->closid == tsk->closid) {
-			tsk->rmid = rdtgrp->mon.rmid;
+			WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
 		} else {
 			rdt_last_cmd_puts("Can't move task to different control group\n");
 			return -EINVAL;

@@ -2316,8 +2316,8 @@ static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to,
 	for_each_process_thread(p, t) {
 		if (!from || is_closid_match(t, from) ||
 		    is_rmid_match(t, from)) {
-			t->closid = to->closid;
-			t->rmid = to->mon.rmid;
+			WRITE_ONCE(t->closid, to->closid);
+			WRITE_ONCE(t->rmid, to->mon.rmid);
 
 			/*
 			 * If the task is on a CPU, set the CPU in the mask.

2.26.2