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

From: Fenghua Yu
Date: Wed Mar 29 2017 - 12:06:31 EST

Next message: Vlastimil Babka: "Re: [PATCH v3 7/8] mm, compaction: restrict async compaction to pageblocks of same migratetype"
Previous message: Arushi Singhal: "[PATCH v2] staging: iio: Remove unneeded parentheses."
In reply to: Jiri Olsa: "[PATCH] x86/intel_rdt: Add cpus_list rdtgroup file"
Next in thread: Jiri Olsa: "Re: [PATCH] x86/intel_rdt: Add cpus_list rdtgroup file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Mar 29, 2017 at 05:09:48PM +0200, Jiri Olsa wrote:
> While playing with the resctrl interface I found it much
> easier to deal with cpumask list rather than just regular
> cpumask.

Could you please explain specifically why and when it's easier
to deal with cpumask list? In programming cases, cpumask
and cpumask list are almost same. And people are working
on higher level tools to control resctrl. The tools can
hide detailed regular cpumask or cpumask list and user
doesn't need to care lower level format of cpumask. So
is it really useful to add cpus_list?

>
> 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
>
> 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 | 16 +++++++++---
> arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 42 +++++++++++++++++++++++---------
> arch/x86/kernel/cpu/intel_rdt_schemata.c | 6 +++--
> 3 files changed, 47 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
> index 0d64397cee58..0e74e58e1f23 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;
>
> @@ -54,16 +57,19 @@ 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);
> + struct seq_file *sf, void *v,
> + unsigned long flags);
> /*
> * write() is the generic write callback which maps directly to
> * kernfs write operation and overrides all other operations.
> * Maximum write size is determined by ->max_write_len.
> */
> ssize_t (*write)(struct kernfs_open_file *of,
> - char *buf, size_t nbytes, loff_t off);
> + char *buf, size_t nbytes, loff_t off,
> + unsigned long flags);

I think no need to use "flags". Withou adding this "flags", the patch
will be much concise.

> };
>
> /**
> @@ -179,9 +185,11 @@ void rdt_cbm_update(void *arg);
> struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn);
> void rdtgroup_kn_unlock(struct kernfs_node *kn);
> ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> - char *buf, size_t nbytes, loff_t off);
> + char *buf, size_t nbytes, loff_t off,
> + unsigned long flags);
> int rdtgroup_schemata_show(struct kernfs_open_file *of,
> - struct seq_file *s, void *v);
> + struct seq_file *s, void *v,
> + unsigned long flags);

No need for this hunk.

>
> /*
> * intel_rdt_sched_in() - Writes the task's CLOSid to IA32_PQR_MSR
> diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> index 9ac2a5cdd9c2..3a1dfb421dc5 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> @@ -153,7 +153,7 @@ static int rdtgroup_seqfile_show(struct seq_file *m, void *arg)
> struct rftype *rft = of->kn->priv;
>
> if (rft->seq_show)
> - return rft->seq_show(of, m, arg);
> + return rft->seq_show(of, m, arg, rft->flags);
> }

Ditto.

>
> @@ -163,7 +163,7 @@ static ssize_t rdtgroup_file_write(struct kernfs_open_file *of, char *buf,
> struct rftype *rft = of->kn->priv;
>
> if (rft->write)
> - return rft->write(of, buf, nbytes, off);
> + return rft->write(of, buf, nbytes, off, rft->flags);
>
> return -EINVAL;
> }

Ditto.

> @@ -175,15 +175,17 @@ static struct kernfs_ops rdtgroup_kf_single_ops = {
> };
>
> 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";

> struct rdtgroup *rdtgrp;
> int ret = 0;
>
> rdtgrp = rdtgroup_kn_lock_live(of->kn);
>
> if (rdtgrp)
> - seq_printf(s, "%*pb\n", cpumask_pr_args(&rdtgrp->cpu_mask));
> + seq_printf(s, fmt, cpumask_pr_args(&rdtgrp->cpu_mask));
> else
> ret = -ENOENT;
> rdtgroup_kn_unlock(of->kn);
> @@ -230,7 +232,8 @@ rdt_update_closid(const struct cpumask *cpu_mask, int *closid)
> }
>
> static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
> - char *buf, size_t nbytes, loff_t off)
> + char *buf, size_t nbytes, loff_t off,
> + unsigned long flags)
> {
> cpumask_var_t tmpmask, newmask;
> struct rdtgroup *rdtgrp, *r;

No need for this hunk.

> @@ -252,7 +255,11 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
> goto unlock;
> }
>
> - ret = cpumask_parse(buf, newmask);
> + if (flags & RFTYPE_FLAGS_CPUS_LIST)

Change to:
+ if (strcmp(of->kn->priv, "cpus"))

> + ret = cpulist_parse(buf, newmask);
> + else
> + ret = cpumask_parse(buf, newmask);
> +
> if (ret)
> goto unlock;
>
> @@ -415,7 +422,8 @@ static int rdtgroup_move_task(pid_t pid, struct rdtgroup *rdtgrp,
> }
>
> static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
> - char *buf, size_t nbytes, loff_t off)
> + char *buf, size_t nbytes, loff_t off,
> + unsigned long flags __maybe_unused)
> {
> struct rdtgroup *rdtgrp;
> int ret = 0;

No need for this hunk.

> @@ -448,7 +456,8 @@ static void show_rdt_tasks(struct rdtgroup *r, struct seq_file *s)
> }
>
> static int rdtgroup_tasks_show(struct kernfs_open_file *of,
> - struct seq_file *s, void *v)
> + struct seq_file *s, void *v,
> + unsigned long flags)
> {
> struct rdtgroup *rdtgrp;
> int ret = 0;

Ditto.

> @@ -473,6 +482,14 @@ static struct rftype rdtgroup_base_files[] = {
> .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,

No need for the rest of the patch.

> @@ -489,7 +506,8 @@ static struct rftype rdtgroup_base_files[] = {
> };
>
> static int rdt_num_closids_show(struct kernfs_open_file *of,
> - struct seq_file *seq, void *v)
> + struct seq_file *seq, void *v,
> + unsigned long flags __maybe_unused)
> {
> struct rdt_resource *r = of->kn->parent->priv;
>
> @@ -499,7 +517,8 @@ static int rdt_num_closids_show(struct kernfs_open_file *of,
> }
>
> static int rdt_cbm_mask_show(struct kernfs_open_file *of,
> - struct seq_file *seq, void *v)
> + struct seq_file *seq, void *v,
> + unsigned long flags __maybe_unused)
> {
> struct rdt_resource *r = of->kn->parent->priv;
>
> @@ -509,7 +528,8 @@ static int rdt_cbm_mask_show(struct kernfs_open_file *of,
> }
>
> static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
> - struct seq_file *seq, void *v)
> + struct seq_file *seq, void *v,
> + unsigned long flags __maybe_unused)
> {
> struct rdt_resource *r = of->kn->parent->priv;
>
> diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> index f369cb8db0d5..3780ebebf36c 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> @@ -132,7 +132,8 @@ static int update_domains(struct rdt_resource *r, int closid)
> }
>
> ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> - char *buf, size_t nbytes, loff_t off)
> + char *buf, size_t nbytes, loff_t off,
> + unsigned long flags)
> {
> struct rdtgroup *rdtgrp;
> struct rdt_resource *r;
> @@ -224,7 +225,8 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
> }
>
> int rdtgroup_schemata_show(struct kernfs_open_file *of,
> - struct seq_file *s, void *v)
> + struct seq_file *s, void *v,
> + unsigned long flags __maybe_unused)
> {
> struct rdtgroup *rdtgrp;
> struct rdt_resource *r;
> --
> 2.9.3
>

Next message: Vlastimil Babka: "Re: [PATCH v3 7/8] mm, compaction: restrict async compaction to pageblocks of same migratetype"
Previous message: Arushi Singhal: "[PATCH v2] staging: iio: Remove unneeded parentheses."
In reply to: Jiri Olsa: "[PATCH] x86/intel_rdt: Add cpus_list rdtgroup file"
Next in thread: Jiri Olsa: "Re: [PATCH] x86/intel_rdt: Add cpus_list rdtgroup file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]