Re: [PATCHv2] x86/intel_rdt: Add cpus_list rdtgroup file

[Fenghua Yu]

On Sun, Apr 09, 2017 at 06:49:29PM +0200, Jiri Olsa wrote:
> On Fri, Mar 31, 2017 at 05:55:12PM +0200, Jiri Olsa wrote:
> > On Fri, Mar 31, 2017 at 11:15:22AM +0200, Thomas Gleixner wrote:
> > > On Wed, 29 Mar 2017, Fenghua Yu wrote:
> > > > >  static int rdtgroup_cpus_show(struct kernfs_open_file *of,
> > > > > -			      struct seq_file *s, void *v)
> > > > > +			      struct seq_file *s, void *v,
> > > > > +			      unsigned long flags)
> > > > >  {
> > > > > +	const char *fmt = flags & RFTYPE_FLAGS_CPUS_LIST ? "%*pbl\n" : "%*pb\n";
> > > > 
> > > > Change to:
> > > > + const char *fmt = strcmp(of->kn->priv, "cpus") ? "%*pbl\n" : "%*pb\n";
> > > 
> > > You couldn't come up with a more horrible hack, right?
> > > 
> > > Jiri was right with adding the flag to the base files, just the propagation
> > > through the callbacks sucks. What's wrong with:
> > > 
> > > 	struct rftype *rft = of->kn->priv;
> > > 	bool list = rtf->flags & RFTYPE_FLAGS_CPUS_LIST;
> > 
> > ok, I'll post the change
> 
> v2 attached
> 
> thanks,
> jirka
> 
> ---
> While playing with the resctrl interface I found it much
> easier to deal with cpumask list rather than just regular
> cpumask.
> 
> Adding cpus_list file to provide cpumask list interface
> to define group's cpus.
> 
>   # cd /sys/fs/resctrl/
>   # echo 1-10 > krava/cpus_list
>   # cat krava/cpus_list
>   1-10
>   # cat krava/cpus
>   0007fe
>   # cat cpus
>   fffff9
>   # cat cpus_list
>   0,3-23

Could you please have a right commit message and patch format?
git am can not pick up your commit message as you expected.

> 
> Cc: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> Cc: Mike Galbraith <efault@xxxxxx>
> Cc: Shaohua Li <shli@xxxxxx>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/intel_rdt.h         |  4 ++++
>  arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 28 ++++++++++++++++++++++++----
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 

Could you please add cpus_list info in intel_rdt_ui.txt?

> diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
> index 3f313991c0b3..a1a6681738a5 100644
> --- a/arch/x86/include/asm/intel_rdt.h
> +++ b/arch/x86/include/asm/intel_rdt.h
> @@ -37,6 +37,9 @@ struct rdtgroup {
>  /* rdtgroup.flags */
>  #define	RDT_DELETED		1
>  
> +/* rftype.flags */
> +#define RFTYPE_FLAGS_CPUS_LIST	1
> +
>  /* List of all resource groups */
>  extern struct list_head rdt_all_groups;
>  
> @@ -56,6 +59,7 @@ struct rftype {
>  	char			*name;
>  	umode_t			mode;
>  	struct kernfs_ops	*kf_ops;
> +	unsigned long		flags;
>  
>  	int (*seq_show)(struct kernfs_open_file *of,
>  			struct seq_file *sf, void *v);
> diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> index 9ac2a5cdd9c2..b089d2452f1e 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> @@ -174,6 +174,13 @@ static ssize_t rdtgroup_file_write(struct kernfs_open_file *of, char *buf,
>  	.seq_show		= rdtgroup_seqfile_show,
>  };
>  
> +static bool is_list(struct kernfs_open_file *of)
> +{
> +	struct rftype *rft = of->kn->priv;
> +
> +	return rft->flags & RFTYPE_FLAGS_CPUS_LIST;
> +}
> +
>  static int rdtgroup_cpus_show(struct kernfs_open_file *of,
>  			      struct seq_file *s, void *v)
>  {
> @@ -182,9 +189,10 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
>  
>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>  
> -	if (rdtgrp)
> -		seq_printf(s, "%*pb\n", cpumask_pr_args(&rdtgrp->cpu_mask));
> -	else
> +	if (rdtgrp) {
> +		seq_printf(s, is_list(of) ? "%*pbl\n" : "%*pb\n",
> +			   cpumask_pr_args(&rdtgrp->cpu_mask));
> +	} else

Unbalanced braces around if-else. You can remove the { and } after if.

>  		ret = -ENOENT;
>  	rdtgroup_kn_unlock(of->kn);
>  
> @@ -252,7 +260,11 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
>  		goto unlock;
>  	}
>  
> -	ret = cpumask_parse(buf, newmask);
> +	if (is_list(of))
> +		ret = cpulist_parse(buf, newmask);
> +	else
> +		ret = cpumask_parse(buf, newmask);
> +
>  	if (ret)
>  		goto unlock;
>  
> @@ -473,6 +485,14 @@ static int rdtgroup_tasks_show(struct kernfs_open_file *of,
>  		.seq_show	= rdtgroup_cpus_show,
>  	},
>  	{
> +		.name		= "cpus_list",
> +		.mode		= 0644,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.write		= rdtgroup_cpus_write,
> +		.seq_show	= rdtgroup_cpus_show,
> +		.flags		= RFTYPE_FLAGS_CPUS_LIST,
> +	},
> +	{
>  		.name		= "tasks",
>  		.mode		= 0644,
>  		.kf_ops		= &rdtgroup_kf_single_ops,
> -- 
> 1.8.3.1
>