lkml.org 
[lkml]   [2020]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 2/3] x86/resctrl: Update PQR_ASSOC MSR synchronously when moving task to resource group
From
Date
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

\
 
 \ /
  Last update: 2020-12-10 01:25    [W:0.130 / U:0.732 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site