lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <MW3PR12MB45537DC45130BB5F02F0F3F095BC9@MW3PR12MB4553.namprd12.prod.outlook.com>
Date:   Thu, 16 Mar 2023 16:27:21 +0000
From:   "Moger, Babu" <Babu.Moger@....com>
To:     Reinette Chatre <reinette.chatre@...el.com>,
        "corbet@....net" <corbet@....net>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>
CC:     "fenghua.yu@...el.com" <fenghua.yu@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
        "paulmck@...nel.org" <paulmck@...nel.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "quic_neeraju@...cinc.com" <quic_neeraju@...cinc.com>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "damien.lemoal@...nsource.wdc.com" <damien.lemoal@...nsource.wdc.com>,
        "songmuchun@...edance.com" <songmuchun@...edance.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "jpoimboe@...nel.org" <jpoimboe@...nel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "chang.seok.bae@...el.com" <chang.seok.bae@...el.com>,
        "pawan.kumar.gupta@...ux.intel.com" 
        <pawan.kumar.gupta@...ux.intel.com>,
        "jmattson@...gle.com" <jmattson@...gle.com>,
        "daniel.sneddon@...ux.intel.com" <daniel.sneddon@...ux.intel.com>,
        "Das1, Sandipan" <Sandipan.Das@....com>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "james.morse@....com" <james.morse@....com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bagasdotme@...il.com" <bagasdotme@...il.com>,
        "eranian@...gle.com" <eranian@...gle.com>,
        "christophe.leroy@...roup.eu" <christophe.leroy@...roup.eu>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "adrian.hunter@...el.com" <adrian.hunter@...el.com>,
        "quic_jiles@...cinc.com" <quic_jiles@...cinc.com>,
        "peternewman@...gle.com" <peternewman@...gle.com>
Subject: RE: [PATCH v3 1/7] x86/resctrl: Add multiple tasks to the resctrl
 group at once

[AMD Official Use Only - General]

Hi Reinette,

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@...el.com>
> Sent: Wednesday, March 15, 2023 1:33 PM
> To: Moger, Babu <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; Das1, Sandipan
> <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.

Agree. It may not be significant performance gain.  Will remove this line. 
> 
> >
> > 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"


Will change it to " Multiple tasks can be assigned together in one command 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.

Ok. Sure

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

Not 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?

I have tested these changes with intel_cmt_cat tool. It didn’t have any problems. 
We are already doing newline check for few other inputs.

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

Yes. We could that also. May not be required for the simple case like this.

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

Yea. I need to separate these failure cases. One is parsing error, and another is invalid pid.

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

You are right we are not doing anything special here. pid failures error was already there.
I will have to change the text here.
Thanks
Babu

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ