[PATCH] x86/resctrl: fix redundant task movement

[PATCH] x86/resctrl: fix redundant task movement

[Shakeel Butt]

Currently a task can be moved to a rdtgroup multiple times or between
resource or monitoring groups. This can cause multiple task works are
added, waste memory and degrade performance.

To fix the issue, only move the task to a rdtgroup when the task
is not in the rdgroup. Don't try to move the task to the rdtgroup
again when the task is already in the rdtgroup.

Also move the setting of tsk->closid and tsk->rmid before task_work_add
to remove a potential race where the task start executing the registered
callback before the request set its closid and rmid.

Signed-off-by: Shakeel Butt <shakeelb@google.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 ++++++++++++++++++--------
 1 file changed, 43 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index dac7209a0708..8ef10c626862 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -543,15 +543,57 @@ static void move_myself(struct callback_head *head)
 static int __rdtgroup_move_task(struct task_struct *tsk,
 				struct rdtgroup *rdtgrp)
 {
+	struct callback_head *head;
 	struct task_move_callback *callback;
 	int ret;
 
-	callback = kzalloc(sizeof(*callback), GFP_KERNEL);
+	/* If the task is already in rdtgrp, don't move the task. */
+	if ((rdtgrp->type == RDTCTRL_GROUP && tsk->closid == rdtgrp->closid &&
+	     tsk->rmid == rdtgrp->mon.rmid) ||
+	    (rdtgrp->type == RDTMON_GROUP &&
+	     rdtgrp->mon.parent->closid == tsk->closid &&
+	     tsk->rmid == rdtgrp->mon.rmid)) {
+		rdt_last_cmd_puts("Task is already in the rdgroup\n");
+		return 0;
+	}
+
+	/*
+	 * For monitor groups, we can move the tasks only from their parent
+	 * CTRL group.
+	 */
+	if (rdtgrp->type == RDTMON_GROUP &&
+	    rdtgrp->mon.parent->closid != tsk->closid) {
+		rdt_last_cmd_puts("Can't move task to different control group\n");
+		return -EINVAL;
+	}
+
+	/* Cancel exiting request and reuse the memory. */
+	head = task_work_cancel(tsk, move_myself);
+	if (head) {
+		callback = container_of(head, struct task_move_callback, work);
+		if (atomic_dec_and_test(&callback->rdtgrp->waitcount) &&
+		    (callback->rdtgrp->flags & RDT_DELETED))
+			kfree(callback->rdtgrp);
+	} else
+		callback = kzalloc(sizeof(*callback), GFP_KERNEL);
+
 	if (!callback)
 		return -ENOMEM;
 	callback->work.func = move_myself;
 	callback->rdtgrp = rdtgrp;
 
+	/*
+	 * For ctrl_mon groups move both closid and rmid.
+	 * For monitor groups, move the tasks only from their parent
+	 * CTRL group.
+	 */
+	if (rdtgrp->type == RDTCTRL_GROUP) {
+		tsk->closid = rdtgrp->closid;
+		tsk->rmid = rdtgrp->mon.rmid;
+	} else if (rdtgrp->type == RDTMON_GROUP &&
+		   rdtgrp->mon.parent->closid == tsk->closid) {
+		tsk->rmid = rdtgrp->mon.rmid;
+	}
 	/*
 	 * Take a refcount, so rdtgrp cannot be freed before the
 	 * callback has been invoked.
@@ -567,23 +609,6 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
 		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;
-			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;
-			}
-		}
 	}
 	return ret;
 }
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH] x86/resctrl: fix redundant task movement

[Fenghua Yu]

On Tue, Jan 28, 2020 at 04:22:22PM -0800, Shakeel Butt wrote:
> Currently a task can be moved to a rdtgroup multiple times or between
> resource or monitoring groups. This can cause multiple task works are
> added, waste memory and degrade performance.
> 
> To fix the issue, only move the task to a rdtgroup when the task
> is not in the rdgroup. Don't try to move the task to the rdtgroup
> again when the task is already in the rdtgroup.

Hi, Shakeel,

Acutally we are working on replacing the callback by a synchronous way
to update closid and rmid when moving a task to a resource group. The
reason is the task may use old (even invalid) closid and rmid before
they are updated.

With the new way to update closid and rmid, the issues related to
the callbacks will be fixed as well.

We will release the new patches soon.

Thanks.

-Fenghua

Re: [PATCH] x86/resctrl: fix redundant task movement

[Shakeel Butt]

On Tue, Jan 28, 2020 at 4:49 PM Fenghua Yu <fenghua.yu@intel.com> wrote:
>
> On Tue, Jan 28, 2020 at 04:22:22PM -0800, Shakeel Butt wrote:
> > Currently a task can be moved to a rdtgroup multiple times or between
> > resource or monitoring groups. This can cause multiple task works are
> > added, waste memory and degrade performance.
> >
> > To fix the issue, only move the task to a rdtgroup when the task
> > is not in the rdgroup. Don't try to move the task to the rdtgroup
> > again when the task is already in the rdtgroup.
>
> Hi, Shakeel,
>
> Acutally we are working on replacing the callback by a synchronous way
> to update closid and rmid when moving a task to a resource group. The
> reason is the task may use old (even invalid) closid and rmid before
> they are updated.
>
> With the new way to update closid and rmid, the issues related to
> the callbacks will be fixed as well.
>
> We will release the new patches soon.
>
> Thanks.

Sounds good to me. I didn't get any response on the previous email
therefore I code this up.

Shakeel