diff mbox series

[RFC,1/3] x86/resctrl: Add multiple tasks to the resctrl group at once

Message ID 167278359522.34228.17439739885350817510.stgit@bmoger-ubuntu
State New
Headers
Series x86/resctrl: Miscellaneous resctrl features |

Commit Message

Moger, Babu Jan. 3, 2023, 10:06 p.m. UTC 
  Right now, the resctrl task assignment for the MONITOR or CONTROL group
needs to be one at a time. For example:
  $mount -t resctrl resctrl /sys/fs/resctrl/
  $mkdir /sys/fs/resctrl/clos1
  $echo 123 > /sys/fs/resctrl/clos1/tasks
  $echo 456 > /sys/fs/resctrl/clos1/tasks
  $echo 789 > /sys/fs/resctrl/clos1/tasks

This is not user-friendly when dealing with hundreds of tasks.

Improve the user experience by supporting the multiple task assignment
in one command with tasks separated by commas. For example:
  $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/x86/resctrl.rst          |   13 ++++++------
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |   35 ++++++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 13 deletions(-)

Comments

Fenghua Yu Jan. 4, 2023, 5:46 a.m. UTC | #1 
Hi, Babu,

> Right now, the resctrl task assignment for the MONITOR or CONTROL group
> needs to be one at a time. For example:
>   $mount -t resctrl resctrl /sys/fs/resctrl/
>   $mkdir /sys/fs/resctrl/clos1
>   $echo 123 > /sys/fs/resctrl/clos1/tasks
>   $echo 456 > /sys/fs/resctrl/clos1/tasks
>   $echo 789 > /sys/fs/resctrl/clos1/tasks
> 
> This is not user-friendly when dealing with hundreds of tasks.
> 
> Improve the user experience by supporting the multiple task assignment in one
> command with tasks separated by commas. For example:
>   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  Documentation/x86/resctrl.rst          |   13 ++++++------
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   35 ++++++++++++++++++++++++++--
> ----
>  2 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> index 71a531061e4e..f26e16412bcb 100644
> --- a/Documentation/x86/resctrl.rst
> +++ b/Documentation/x86/resctrl.rst
> @@ -208,12 +208,13 @@ All groups contain the following files:
>  "tasks":
>  	Reading this file shows the list of all tasks that belong to
>  	this group. Writing a task id to the file will add a task to the
> -	group. If the group is a CTRL_MON group the task is removed from
> -	whichever previous CTRL_MON group owned the task and also from
> -	any MON group that owned the task. If the group is a MON group,
> -	then the task must already belong to the CTRL_MON parent of this
> -	group. The task is removed from any previous MON group.
> -
> +	group. Multiple tasks can be assigned at once with each task
> +	separated by commas. If the group is a CTRL_MON group the task
> +	is removed from whichever previous CTRL_MON group owned the task
> +	and also from any MON group that owned the task. If the group is
> +	a MON group, then the task must already belong to the CTRL_MON
> +	parent of this group. The task is removed from any previous MON
> +	group.

Multiple tasks movement may fail in the middle. How to handle the failure
in the middle? Abort on all previous success movements?

Seems simple way is to exit from the failed task movement. That means
all previous successful movements will not be reversed and all tasks won't
be moved since the failure.

Then this info needs to be explained in the doc.
> 
>  "cpus":
>  	Reading this file shows a bitmask of the logical CPUs owned by diff --git
> a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index e5a48f05e787..344607853f4c 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -686,28 +686,49 @@ static ssize_t rdtgroup_tasks_write(struct
> kernfs_open_file *of,
>  				    char *buf, size_t nbytes, loff_t off)  {
>  	struct rdtgroup *rdtgrp;
> +	char *pid_str;
>  	int ret = 0;
>  	pid_t pid;
> 
> -	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
> +	/* Valid input requires a trailing newline */
> +	if (nbytes == 0 || buf[nbytes - 1] != '\n')
>  		return -EINVAL;
> +
> +	buf[nbytes - 1] = '\0';
> +
> +	cpus_read_lock();
>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>  	if (!rdtgrp) {
> -		rdtgroup_kn_unlock(of->kn);
> -		return -ENOENT;
> +		ret = -ENOENT;
> +		goto exit;
> +	}
> +
> +next:
> +	if (!buf || buf[0] == '\0')
> +		goto exit;
> +
> +	pid_str = strim(strsep(&buf, ","));
> +
> +	if (kstrtoint(pid_str, 0, &pid) || pid < 0) {
> +		ret = -EINVAL;

rdt_last_cmd_puts() to record the error.

> +		goto exit;
>  	}
> +
>  	rdt_last_cmd_clear();
> 
>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> -	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> -		ret = -EINVAL;
> +			rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>  		rdt_last_cmd_puts("Pseudo-locking in progress\n");
> -		goto unlock;
> +		ret = -EINVAL;
> +		goto exit;
>  	}
> 
>  	ret = rdtgroup_move_task(pid, rdtgrp, of);

Do you want to exit if there is error in rdtgroup_move_task()?
Otherwise, the failure won't be captured if later take movement succeeds.

> 
> -unlock:
> +	goto next;
> +
> +exit:
> +	cpus_read_unlock();
>  	rdtgroup_kn_unlock(of->kn);
> 
>  	return ret ?: nbytes;
> 

Thanks.

-Fenghua
Moger, Babu Jan. 4, 2023, 5:20 p.m. UTC | #2 
Hi Fenghua,

On 1/3/23 23:46, Yu, Fenghua wrote:
> Hi, Babu,
>
>> Right now, the resctrl task assignment for the MONITOR or CONTROL group
>> needs to be one at a time. For example:
>>   $mount -t resctrl resctrl /sys/fs/resctrl/
>>   $mkdir /sys/fs/resctrl/clos1
>>   $echo 123 > /sys/fs/resctrl/clos1/tasks
>>   $echo 456 > /sys/fs/resctrl/clos1/tasks
>>   $echo 789 > /sys/fs/resctrl/clos1/tasks
>>
>> This is not user-friendly when dealing with hundreds of tasks.
>>
>> Improve the user experience by supporting the multiple task assignment in one
>> command with tasks separated by commas. For example:
>>   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  Documentation/x86/resctrl.rst          |   13 ++++++------
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   35 ++++++++++++++++++++++++++--
>> ----
>>  2 files changed, 35 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
>> index 71a531061e4e..f26e16412bcb 100644
>> --- a/Documentation/x86/resctrl.rst
>> +++ b/Documentation/x86/resctrl.rst
>> @@ -208,12 +208,13 @@ All groups contain the following files:
>>  "tasks":
>>  	Reading this file shows the list of all tasks that belong to
>>  	this group. Writing a task id to the file will add a task to the
>> -	group. If the group is a CTRL_MON group the task is removed from
>> -	whichever previous CTRL_MON group owned the task and also from
>> -	any MON group that owned the task. If the group is a MON group,
>> -	then the task must already belong to the CTRL_MON parent of this
>> -	group. The task is removed from any previous MON group.
>> -
>> +	group. Multiple tasks can be assigned at once with each task
>> +	separated by commas. If the group is a CTRL_MON group the task
>> +	is removed from whichever previous CTRL_MON group owned the task
>> +	and also from any MON group that owned the task. If the group is
>> +	a MON group, then the task must already belong to the CTRL_MON
>> +	parent of this group. The task is removed from any previous MON
>> +	group.
> Multiple tasks movement may fail in the middle. How to handle the failure
> in the middle? Abort on all previous success movements?
>
> Seems simple way is to exit from the failed task movement. That means
> all previous successful movements will not be reversed and all tasks won't
> be moved since the failure.

Yes. That is what even I am thinking. Exit on a failed movement and record
the error. Don't need to reverse the successful movements.

>
> Then this info needs to be explained in the doc.
Sure.
>>  "cpus":
>>  	Reading this file shows a bitmask of the logical CPUs owned by diff --git
>> a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index e5a48f05e787..344607853f4c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -686,28 +686,49 @@ static ssize_t rdtgroup_tasks_write(struct
>> kernfs_open_file *of,
>>  				    char *buf, size_t nbytes, loff_t off)  {
>>  	struct rdtgroup *rdtgrp;
>> +	char *pid_str;
>>  	int ret = 0;
>>  	pid_t pid;
>>
>> -	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
>> +	/* Valid input requires a trailing newline */
>> +	if (nbytes == 0 || buf[nbytes - 1] != '\n')
>>  		return -EINVAL;
>> +
>> +	buf[nbytes - 1] = '\0';
>> +
>> +	cpus_read_lock();
>>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>>  	if (!rdtgrp) {
>> -		rdtgroup_kn_unlock(of->kn);
>> -		return -ENOENT;
>> +		ret = -ENOENT;
>> +		goto exit;
>> +	}
>> +
>> +next:
>> +	if (!buf || buf[0] == '\0')
>> +		goto exit;
>> +
>> +	pid_str = strim(strsep(&buf, ","));
>> +
>> +	if (kstrtoint(pid_str, 0, &pid) || pid < 0) {
>> +		ret = -EINVAL;
> rdt_last_cmd_puts() to record the error.
Sure.
>
>> +		goto exit;
>>  	}
>> +
>>  	rdt_last_cmd_clear();
>>
>>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
>> -	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>> -		ret = -EINVAL;
>> +			rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>>  		rdt_last_cmd_puts("Pseudo-locking in progress\n");
>> -		goto unlock;
>> +		ret = -EINVAL;
>> +		goto exit;
>>  	}
>>
>>  	ret = rdtgroup_move_task(pid, rdtgrp, of);
> Do you want to exit if there is error in rdtgroup_move_task()?
> Otherwise, the failure won't be captured if later take movement succeeds.

Yes, that makes more sense. Exit on a failed movement and record the error.

Thanks

Babu

>
>> -unlock:
>> +	goto next;
>> +
>> +exit:
>> +	cpus_read_unlock();
>>  	rdtgroup_kn_unlock(of->kn);
>>
>>  	return ret ?: nbytes;
>>
> Thanks.
>
> -Fenghua
diff mbox series

Patch

diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
index 71a531061e4e..f26e16412bcb 100644
--- a/Documentation/x86/resctrl.rst
+++ b/Documentation/x86/resctrl.rst
@@ -208,12 +208,13 @@  All groups contain the following files:
 "tasks":
 	Reading this file shows the list of all tasks that belong to
 	this group. Writing a task id to the file will add a task to the
-	group. If the group is a CTRL_MON group the task is removed from
-	whichever previous CTRL_MON group owned the task and also from
-	any MON group that owned the task. If the group is a MON group,
-	then the task must already belong to the CTRL_MON parent of this
-	group. The task is removed from any previous MON group.
-
+	group. Multiple tasks can be assigned at once with each task
+	separated by commas. If the group is a CTRL_MON group the task
+	is removed from whichever previous CTRL_MON group owned the task
+	and also from any MON group that owned the task. If the group is
+	a MON group, then the task must already belong to the CTRL_MON
+	parent of this group. The task is removed from any previous MON
+	group.
 
 "cpus":
 	Reading this file shows a bitmask of the logical CPUs owned by
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e5a48f05e787..344607853f4c 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -686,28 +686,49 @@  static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
 				    char *buf, size_t nbytes, loff_t off)
 {
 	struct rdtgroup *rdtgrp;
+	char *pid_str;
 	int ret = 0;
 	pid_t pid;
 
-	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
+	/* Valid input requires a trailing newline */
+	if (nbytes == 0 || buf[nbytes - 1] != '\n')
 		return -EINVAL;
+
+	buf[nbytes - 1] = '\0';
+
+	cpus_read_lock();
 	rdtgrp = rdtgroup_kn_lock_live(of->kn);
 	if (!rdtgrp) {
-		rdtgroup_kn_unlock(of->kn);
-		return -ENOENT;
+		ret = -ENOENT;
+		goto exit;
+	}
+
+next:
+	if (!buf || buf[0] == '\0')
+		goto exit;
+
+	pid_str = strim(strsep(&buf, ","));
+
+	if (kstrtoint(pid_str, 0, &pid) || pid < 0) {
+		ret = -EINVAL;
+		goto exit;
 	}
+
 	rdt_last_cmd_clear();
 
 	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
-	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
-		ret = -EINVAL;
+			rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
 		rdt_last_cmd_puts("Pseudo-locking in progress\n");
-		goto unlock;
+		ret = -EINVAL;
+		goto exit;
 	}
 
 	ret = rdtgroup_move_task(pid, rdtgrp, of);
 
-unlock:
+	goto next;
+
+exit:
+	cpus_read_unlock();
 	rdtgroup_kn_unlock(of->kn);
 
 	return ret ?: nbytes;