Skip to contentSkip to conversation

x86/intel_rdt: Add cpus_list rdtgroup file

PATCH In review Jiri Olsa 5 participants 17 messages Mar 29, 2017 – Apr 10, 2017

Subsystems (2)
Documentation x86
lore
Share & tools
lore

Jiri Olsa

author
#0 lore raw

PATCH 1/1 x86/intel_rdt: Add cpus_list rdtgroup file

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
Cc Fenghua Yu <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Shaohua Li <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 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(-)
arch/x86/include/asm/intel_rdt.h +12 -4
index 0d64397cee58..0e74e58e1f23 100644
@@ -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);
 };
 
 /**

@@ -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);
 
 /*
  * intel_rdt_sched_in() - Writes the task's CLOSid to IA32_PQR_MSR
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +31 -11

115 lines not on this page · raw message

arch/x86/kernel/cpu/intel_rdt_schemata.c +4 -2
index f369cb8db0d5..3780ebebf36c 100644
@@ -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;
Signature · 1 line
2.9.3
#1 lore raw
Quoted 3 lines · Jiri Olsa

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?

Quoted 49 lines

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 <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Shaohua Li <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 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(-)
arch/x86/include/asm/intel_rdt.h +5 -1
index 0d64397cee58..0e74e58e1f23 100644
@@ -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,

arch/x86/include/asm/intel_rdt.h struct rftype { · line 72

Quoted 10 lines
arch/x86/include/asm/intel_rdt.h +3 -1
+			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.

Quoted 4 lines
arch/x86/include/asm/intel_rdt.h +0 -0
 };
 
 /**

arch/x86/include/asm/intel_rdt.h void rdt_cbm_update(void *arg); · line 192

Quoted 10 lines
arch/x86/include/asm/intel_rdt.h +4 -2
@@ -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.

Quoted 8 lines
arch/x86/include/asm/intel_rdt.h +0 -0
 /*
  * intel_rdt_sched_in() - Writes the task's CLOSid to IA32_PQR_MSR
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +0 -0
index 9ac2a5cdd9c2..3a1dfb421dc5 100644

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdtgroup_seqfile_show(struct seq_file *m, void *arg) · line 157

Quoted 6 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +1 -1
@@ -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.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static ssize_t rdtgroup_file_write(struct kernfs_open_file *of, char *buf, · line 169

Quoted 10 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +1 -1
@@ -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.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static struct kernfs_ops rdtgroup_kf_single_ops = { · line 181

Quoted 9 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +3 -1
@@ -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";

Quoted 12 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +1 -1
 	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);

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c rdt_update_closid(const struct cpumask *cpu_mask, int *closid) · line 239

Quoted 9 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +2 -1
@@ -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.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, · line 258

Quoted 6 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +1 -1
@@ -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"))

Quoted 8 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +4 -0
+		ret = cpulist_parse(buf, newmask);
+	else
+		ret = cpumask_parse(buf, newmask);
+
 	if (ret)
 		goto unlock;

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdtgroup_move_task(pid_t pid, struct rdtgroup *rdtgrp, · line 429

Quoted 9 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +2 -1
@@ -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.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static void show_rdt_tasks(struct rdtgroup *r, struct seq_file *s) · line 463

Quoted 10 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +2 -1
@@ -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.

Quoted 5 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +1 -0
@@ -473,6 +482,14 @@ static struct rftype rdtgroup_base_files[] = {
 		.seq_show	= rdtgroup_cpus_show,
 	},
 	{
+		.name		= "cpus_list",

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static struct rftype rdtgroup_base_files[] = { · line 495

Quoted 10 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +7 -0
+		.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.

Quoted 57 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +6 -3
@@ -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;
arch/x86/kernel/cpu/intel_rdt_schemata.c +4 -2
index f369cb8db0d5..3780ebebf36c 100644
@@ -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;
Signature · 1 line
2.9.3
#2 lore raw
Quoted 5 lines · Fenghua Yu
Quoted 3 lines · Jiri Olsa

While playing with the resctrl interface I found it much easier to deal with cpumask list rather than just regular cpumask.

Quoted 7 lines

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?

well I'm not aware about any such tool so I used resctrl interface directly, and in that case it was much simpler

jirka

#3 lore raw
Quoted 15 lines · Jiri Olsa
Quoted 5 lines · Fenghua Yu
Quoted 3 lines · Jiri Olsa

While playing with the resctrl interface I found it much easier to deal with cpumask list rather than just regular cpumask.

Quoted 7 lines

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?

Quoted 2 lines

well I'm not aware about any such tool so I used resctrl interface directly, and in that case it was much simpler

is there any tool available for this actualy?

thanks,
jirka

#4 lore raw
Quoted 16 lines · Jiri Olsa
Quoted 15 lines · Jiri Olsa
Quoted 12 lines · Fenghua Yu
Quoted 3 lines · Jiri Olsa

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?

Quoted 4 lines
Quoted 2 lines

well I'm not aware about any such tool so I used resctrl interface directly, and in that case it was much simpler

is there any tool available for this actualy?

Here is a tool created by Marcelo:
https://lwn.net/Articles/710514/

Thanks.

-Fenghua

#5 lore raw
In reply to Fenghua Yu
Quoted 5 lines · Fenghua Yu
Quoted 3 lines · Jiri Olsa

While playing with the resctrl interface I found it much easier to deal with cpumask list rather than just regular cpumask.

Quoted 7 lines

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?

Yes, because a lot of people including me do not care about these tools at all. Making it easy to read and write from the command line is the first thing to do.

Thanks,

tglx

#6 lore raw
Quoted 14 lines · Thomas Gleixner
Quoted 5 lines · Fenghua Yu
Quoted 3 lines · Jiri Olsa

While playing with the resctrl interface I found it much easier to deal with cpumask list rather than just regular cpumask.

Quoted 7 lines

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?

Quoted 3 lines

Yes, because a lot of people including me do not care about these tools at all. Making it easy to read and write from the command line is the first thing to do.

Sure, you are right. Hopefully adding a bit more explanation in the commit message will be helpful.

Thanks.

-Fenghua

#7 lore raw
In reply to Fenghua Yu
Quoted 9 lines · Fenghua Yu

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static struct kernfs_ops rdtgroup_kf_single_ops = { · line 181

Quoted 6 lines

static int rdtgroup_cpus_show(struct kernfs_open_file *of,

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +3 -1
-			      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;

Thanks,

tglx

#8 lore raw
Quoted 11 lines · Thomas Gleixner
Quoted 9 lines · Fenghua Yu

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static struct kernfs_ops rdtgroup_kf_single_ops = { · line 181

Quoted 6 lines

static int rdtgroup_cpus_show(struct kernfs_open_file *of,

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +3 -1
-			      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";

Quoted 7 lines

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;

You are absolutely correct.

Thanks.

-Fenghua

#9 lore raw
In reply to Thomas Gleixner
Quoted 11 lines · Thomas Gleixner
Quoted 9 lines · Fenghua Yu

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static struct kernfs_ops rdtgroup_kf_single_ops = { · line 181

Quoted 6 lines

static int rdtgroup_cpus_show(struct kernfs_open_file *of,

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +3 -1
-			      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";

Quoted 7 lines

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

thanks,
jirka

#10 lore raw

PATCH v2 1/1 x86/intel_rdt: Add cpus_list rdtgroup file

Quoted 14 lines · Jiri Olsa
Quoted 13 lines · Thomas Gleixner
Quoted 9 lines · Fenghua Yu

Review

Quoted 6 lines

static int rdtgroup_cpus_show(struct kernfs_open_file *of,

(quoted diff) +3 -1
-			      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?

Quoted 7 lines
Quoted 5 lines

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
Cc Fenghua Yu <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Shaohua Li <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 arch/x86/include/asm/intel_rdt.h         |  4 ++++
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 28 ++++++++++++++++++++++++----
 2 files changed, 28 insertions(+), 4 deletions(-)
arch/x86/include/asm/intel_rdt.h +4 -0
index 3f313991c0b3..a1a6681738a5 100644
@@ -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);
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +24 -4
index 9ac2a5cdd9c2..b089d2452f1e 100644
@@ -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
 		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,
Signature · 1 line
1.8.3.1
#11 lore raw
Quoted 36 lines · Jiri Olsa
Quoted 14 lines · Jiri Olsa
Quoted 13 lines · Thomas Gleixner
Quoted 9 lines · Fenghua Yu

Review

Quoted 6 lines

static int rdtgroup_cpus_show(struct kernfs_open_file *of,

(quoted diff) +3 -1
-			      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?

Quoted 7 lines
Quoted 5 lines

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.

Quoted 10 lines
  # 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.

Quoted 11 lines
Cc Fenghua Yu <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Shaohua Li <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 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?

Quoted 41 lines
arch/x86/include/asm/intel_rdt.h +4 -0
index 3f313991c0b3..a1a6681738a5 100644
@@ -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);
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +7 -0
index 9ac2a5cdd9c2..b089d2452f1e 100644
@@ -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)
 {

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdtgroup_cpus_show(struct kernfs_open_file *of, · line 195

Quoted 10 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +4 -3
@@ -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.

Quoted 34 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +13 -1
 		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,
Signature · 1 line
1.8.3.1
#12 lore raw

PATCH v3 1/1 x86/intel_rdt: Add cpus_list rdtgroup file

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
Cc Fenghua Yu <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Shaohua Li <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 arch/x86/include/asm/intel_rdt.h         |  4 ++++
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 28 ++++++++++++++++++++++++----
 2 files changed, 28 insertions(+), 4 deletions(-)
arch/x86/include/asm/intel_rdt.h +4 -0
index 3f313991c0b3..a1a6681738a5 100644
@@ -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);
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +24 -4
index 9ac2a5cdd9c2..b089d2452f1e 100644
@@ -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
 		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,
Signature · 1 line
1.8.3.1
#13 lore raw

arch/x86/include/asm/intel_rdt.h struct rftype { · line 62

Quoted 8 lines · Jiri Olsa

/* List of all resource groups */
extern struct list_head rdt_all_groups;

arch/x86/include/asm/intel_rdt.h +1 -0
@@ -56,6 +59,7 @@ struct rftype {
 	char			*name;
 	umode_t			mode;
 	struct kernfs_ops	*kf_ops;
+	unsigned long		flags;

Lacks update in the kernel-doc comment.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdtgroup_cpus_show(struct kernfs_open_file *of, · line 196

Quoted 12 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +4 -3
@@ -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
 		ret = -ENOENT;

This lacks braces around the else path

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdtgroup_tasks_show(struct kernfs_open_file *of, · line 494

Quoted 11 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +7 -0
@@ -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,
+	},

Please add the new file to Documentation/x86/intel_rdt_ui.txt

Thanks,

tglx

#14 lore raw

PATCH v4 1/1 x86/intel_rdt: Add cpus_list rdtgroup file

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
Cc Fenghua Yu <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Shaohua Li <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 Documentation/x86/intel_rdt_ui.txt       |  3 +++
 arch/x86/include/asm/intel_rdt.h         |  5 +++++
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 29 +++++++++++++++++++++++++----
 3 files changed, 33 insertions(+), 4 deletions(-)
Documentation/x86/intel_rdt_ui.txt +3 -0
index 3ea198460469..7029071e2373 100644
@@ -59,6 +59,9 @@ There are three files associated with each group:
 	given to the default (root) group. You cannot remove CPUs
 	from the default group.
 
+"cpus_list": A bitmask *list* of logical CPUs assigned to this group.
+	Same rules apply like for the "cpus" file.
+
 "schemata": A list of all the resources available to this group.
 	Each resource has its own line and format - see below for
 	details.
arch/x86/include/asm/intel_rdt.h +5 -0
index 3f313991c0b3..9069402f7497 100644
@@ -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;
 

@@ -49,6 +52,7 @@ struct rdtgroup {
  * @name: file name
  * @mode: access mode
  * @kf_ops: operations
+ * flags: RFTYPE_FLAGS_* flags
  * @seq_show: show content of the file
  * @write: write to the file
  */

@@ -56,6 +60,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);
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +25 -4
index 9ac2a5cdd9c2..56b5bdaf98e1 100644
@@ -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,10 +189,12 @@ 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 {
 		ret = -ENOENT;
+	}
 	rdtgroup_kn_unlock(of->kn);
 
 	return ret;

@@ -252,7 +261,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 +486,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,
Signature · 1 line
1.8.3.1
merge bot
#15 lore raw
x86/intel_rdt: Add cpus_list rdtgroup file
Commit-ID:  4ffa3c977b5da2907eb294dc6d0259a02f2284aa
Gitweb:     http://git.kernel.org/tip/4ffa3c977b5da2907eb294dc6d0259a02f2284aa
Author:     Jiri Olsa <[email protected]>
AuthorDate: Mon, 10 Apr 2017 16:52:32 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Mon, 10 Apr 2017 19:10:25 +0200

x86/intel_rdt: Add cpus_list rdtgroup file

The resource control filesystem provides only a bitmask based cpus file for assigning CPUs to a resource group. That's cumbersome with large cpumasks and non-intuitive when modifying the file from the command line.

Range based cpu lists are commonly used along with bitmask based cpu files in various subsystems throughout the kernel.

Add 'cpus_list' file which is CPU range based.

  # 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

[ tglx: Massaged changelog and replaced "bitmask lists" by "CPU ranges" ]

Signed-off-by Jiri Olsa <[email protected]>
Cc Fenghua Yu <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Shaohua Li <[email protected]>
Link http://lkml.kernel.org/r/20170410145232.GF25354@krava open here
Signed-off-by Thomas Gleixner <[email protected]>
---
 Documentation/x86/intel_rdt_ui.txt       |  3 +++
 arch/x86/include/asm/intel_rdt.h         |  5 +++++
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 29 +++++++++++++++++++++++++----
 3 files changed, 33 insertions(+), 4 deletions(-)
Documentation/x86/intel_rdt_ui.txt +3 -0
index 3ea1984..a1ace91 100644
@@ -59,6 +59,9 @@ There are three files associated with each group:
 	given to the default (root) group. You cannot remove CPUs
 	from the default group.
 
+"cpus_list": One or more CPU ranges of logical CPUs assigned to this
+	     group. Same rules apply like for the "cpus" file.
+
 "schemata": A list of all the resources available to this group.
 	Each resource has its own line and format - see below for
 	details.
arch/x86/include/asm/intel_rdt.h +5 -0
index 9030bcc..611c823 100644
@@ -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;
 

@@ -49,6 +52,7 @@ int __init rdtgroup_init(void);
  * @name:	File name
  * @mode:	Access mode
  * @kf_ops:	File operations
+ * @flags:	File specific RFTYPE_FLAGS_* flags
  * @seq_show:	Show content of the file
  * @write:	Write to the file
  */

@@ -56,6 +60,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);
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +25 -4
index c05509d..8401cf5 100644
@@ -174,6 +174,13 @@ static struct kernfs_ops rdtgroup_kf_single_ops = {
 	.seq_show		= rdtgroup_seqfile_show,
 };
 
+static bool is_cpu_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,10 +189,12 @@ 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_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
+			   cpumask_pr_args(&rdtgrp->cpu_mask));
+	} else {
 		ret = -ENOENT;
+	}
 	rdtgroup_kn_unlock(of->kn);
 
 	return ret;

@@ -252,7 +261,11 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
 		goto unlock;
 	}
 
-	ret = cpumask_parse(buf, newmask);
+	if (is_cpu_list(of))
+		ret = cpulist_parse(buf, newmask);
+	else
+		ret = cpumask_parse(buf, newmask);
+
 	if (ret)
 		goto unlock;
 

@@ -473,6 +486,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,
#16 lore raw
In reply to Fenghua Yu
Quoted 11 lines · Fenghua Yu

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdtgroup_cpus_show(struct kernfs_open_file *of, · line 195

Quoted 9 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +4 -3
 	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.

Wrong. I've explained that a gazillion of times already.

http://lkml.kernel.org/r/alpine.DEB.2.20.1701171956290.3645@nanos

So yes, the else path wants braces.

Thanks,

tglx