diff mbox series

[v4,2/3] x86/resctrl: Parameterize rdt_move_group_tasks() task matching

Message ID 20230308131452.383914-3-peternewman@google.com
State New
Headers
Series Subject: x86/resctrl: Implement rename to help move containers' tasks |

Commit Message

Peter Newman March 8, 2023, 1:14 p.m. UTC 
  Allow rdt_move_group_tasks() to be used for new group-scope operations.
This function is currently only used to implement rmdir on a group or
unmounting resctrlfs.

Callers now provide a filtering function to indicate which tasks should
be moved.

No functional change.

Signed-off-by: Peter Newman <peternewman@google.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 34 +++++++++++++++++++-------
 1 file changed, 25 insertions(+), 9 deletions(-)

Comments

Reinette Chatre March 23, 2023, 6:02 p.m. UTC | #1 
Hi Peter,

On 3/8/2023 5:14 AM, Peter Newman wrote:
> Allow rdt_move_group_tasks() to be used for new group-scope operations.

This changelog jumps right into the solution. By doing so it makes what
follows hard to parse. Could you please start with the context, then
the problem and end with the solution?

> This function is currently only used to implement rmdir on a group or
> unmounting resctrlfs.
> 
> Callers now provide a filtering function to indicate which tasks should
> be moved.
> 
> No functional change.
> 
> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 34 +++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index c3fb525d52e9..84af23a29612 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2393,22 +2393,29 @@ static int reset_all_ctrls(struct rdt_resource *r)
>  }
>  
>  /*
> - * Move tasks from one to the other group. If @from is NULL, then all tasks
> - * in the systems are moved unconditionally (used for teardown).
> + * Move tasks from one to the other group.
> + *
> + * @from:		passed unmodified to task_match_fn() for each task

When using this format it usually starts with a description of the parameter
before moving to how the parameter is used. Perhaps prefix the above with
something like "Resource group tasks are moved from." (Please feel free to
improve.) When starting to provide multiple sentences it helps formatting
to have each sentence start with a capital letter and end with a period.

> + * @to:			group providing new config values for matching tasks
> + * @task_match_fn:	callback returning true when a task requires update

Could this order please match the order of the parameters in the function?

> + * @mask:		output-parameter indicating set of CPUs impacted by this
> + *			operation
>   *
>   * 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.

Could the above be merged with the earlier description of @mask? Please change
cpus to CPUs if you do.

>   */
> -static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to,
> -				 struct cpumask *mask)
> +static void rdt_move_group_tasks(struct rdtgroup *from,
> +				 struct rdtgroup *to,
> +				 struct cpumask *mask,
> +				 bool task_match_fn(struct task_struct *,
> +						    struct rdtgroup *))
>  {
>  	struct task_struct *p, *t;
>  
>  	read_lock(&tasklist_lock);
>  	for_each_process_thread(p, t) {
> -		if (!from || is_closid_match(t, from) ||
> -		    is_rmid_match(t, from)) {
> +		if (task_match_fn(t, from)) {
>  			WRITE_ONCE(t->closid, to->closid);
>  			WRITE_ONCE(t->rmid, to->mon.rmid);
>  
> @@ -2451,6 +2458,15 @@ static void free_all_child_rdtgrp(struct rdtgroup *rdtgrp)
>  	}
>  }
>  
> +/*
> + * If @from is NULL, then all tasks in the systems are moved unconditionally
> + * (used for teardown).

Could this description be expanded to describe what the matching does? Just jumping
in with the above sentence is quite cryptic.

> + */
> +static bool rmdir_match(struct task_struct *t, struct rdtgroup *from)

Could the function's name please reflect what the function does as opposed to
what the current users are doing at the time they call it? Perhaps
something like "task_in_any_group()" (thinking ahead about a possible
"task_in_mon_group()" for the next patch, please feel free to change).
Also note that the "from" is another naming that reflects the usage as
opposed to what the function does. It could just be "rdtgrp".

> +{
> +	return !from || is_closid_match(t, from) || is_rmid_match(t, from);
> +}
> +
>  /*
>   * Forcibly remove all of subdirectories under root.
>   */
> @@ -2459,7 +2475,7 @@ static void rmdir_all_sub(void)
>  	struct rdtgroup *rdtgrp, *tmp;
>  
>  	/* Move all tasks to the default resource group */
> -	rdt_move_group_tasks(NULL, &rdtgroup_default, NULL);
> +	rdt_move_group_tasks(NULL, &rdtgroup_default, NULL, rmdir_match);
>  
>  	list_for_each_entry_safe(rdtgrp, tmp, &rdt_all_groups, rdtgroup_list) {
>  		/* Free any child rmids */
> @@ -3124,7 +3140,7 @@ static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  	int cpu;
>  
>  	/* Give any tasks back to the parent group */
> -	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
> +	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask, rmdir_match);
>  
>  	/* Update per cpu rmid of the moved CPUs first */
>  	for_each_cpu(cpu, &rdtgrp->cpu_mask)
> @@ -3164,7 +3180,7 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  	int cpu;
>  
>  	/* Give any tasks back to the default group */
> -	rdt_move_group_tasks(rdtgrp, &rdtgroup_default, tmpmask);
> +	rdt_move_group_tasks(rdtgrp, &rdtgroup_default, tmpmask, rmdir_match);
>  
>  	/* Give any CPUs back to the default group */
>  	cpumask_or(&rdtgroup_default.cpu_mask,

This looks good. Thanks for creating the match function. I think it
turned out well.

Reinette
Peter Newman March 30, 2023, 12:50 p.m. UTC | #2 
Hi Reinette,

On Thu, Mar 23, 2023 at 7:02 PM Reinette Chatre
<reinette.chatre@intel.com> wrote:
> On 3/8/2023 5:14 AM, Peter Newman wrote:
> > +/*
> > + * If @from is NULL, then all tasks in the systems are moved unconditionally
> > + * (used for teardown).
>
> Could this description be expanded to describe what the matching does? Just jumping
> in with the above sentence is quite cryptic.
>
> > + */
> > +static bool rmdir_match(struct task_struct *t, struct rdtgroup *from)
>
> Could the function's name please reflect what the function does as opposed to
> what the current users are doing at the time they call it? Perhaps
> something like "task_in_any_group()" (thinking ahead about a possible
> "task_in_mon_group()" for the next patch, please feel free to change).
> Also note that the "from" is another naming that reflects the usage as
> opposed to what the function does. It could just be "rdtgrp".

I read over the behavior of this function more carefully to try to come
up with a more descriptive name and realized that for MON groups it
behaves identically to the new one I created for rename().

The original rdt_move_group_tasks() was actually fine all along so I'm
just going to drop this patch.

-Peter
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index c3fb525d52e9..84af23a29612 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2393,22 +2393,29 @@  static int reset_all_ctrls(struct rdt_resource *r)
 }
 
 /*
- * Move tasks from one to the other group. If @from is NULL, then all tasks
- * in the systems are moved unconditionally (used for teardown).
+ * Move tasks from one to the other group.
+ *
+ * @from:		passed unmodified to task_match_fn() for each task
+ * @to:			group providing new config values for matching tasks
+ * @task_match_fn:	callback returning true when a task requires update
+ * @mask:		output-parameter indicating set of CPUs impacted by this
+ *			operation
  *
  * 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 rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to,
-				 struct cpumask *mask)
+static void rdt_move_group_tasks(struct rdtgroup *from,
+				 struct rdtgroup *to,
+				 struct cpumask *mask,
+				 bool task_match_fn(struct task_struct *,
+						    struct rdtgroup *))
 {
 	struct task_struct *p, *t;
 
 	read_lock(&tasklist_lock);
 	for_each_process_thread(p, t) {
-		if (!from || is_closid_match(t, from) ||
-		    is_rmid_match(t, from)) {
+		if (task_match_fn(t, from)) {
 			WRITE_ONCE(t->closid, to->closid);
 			WRITE_ONCE(t->rmid, to->mon.rmid);
 
@@ -2451,6 +2458,15 @@  static void free_all_child_rdtgrp(struct rdtgroup *rdtgrp)
 	}
 }
 
+/*
+ * If @from is NULL, then all tasks in the systems are moved unconditionally
+ * (used for teardown).
+ */
+static bool rmdir_match(struct task_struct *t, struct rdtgroup *from)
+{
+	return !from || is_closid_match(t, from) || is_rmid_match(t, from);
+}
+
 /*
  * Forcibly remove all of subdirectories under root.
  */
@@ -2459,7 +2475,7 @@  static void rmdir_all_sub(void)
 	struct rdtgroup *rdtgrp, *tmp;
 
 	/* Move all tasks to the default resource group */
-	rdt_move_group_tasks(NULL, &rdtgroup_default, NULL);
+	rdt_move_group_tasks(NULL, &rdtgroup_default, NULL, rmdir_match);
 
 	list_for_each_entry_safe(rdtgrp, tmp, &rdt_all_groups, rdtgroup_list) {
 		/* Free any child rmids */
@@ -3124,7 +3140,7 @@  static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
 	int cpu;
 
 	/* Give any tasks back to the parent group */
-	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
+	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask, rmdir_match);
 
 	/* Update per cpu rmid of the moved CPUs first */
 	for_each_cpu(cpu, &rdtgrp->cpu_mask)
@@ -3164,7 +3180,7 @@  static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
 	int cpu;
 
 	/* Give any tasks back to the default group */
-	rdt_move_group_tasks(rdtgrp, &rdtgroup_default, tmpmask);
+	rdt_move_group_tasks(rdtgrp, &rdtgroup_default, tmpmask, rmdir_match);
 
 	/* Give any CPUs back to the default group */
 	cpumask_or(&rdtgroup_default.cpu_mask,