Re: [PATCH 2/2] x86/intel_rdt: Update task closid immediately on CPU in rmdir and unmount
On Sat, Nov 26, 2016 at 10:08:57AM +0100, Thomas Gleixner wrote: > On Fri, 25 Nov 2016, Fenghua Yu wrote: > > On Wed, Nov 23, 2016 at 03:23:50PM +0100, Thomas Gleixner wrote: > > > +#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 > > > + * there is no other side effect. > > > + */ > > > > If process p1 is running on CPU1 before this point, > > > > > + if (mask && t->on_cpu) > > > + cpumask_set_cpu(task_cpu(t), mask); > > > > If between CPU1 is set in mask and rdt_update_closid(tmpmask, NULL) is > > called, p1 is switched to CPU2, and process p2 with its own closid > > (e.g. 2) is switched to CPU1. > > > > Then closid in PQR_ASSOC is set incorrectly as 0 instead of 2 on CPU1. > > 0 may stay in PQR_ASSOC until next context switch which may take long time > > in cases of real time or HPC. > > > > Don't we need to care this situation? In this situation, the function call > > is not "pointless" but it's wrong, right? > > No. > > CPU0 CPU1CPU2 > T1 (closid 0) T2 (closid 2) > > (t1->on_cpu) > set(1, mask) > preemption > T1 ->CPU2 > switch_to(T3) preemption > switch_to(idle) > T2 -> CPU1 > switch_to(T2) switch_to(T1) >intel_rdt_sched_in()intel_rdt_sched_in() > closid = T2->closidclosid = T1->closid > closid =2 closid = CPU2->closid > > rdt_update_closid(mask) > > rdt_update_cpu_closid() > intel_rdt_sched_in() > closid = T2->closid > closid = 2 > > IOW, whatever comes first, sched_switch() or function call will update the > closid to the correct value. > > If CPU2 was in the removed group then this looks the following way: > > CPU0 CPU1CPU2 > T1 (closid 0) T2 (closid 2) > > (t1->on_cpu) > set(1, mask) > preemption > T1 ->CPU2 > switch_to(T3) preemption > switch_to(idle) > T2 -> CPU1 > switch_to(T2) switch_to(T1) >intel_rdt_sched_in()intel_rdt_sched_in() > closid = T2->closid closid = T1->closid (0) > closid =2 closid = CPU2->closid >closid = 5 > for_each_cpu(grp->mask) > CPU2->closid = 0 > > rdt_update_closid(mask) > > rdt_update_cpu_closid() rdt_update_cpu_closid() > intel_rdt_sched_in(intel_rdt_sched_in() > closid = T2->closidclosid = T1->closid (0) > closid = 2 closid = CPU2->closid > closid = 0 > > But on CPU2 the function call might be pointless as well in the following > situation: > > CPU0 CPU1CPU2 > T1 (closid 0) T2 (closid 2) > > (t1->on_cpu) > set(1, mask) > preemption > T1 ->CPU2 > switch_to(T3) preemption > switch_to(idle) > > for_each_cpu(grp->mask) > CPU2->closid = 0 > T2 -> CPU1 > switch_to(T2) switch_to(T1) >intel_rdt_sched_in()intel_rdt_sched_in() > closid = T2->closid closid = T1->closid (0) > closid =2 closid = CPU2->closid >closid = 0 > > rdt_update_closid(mask) > > rdt_update_cpu_closid() rdt_update_cpu_closid() > intel_rdt_sched_in(intel_rdt_sched_in() > closid = T2->closidclosid = T1->closid (0) > closid = 2 closid = CPU2->closid > closid = 0 > > The whole thing works by ordering: > > 1) Update closids of each task in the group and if a task is running on a >cpu then mark the CPU on which the task is running for the function >call. > > 2) Update closids of each CPU in the group > > 3) Or the cpumasks of the tasks and the groups and invoke the function call >on all of them > > If an
Re: [PATCH 2/2] x86/intel_rdt: Update task closid immediately on CPU in rmdir and unmount
On Fri, 25 Nov 2016, Fenghua Yu wrote: > On Wed, Nov 23, 2016 at 03:23:50PM +0100, Thomas Gleixner wrote: > > +#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 > > +* there is no other side effect. > > +*/ > > If process p1 is running on CPU1 before this point, > > > + if (mask && t->on_cpu) > > + cpumask_set_cpu(task_cpu(t), mask); > > If between CPU1 is set in mask and rdt_update_closid(tmpmask, NULL) is > called, p1 is switched to CPU2, and process p2 with its own closid > (e.g. 2) is switched to CPU1. > > Then closid in PQR_ASSOC is set incorrectly as 0 instead of 2 on CPU1. > 0 may stay in PQR_ASSOC until next context switch which may take long time > in cases of real time or HPC. > > Don't we need to care this situation? In this situation, the function call > is not "pointless" but it's wrong, right? No. CPU0CPU1CPU2 T1 (closid 0) T2 (closid 2) (t1->on_cpu) set(1, mask) preemption T1 ->CPU2 switch_to(T3) preemption switch_to(idle) T2 -> CPU1 switch_to(T2) switch_to(T1) intel_rdt_sched_in()intel_rdt_sched_in() closid = T2->closidclosid = T1->closid closid =2 closid = CPU2->closid rdt_update_closid(mask) rdt_update_cpu_closid() intel_rdt_sched_in() closid = T2->closid closid = 2 IOW, whatever comes first, sched_switch() or function call will update the closid to the correct value. If CPU2 was in the removed group then this looks the following way: CPU0CPU1CPU2 T1 (closid 0) T2 (closid 2) (t1->on_cpu) set(1, mask) preemption T1 ->CPU2 switch_to(T3) preemption switch_to(idle) T2 -> CPU1 switch_to(T2) switch_to(T1) intel_rdt_sched_in()intel_rdt_sched_in() closid = T2->closid closid = T1->closid (0) closid =2 closid = CPU2->closid closid = 5 for_each_cpu(grp->mask) CPU2->closid = 0 rdt_update_closid(mask) rdt_update_cpu_closid() rdt_update_cpu_closid() intel_rdt_sched_in(intel_rdt_sched_in() closid = T2->closidclosid = T1->closid (0) closid = 2 closid = CPU2->closid closid = 0 But on CPU2 the function call might be pointless as well in the following situation: CPU0CPU1CPU2 T1 (closid 0) T2 (closid 2) (t1->on_cpu) set(1, mask) preemption T1 ->CPU2 switch_to(T3) preemption switch_to(idle) for_each_cpu(grp->mask) CPU2->closid = 0 T2 -> CPU1 switch_to(T2) switch_to(T1) intel_rdt_sched_in()intel_rdt_sched_in() closid = T2->closid closid = T1->closid (0) closid =2 closid = CPU2->closid closid = 0 rdt_update_closid(mask) rdt_update_cpu_closid() rdt_update_cpu_closid() intel_rdt_sched_in(intel_rdt_sched_in() closid = T2->closidclosid = T1->closid (0) closid = 2 closid = CPU2->closid closid = 0 The whole thing works by ordering: 1) Update closids of each task in the group and if a task is running on a cpu then mark the CPU on which the task is running for the function call. 2) Update closids of each CPU in the group 3) Or the cpumasks of the tasks and the groups and invoke the function call on all of them If an affected task does a sched_switch after task->closid is updated and before the function call is invoked then the function call is pointless. If a sched switch happens on a CPU aft
Re: [PATCH 2/2] x86/intel_rdt: Update task closid immediately on CPU in rmdir and unmount
On Wed, Nov 23, 2016 at 03:23:50PM +0100, Thomas Gleixner wrote:
> On Fri, 18 Nov 2016, Fenghua Yu wrote:
> Reworked untested patch below.
The reworked patch passes my baisc tests. But I have a quesiton on
rdt_move_group_tasks() (please see below).
>
> Thanks,
>
> tglx
>
> 8<-
> --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> @@ -194,12 +194,13 @@ static int rdtgroup_cpus_show(struct ker
> /*
> * This is safe against intel_rdt_sched_in() called from __switch_to()
> * because __switch_to() is executed with interrupts disabled. A local call
> - * from rdt_update_percpu_closid() is proteced against __switch_to() because
> + * from rdt_update_closid() is proteced against __switch_to() because
> * preemption is disabled.
> */
> -static void rdt_update_cpu_closid(void *v)
> +static void rdt_update_cpu_closid(void *closid)
> {
> - this_cpu_write(cpu_closid, *(int *)v);
> + if (closid)
> + this_cpu_write(cpu_closid, *(int *)closid);
> /*
>* We cannot unconditionally write the MSR because the current
>* executing task might have its own closid selected. Just reuse
> @@ -208,14 +209,23 @@ static void rdt_update_cpu_closid(void *
> intel_rdt_sched_in();
> }
>
> -/* Update the per cpu closid and eventually the PGR_ASSOC MSR */
> -static void rdt_update_percpu_closid(const struct cpumask *cpu_mask, int
> closid)
> +/*
> + * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
> + *
> + * Per task closids must have been set up before calling this function.
> + *
> + * The per cpu closids are updated with the smp function call, when @closid
> + * is not NULL. If @closid is NULL then all affected percpu closids must
> + * have been set up before calling this function.
> + */
> +static void
> +rdt_update_closid(const struct cpumask *cpu_mask, int *closid)
> {
> int cpu = get_cpu();
>
> if (cpumask_test_cpu(cpu, cpu_mask))
> - rdt_update_cpu_closid(&closid);
> - smp_call_function_many(cpu_mask, rdt_update_cpu_closid, &closid, 1);
> + rdt_update_cpu_closid(closid);
> + smp_call_function_many(cpu_mask, rdt_update_cpu_closid, closid, 1);
> put_cpu();
> }
>
> @@ -264,7 +274,7 @@ static ssize_t rdtgroup_cpus_write(struc
> /* Give any dropped cpus to rdtgroup_default */
> cpumask_or(&rdtgroup_default.cpu_mask,
> &rdtgroup_default.cpu_mask, tmpmask);
> - rdt_update_percpu_closid(tmpmask, rdtgroup_default.closid);
> + rdt_update_closid(tmpmask, &rdtgroup_default.closid);
> }
>
> /*
> @@ -278,7 +288,7 @@ static ssize_t rdtgroup_cpus_write(struc
> continue;
> cpumask_andnot(&r->cpu_mask, &r->cpu_mask, tmpmask);
> }
> - rdt_update_percpu_closid(tmpmask, rdtgrp->closid);
> + rdt_update_closid(tmpmask, &rdtgrp->closid);
> }
>
> /* Done pushing/pulling - update this group with new mask */
> @@ -807,18 +817,49 @@ static int reset_all_cbms(struct rdt_res
> }
>
> /*
> - * Forcibly remove all of subdirectories under root.
> + * Move tasks from one to the other group. If @from is NULL, then all tasks
> + * in the systems are moved unconditionally (used for teardown).
> + *
> + * If @mask is not NULL the cpus on which moved tasks are running are set
> + * in that mask so the update smp function call is restricted to affected
> + * cpus.
> */
> -static void rmdir_all_sub(void)
> +static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to,
> + struct cpumask *mask)
> {
> - struct rdtgroup *rdtgrp, *tmp;
> struct task_struct *p, *t;
>
> - /* move all tasks to default resource group */
> read_lock(&tasklist_lock);
> - for_each_process_thread(p, t)
> - t->closid = 0;
> + for_each_process_thread(p, t) {
> + if (!from || t->closid == from->closid) {
> + t->closid = to->closid;
> +#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
> + * there is no other side effect.
> + */
If process p1 is running on CPU1 before this point,
> + if (mask && t->on_cpu)
> + cpumask_set_cpu(task_cpu(t), mask);
If between CPU1 is set in mask and rdt_update_closid(tmpmask, NULL) is
called, p1 is switched to CPU2, and process p2 with its own closid
(e.g. 2)
Re: [PATCH 2/2] x86/intel_rdt: Update task closid immediately on CPU in rmdir and unmount
On Fri, 18 Nov 2016, Fenghua Yu wrote:
>
> When removing a sub directory/rdtgroup by rmdir or umount, closid in a
> task in the sub directory is set to default rdtgroup's closid which is 0.
> If the task is running on a CPU, the PQR_ASSOC MSR is only updated
> when the task runs through a context switch. Up to the context switch,
> the task runs with the wrong closid.
>
> Make the change immediately effective by invoking a smp function call
> on all online CPUs which calls intel_rdt_sched_in() to update the
> PQR_ASSOC MSR.
We can be smarter than that and figure out which cpus need that update
rather than interrupting the world for nothing.
> rdt_update_closid() (renamed from rdt_update_percpu_closid()) calls
> intel_rdt_sched_in() to update closid in the PQR_ASSOC MSR on a CPU.
> The task closid and percpu closid are set up before
> rdt_update_closid() is called. Handling PQR_ASSOC MSR update for
> both task closid and percpu closid in rdt_update_closid() reduces
> redundant smp function calls.
You should not describe WHAT the patch is doing, the WHY and the concept is
what needs to be in the changelog. The WHAT is in the patch.
> @@ -224,7 +226,7 @@ static ssize_t rdtgroup_cpus_write(struct
> kernfs_open_file *of,
> {
> cpumask_var_t tmpmask, newmask;
> struct rdtgroup *rdtgrp, *r;
> - int ret;
> + int cpu, ret;
>
> if (!buf)
> return -EINVAL;
> @@ -264,7 +266,9 @@ static ssize_t rdtgroup_cpus_write(struct
> kernfs_open_file *of,
> /* Give any dropped cpus to rdtgroup_default */
> cpumask_or(&rdtgroup_default.cpu_mask,
> &rdtgroup_default.cpu_mask, tmpmask);
> - rdt_update_percpu_closid(tmpmask, rdtgroup_default.closid);
> + for_each_cpu(cpu, tmpmask)
> + per_cpu(cpu_closid, cpu) = rdtgroup_default.closid;
> + rdt_update_closid(tmpmask);
Bah, this is silly. We can make that conditional instead of mindlessly
slapping a for_each_cpu() loop into every other function.
static void rdt_update_cpu_closid(void *closid)
{
if (closid)
this_cpu_write(cpu_closid, *(int *)closid);
}
and have:
static void rdt_update_closid(const struct cpumask *cpu_mask, int *closid)
{
int cpu = get_cpu();
if (cpumask_test_cpu(cpu, cpu_mask))
rdt_update_cpu_closid(closid);
smp_call_function_many(cpu_mask, rdt_update_cpu_closid, closid, 1);
put_cpu();
}
So the change here becomes:
- rdt_update_percpu_closid(tmpmask, rdtgroup_default.closid);
+ rdt_update_closid(tmpmask, &rdtgroup_default.closid);
and for the case where we need to deal with tasks, do the task move, the
percpu update and call rdt_update_closid(mask, NULL);
> +static int rdt_move_task_closid(struct rdtgroup *from, struct rdtgroup *to)
Bah, consistent types are optional, right? int != bool
> +{
So the right thing to do here is aside of using a proper function name:
static bool rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to,
struct cpumask *mask)
{
struct task_struct *p, *t;
bool moved_task = false;
read_lock(&tasklist_lock);
for_each_process_thread(p, t) {
if (!from || t->closid == from->closid) {
t->closid = to->closid;
#ifdef CONFIG_SMP
if (mask && task->on_cpu) {
moved_task = true;
cpumask_set_cpu(task_cpu(t), mask);
}
#endif
}
}
read_unlock(&tasklist_lock);
return moved_task;
}
> /*
> * Forcibly remove all of subdirectories under root.
> */
> static void rmdir_all_sub(void)
> {
> struct rdtgroup *rdtgrp, *tmp;
> - struct task_struct *p, *t;
> + int cpu;
>
> /* move all tasks to default resource group */
> - read_lock(&tasklist_lock);
> - for_each_process_thread(p, t)
> - t->closid = 0;
> - read_unlock(&tasklist_lock);
> + rdt_move_task_closid(NULL, &rdtgroup_default);
So this becomes
rdt_move_group_tasks(NULL, &rdtgroup_default, NULL);
>
> list_for_each_entry_safe(rdtgrp, tmp, &rdt_all_groups, rdtgroup_list) {
> /* Remove each rdtgroup other than root */
> @@ -833,13 +853,18 @@ static void rmdir_all_sub(void)
> cpumask_or(&rdtgroup_default.cpu_mask,
> &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask);
>
> - rdt_update_percpu_closid(&rdtgrp->cpu_mask,
> - rdtgroup_default.closid);
> + for_each_cpu(cpu, &rdtgrp->cpu_mask)
> + per_cpu(cpu_closid, cpu) = rdtgroup_default.closid;
And this is pointless when we have the cpuid pointer in rdt_update_closid()
and the below becomes:
> + rdt_update_closid(c
[PATCH 2/2] x86/intel_rdt: Update task closid immediately on CPU in rmdir and unmount
From: Fenghua Yu
When removing a sub directory/rdtgroup by rmdir or umount, closid in a
task in the sub directory is set to default rdtgroup's closid which is 0.
If the task is running on a CPU, the PQR_ASSOC MSR is only updated
when the task runs through a context switch. Up to the context switch,
the task runs with the wrong closid.
Make the change immediately effective by invoking a smp function call
on all online CPUs which calls intel_rdt_sched_in() to update the
PQR_ASSOC MSR.
rdt_update_closid() (renamed from rdt_update_percpu_closid()) calls
intel_rdt_sched_in() to update closid in the PQR_ASSOC MSR on a CPU.
The task closid and percpu closid are set up before
rdt_update_closid() is called. Handling PQR_ASSOC MSR update for
both task closid and percpu closid in rdt_update_closid() reduces
redundant smp function calls.
Signed-off-by: Fenghua Yu
---
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 88 +++-
1 file changed, 63 insertions(+), 25 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index eccea8a..ff0ee57 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -194,12 +194,14 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
/*
* This is safe against intel_rdt_sched_in() called from __switch_to()
* because __switch_to() is executed with interrupts disabled. A local call
- * from rdt_update_percpu_closid() is proteced against __switch_to() because
+ * from rdt_update_closid() is proteced against __switch_to() because
* preemption is disabled.
+ *
+ * Task closid and percpu closid should be set up before calling
+ * this function.
*/
-static void rdt_update_cpu_closid(void *v)
+static void rdt_update_cpu_closid(void *unused)
{
- this_cpu_write(cpu_closid, *(int *)v);
/*
* We cannot unconditionally write the MSR because the current
* executing task might have its own closid selected. Just reuse
@@ -208,14 +210,14 @@ static void rdt_update_cpu_closid(void *v)
intel_rdt_sched_in();
}
-/* Update the per cpu closid and eventually the PGR_ASSOC MSR */
-static void rdt_update_percpu_closid(const struct cpumask *cpu_mask, int
closid)
+/* Update closid in the PGR_ASSOC MSR */
+static void rdt_update_closid(const struct cpumask *cpu_mask)
{
int cpu = get_cpu();
if (cpumask_test_cpu(cpu, cpu_mask))
- rdt_update_cpu_closid(&closid);
- smp_call_function_many(cpu_mask, rdt_update_cpu_closid, &closid, 1);
+ rdt_update_cpu_closid(NULL);
+ smp_call_function_many(cpu_mask, rdt_update_cpu_closid, NULL, 1);
put_cpu();
}
@@ -224,7 +226,7 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file
*of,
{
cpumask_var_t tmpmask, newmask;
struct rdtgroup *rdtgrp, *r;
- int ret;
+ int cpu, ret;
if (!buf)
return -EINVAL;
@@ -264,7 +266,9 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file
*of,
/* Give any dropped cpus to rdtgroup_default */
cpumask_or(&rdtgroup_default.cpu_mask,
&rdtgroup_default.cpu_mask, tmpmask);
- rdt_update_percpu_closid(tmpmask, rdtgroup_default.closid);
+ for_each_cpu(cpu, tmpmask)
+ per_cpu(cpu_closid, cpu) = rdtgroup_default.closid;
+ rdt_update_closid(tmpmask);
}
/*
@@ -278,7 +282,9 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file
*of,
continue;
cpumask_andnot(&r->cpu_mask, &r->cpu_mask, tmpmask);
}
- rdt_update_percpu_closid(tmpmask, rdtgrp->closid);
+ for_each_cpu(cpu, tmpmask)
+ per_cpu(cpu_closid, cpu) = rdtgrp->closid;
+ rdt_update_closid(tmpmask);
}
/* Done pushing/pulling - update this group with new mask */
@@ -806,19 +812,33 @@ static int reset_all_cbms(struct rdt_resource *r)
return 0;
}
+static int rdt_move_task_closid(struct rdtgroup *from, struct rdtgroup *to)
+{
+ struct task_struct *p, *t;
+ bool moved_task = false;
+
+ read_lock(&tasklist_lock);
+ for_each_process_thread(p, t) {
+ if (!from || t->closid == from->closid) {
+ t->closid = to->closid;
+ moved_task = true;
+ }
+ }
+ read_unlock(&tasklist_lock);
+
+ return moved_task;
+}
+
/*
* Forcibly remove all of subdirectories under root.
*/
static void rmdir_all_sub(void)
{
struct rdtgroup *rdtgrp, *tmp;
- struct task_struct *p, *t;
+ int cpu;
/* move all tasks to default resource group */
- read_lock(&tasklist_lock);
- for_each_process_thread(p, t)
- t->closid = 0;
- read_unlock(&tasklist_lock);
