Message-ID: <9af9eb7a-476c-d4b0-e114-3f5c2b45dd95@intel.com>
Date:   Wed, 15 Mar 2023 11:32:45 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Babu Moger <babu.moger@....com>, <corbet@....net>,
        <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>
CC:     <fenghua.yu@...el.com>, <dave.hansen@...ux.intel.com>,
        <x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
        <akpm@...ux-foundation.org>, <quic_neeraju@...cinc.com>,
        <rdunlap@...radead.org>, <damien.lemoal@...nsource.wdc.com>,
        <songmuchun@...edance.com>, <peterz@...radead.org>,
        <jpoimboe@...nel.org>, <pbonzini@...hat.com>,
        <chang.seok.bae@...el.com>, <pawan.kumar.gupta@...ux.intel.com>,
        <jmattson@...gle.com>, <daniel.sneddon@...ux.intel.com>,
        <sandipan.das@....com>, <tony.luck@...el.com>,
        <james.morse@....com>, <linux-doc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <bagasdotme@...il.com>,
        <eranian@...gle.com>, <christophe.leroy@...roup.eu>,
        <jarkko@...nel.org>, <adrian.hunter@...el.com>,
        <quic_jiles@...cinc.com>, <peternewman@...gle.com>
Subject: Re: [PATCH v3 1/7] x86/resctrl: Add multiple tasks to the resctrl
 group at once

Hi Babu,

On 3/2/2023 12:24 PM, Babu Moger wrote:
> The resctrl task assignment for MONITOR or CONTROL group needs to be
> done 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. Also,
> there is a syscall overhead for each command executed from user space.

To support this change it may also be helpful to add that
moving tasks take the mutex so attempting to move tasks in
parallel will not achieve a significant performance gain.

> 
> It can be improved by supporting the multiple task assignment in one
> command with the tasks separated by commas. For example:
> 
>   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
> 
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
>  Documentation/x86/resctrl.rst          |   11 +++++++++--
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   24 +++++++++++++++++++++++-
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> index 058257dc56c8..25203f20002d 100644
> --- a/Documentation/x86/resctrl.rst
> +++ b/Documentation/x86/resctrl.rst
> @@ -292,13 +292,20 @@ 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
> +	group. Multiple tasks can be assigned together in one command by
> +	inputting the tasks separated by commas. Tasks will be assigned

How about "tasks separated" -> "task ids separated" or "by inputting the tasks
separated by commas" -> "by separating the task ids with commas"

> +	sequentially in the order it is provided. Failure while assigning
> +	the tasks will be aborted immediately. The tasks before the failure
> +	will be assigned and the tasks next in the sequence will not be
> +	assigned. Users may need to retry them again. The failure details
> +	will be logged in resctrl/info/last_cmd_status file.

Please use full path as is done in rest of doc.

> +
> +	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.
>  
> -

Why is this line removal needed?

>  "cpus":
>  	Reading this file shows a bitmask of the logical CPUs owned by
>  	this group. Writing a mask to this file will add and remove
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index e2c1599d1b37..15ea5b550fe9 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -683,16 +683,34 @@ 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;

The resctrl files should be seen as user space API. With the above change
you take an interface that did not require a newline and dictate that it
should have a trailing newline. How convinced are you that this does not
break any current user space scripts or applications? Why does this
feature require a trailing newline?

> +
> +	buf[nbytes - 1] = '\0';
> +
>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>  	if (!rdtgrp) {
>  		rdtgroup_kn_unlock(of->kn);
>  		return -ENOENT;
>  	}
> +
> +next:
> +	if (!buf || buf[0] == '\0')
> +		goto unlock;
> +
> +	pid_str = strim(strsep(&buf, ","));
> +

Could lib/cmdline.c:get_option() be useful?

> +	if (kstrtoint(pid_str, 0, &pid) || pid < 0) {
> +		rdt_last_cmd_printf("Invalid pid %d value\n", pid);

This is risky. What will pid be if kstrtoint() failed? 

> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
>  	rdt_last_cmd_clear();
>  
>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> @@ -703,6 +721,10 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>  	}
>  
>  	ret = rdtgroup_move_task(pid, rdtgrp, of);
> +	if (ret)
> +		goto unlock;
> +	else
> +		goto next;
>  

The documentation states "The failure details will be logged
in resctrl/info/last_cmd_status file." but I do not see how
this is happening here. From what I can tell this implementation
does not do anything beyond what last_cmd_status already does
so any special mention in the docs is not clear to me. The
cover letter stated "Added pid in last_cmd_status when
applicable." - it sounded as though last_cmd_status would
contain the error with the pid that encountered the error but
I do not see this happening here.

>  unlock:
>  	rdtgroup_kn_unlock(of->kn);
> 
> 

Reinette

Powered by blists - more mailing lists