Hi James,
On 12/9/2020 8:51 AM, James Morse wrote:
Hi Reinette, Fenghua,
Subject nit: I think 'use IPI instead of task_work() to update PQR_ASSOC_MSR' conveys the guts of this change more quickly!
Sure. Thank you. A small change is that I plan to write "PQR_ASSOC MSR" instead to closer match the name.
On 03/12/2020 23:25, Reinette Chatre wrote:
From: Fenghua Yu fenghua.yu@intel.com
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.
kernel exit makes me thing of user-space... is it enough to just say: "by __switch_to() when task is next run"?
I do not think that would be accurate. The paragraph to which you are proposing the change states the current context, before the fix, when task_work_add() is still used to update PQR_ASSOC. The "kernel exit" text you refer to is quite close to task_work_add()'s comments and indeed refers to the work that is run before returning to user space. If a function name would make things clearer perhaps adding exit_to_user_mode_loop() instead? Changing the text to __switch_to() does not reflect the context described here since __switch_to() is what is called when task is context switched back from where resctrl_sched_in() is called, not the queued work being described here.
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.
(as t->on_cpu is already used...)
Reviewed-by: James Morse james.morse@arm.com
Thank you very much. I do plan to follow your suggestion below though.
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c index 68db7d2dec8f..9d62f1fadcc3 100644 --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c @@ -525,6 +525,16 @@ static void rdtgroup_remove(struct rdtgroup *rdtgrp)
+static void update_task_closid_rmid(struct task_struct *t) {
- int cpu;
- if (task_on_cpu(t, &cpu))
smp_call_function_single(cpu, _update_task_closid_rmid, t, 1);I think: | if (task_curr(t)) | smp_call_function_single(task_cpu(t), _update_task_closid_rmid, t, 1);
here would make for an easier backport as it doesn't depend on the previous patch.
Will do, thank you very much.
The previous patch has now become a bugfix in its own right after you pointed out the issue with using t->on_cpu. In addressing that I plan to remove the helpers found in patch #1 so backporting should continue to be easier.
+}
[...]
static int __rdtgroup_move_task(struct task_struct *tsk, struct rdtgroup *rdtgrp) {
- if (rdtgrp->type == RDTCTRL_GROUP) {
tsk->closid = rdtgrp->closid;tsk->rmid = rdtgrp->mon.rmid;- } else if (rdtgrp->type == RDTMON_GROUP) {
[...]
- } else {
rdt_last_cmd_puts("Invalid resource group type\n");return -EINVAL;Wouldn't this be a kernel bug? I'd have thought there would be a WARN_ON_ONCE() here to make it clear this isn't user-space's fault!
You are right, this would be a kernel bug. I'll add a WARN_ON_ONCE().
}
- return ret;
- /*
* 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; }
Thank you very much.
Reinette