Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1734516 > unrolled thread

[RFC 0/5] x86/intel_rdt: Better diagnostics

Back to article view | Back to linux.kernel

Contents

  [RFC 0/5] x86/intel_rdt: Better diagnostics "Luck, Tony" <tony.luck@intel.com> - 2017-09-19 00:20 +0200
    [PATCH 2/5] x86/intel_rdt: Add diagnostics when writing the schemata file "Luck, Tony" <tony.luck@intel.com> - 2017-09-19 00:20 +0200
      Re: [PATCH 2/5] x86/intel_rdt: Add diagnostics when writing the  schemata file Thomas Gleixner <tglx@linutronix.de> - 2017-09-25 16:10 +0200
        Re: [PATCH 2/5] x86/intel_rdt: Add diagnostics when writing the  schemata file "Luck, Tony" <tony.luck@intel.com> - 2017-09-25 23:40 +0200
          RE: [PATCH 2/5] x86/intel_rdt: Add diagnostics when writing the  schemata file "Luck, Tony" <tony.luck@intel.com> - 2017-09-26 00:20 +0200
          Re: [PATCH 2/5] x86/intel_rdt: Add diagnostics when writing the  schemata file Thomas Gleixner <tglx@linutronix.de> - 2017-09-26 00:20 +0200
    Re: [RFC 0/5] x86/intel_rdt: Better diagnostics Steven Rostedt <rostedt@goodmis.org> - 2017-09-19 03:20 +0200
    [PATCH 6/5] x86/intel_rdt: Add documentation for "info/last_cmd_status" "Luck, Tony" <tony.luck@intel.com> - 2017-09-20 01:20 +0200
    Re: [RFC 0/5] x86/intel_rdt: Better diagnostics Borislav Petkov <bp@suse.de> - 2017-09-21 14:10 +0200

#1734516 — [RFC 0/5] x86/intel_rdt: Better diagnostics

From: Tony Luck <tony.luck@intel.com>

Chatting online with Boris to diagnose why his test cases for RDT
weren't working, we came up with either a good idea (in which case
I credit Boris) or a dumb one (in which case this is all my fault).

The basic problem is that there aren't many good error codes for
a file system interface to pass back to the user.  I'd resisted
adding printk() calls because it is a pain to parse the console
log, doubly so if you want to do it from a shell script that is
actually issuing the commands to RDT.

The answer is to add new file in the "info" directory that gives
the status of the last "command" to RDT (either a mkdir, or a
write to one of the control files).

I used the seq_buf* framework because I initially thought a single
command might result in multiple messages. But currently that isn't
true and we could potentially just use "strcpy()/sprintf()" to a
fixed buffer.  I didn't switch to that because the seq_buf* seems
very lightweight and allows for future extra messages while including
checking for exceeding the length of the buffer.

Tony Luck (5):
  x86/intel_rdt: Add framework for better RDT UI diagnostics
  x86/intel_rdt: Add diagnostics when writing the schemata file
  x86/intel_rdt: Add diagnostics when writing the tasks file
  x86/intel_rdt: Add diagnostics when writing the cpus file
  x86/intel_rdt: Add diagnostics when making directories

 arch/x86/kernel/cpu/intel_rdt.h             |  6 ++
 arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c | 61 +++++++++++++++----
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c    | 93 +++++++++++++++++++++++++----
 3 files changed, 137 insertions(+), 23 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1734517 — [PATCH 2/5] x86/intel_rdt: Add diagnostics when writing the schemata file

From: Tony Luck <tony.luck@intel.com>

Save helpful descriptions of what went wrong when writing a
schemata file.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c | 61 +++++++++++++++++++++++------
 1 file changed, 50 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c b/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
index f6ea94f8954a..d87a060ab0db 100644
--- a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
@@ -42,15 +42,25 @@ static bool bw_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 	/*
 	 * Only linear delay values is supported for current Intel SKUs.
 	 */
-	if (!r->membw.delay_linear)
+	if (!r->membw.delay_linear) {
+		seq_buf_puts(&last_cmd_status,
+			     "No support for non-linear MB domains\n");
 		return false;
+	}
 
 	ret = kstrtoul(buf, 10, &bw);
-	if (ret)
+	if (ret) {
+		seq_buf_printf(&last_cmd_status,
+			       "Non-decimal digit in MB value %s\n", buf);
 		return false;
+	}
 
-	if (bw < r->membw.min_bw || bw > r->default_ctrl)
+	if (bw < r->membw.min_bw || bw > r->default_ctrl) {
+		seq_buf_printf(&last_cmd_status,
+			       "MB value %ld out of range [%d,%d]\n", bw,
+			       r->membw.min_bw, r->default_ctrl);
 		return false;
+	}
 
 	*data = roundup(bw, (unsigned long)r->membw.bw_gran);
 	return true;
@@ -60,8 +70,11 @@ int parse_bw(char *buf, struct rdt_resource *r, struct rdt_domain *d)
 {
 	unsigned long data;
 
-	if (d->have_new_ctrl)
+	if (d->have_new_ctrl) {
+		seq_buf_printf(&last_cmd_status, "duplicate domain %d\n",
+			       d->id);
 		return -EINVAL;
+	}
 
 	if (!bw_validate(buf, &data, r))
 		return -EINVAL;
@@ -84,20 +97,32 @@ static bool cbm_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 	int ret;
 
 	ret = kstrtoul(buf, 16, &val);
-	if (ret)
+	if (ret) {
+		seq_buf_printf(&last_cmd_status,
+			       "non-hex character in mask %s\n", buf);
 		return false;
+	}
 
-	if (val == 0 || val > r->default_ctrl)
+	if (val == 0 || val > r->default_ctrl) {
+		seq_buf_puts(&last_cmd_status, "mask out of range\n");
 		return false;
+	}
 
 	first_bit = find_first_bit(&val, cbm_len);
 	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
 
-	if (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)
+	if (find_next_bit(&val, cbm_len, zero_bit) < cbm_len) {
+		seq_buf_printf(&last_cmd_status,
+			       "mask %lx has non-consecutive 1-bits\n", val);
 		return false;
+	}
 
-	if ((zero_bit - first_bit) < r->cache.min_cbm_bits)
+	if ((zero_bit - first_bit) < r->cache.min_cbm_bits) {
+		seq_buf_printf(&last_cmd_status,
+			       "Need at least %d bits in mask\n",
+			       r->cache.min_cbm_bits);
 		return false;
+	}
 
 	*data = val;
 	return true;
@@ -111,8 +136,11 @@ int parse_cbm(char *buf, struct rdt_resource *r, struct rdt_domain *d)
 {
 	unsigned long data;
 
-	if (d->have_new_ctrl)
+	if (d->have_new_ctrl) {
+		seq_buf_printf(&last_cmd_status, "duplicate domain %d\n",
+			       d->id);
 		return -EINVAL;
+	}
 
 	if(!cbm_validate(buf, &data, r))
 		return -EINVAL;
@@ -139,8 +167,11 @@ static int parse_line(char *line, struct rdt_resource *r)
 		return 0;
 	dom = strsep(&line, ";");
 	id = strsep(&dom, "=");
-	if (!dom || kstrtoul(id, 10, &dom_id))
+	if (!dom || kstrtoul(id, 10, &dom_id)) {
+		seq_buf_puts(&last_cmd_status,
+			     "Missing '=' or non-numeric domain\n");
 		return -EINVAL;
+	}
 	dom = strim(dom);
 	list_for_each_entry(d, &r->domains, list) {
 		if (d->id == dom_id) {
@@ -196,6 +227,8 @@ static int rdtgroup_parse_resource(char *resname, char *tok, int closid)
 		if (!strcmp(resname, r->name) && closid < r->num_closid)
 			return parse_line(tok, r);
 	}
+	seq_buf_printf(&last_cmd_status,
+		       "unknown/unsupported resource name '%s'\n", resname);
 	return -EINVAL;
 }
 
@@ -208,14 +241,19 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 	char *tok, *resname;
 	int closid, ret = 0;
 
+	seq_buf_clear(&last_cmd_status);
+
 	/* Valid input requires a trailing newline */
-	if (nbytes == 0 || buf[nbytes - 1] != '\n')
+	if (nbytes == 0 || buf[nbytes - 1] != '\n') {
+		seq_buf_puts(&last_cmd_status, "no trailing newline\n");
 		return -EINVAL;
+	}
 	buf[nbytes - 1] = '\0';
 
 	rdtgrp = rdtgroup_kn_lock_live(of->kn);
 	if (!rdtgrp) {
 		rdtgroup_kn_unlock(of->kn);
+		seq_buf_puts(&last_cmd_status, "directory was removed\n");
 		return -ENOENT;
 	}
 
@@ -229,6 +267,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 	while ((tok = strsep(&buf, "\n")) != NULL) {
 		resname = strim(strsep(&tok, ":"));
 		if (!tok) {
+			seq_buf_puts(&last_cmd_status, "Missing ':'\n");
 			ret = -EINVAL;
 			goto out;
 		}
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1739079 — Re: [PATCH 2/5] x86/intel_rdt: Add diagnostics when writing the schemata file

On Mon, 18 Sep 2017, Luck, Tony wrote:
> @@ -208,14 +241,19 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>  	char *tok, *resname;
>  	int closid, ret = 0;
>  
> +	seq_buf_clear(&last_cmd_status);
> +
>  	/* Valid input requires a trailing newline */
> -	if (nbytes == 0 || buf[nbytes - 1] != '\n')
> +	if (nbytes == 0 || buf[nbytes - 1] != '\n') {
> +		seq_buf_puts(&last_cmd_status, "no trailing newline\n");
>  		return -EINVAL;
> +	}
>  	buf[nbytes - 1] = '\0';

In all other instances you access last_cmd_status within the
rdtgroup_kn_lock_live() protected section, which also serializes the show()
function via rdtgroup_mutex. Here you do it outside for obvious reasons,
but that opens a can of evil worms ...

Can you please provide and use two helpers - last_cmd_buf_clear() and
last_cmd_buf_puts() - which both have a
lockdep_assert_held(&rdtgroup_mutex) inside to make sure that we don't end
up with unprotected access accidentally?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1739289 — Re: [PATCH 2/5] x86/intel_rdt: Add diagnostics when writing the schemata file

On Mon, Sep 25, 2017 at 04:04:07PM +0200, Thomas Gleixner wrote:
> On Mon, 18 Sep 2017, Luck, Tony wrote:
> > @@ -208,14 +241,19 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> >  	char *tok, *resname;
> >  	int closid, ret = 0;
> >  
> > +	seq_buf_clear(&last_cmd_status);
> > +
> >  	/* Valid input requires a trailing newline */
> > -	if (nbytes == 0 || buf[nbytes - 1] != '\n')
> > +	if (nbytes == 0 || buf[nbytes - 1] != '\n') {
> > +		seq_buf_puts(&last_cmd_status, "no trailing newline\n");
> >  		return -EINVAL;
> > +	}
> >  	buf[nbytes - 1] = '\0';
> 
> In all other instances you access last_cmd_status within the
> rdtgroup_kn_lock_live() protected section, which also serializes the show()
> function via rdtgroup_mutex. Here you do it outside for obvious reasons,
> but that opens a can of evil worms ...

Indeed.

> Can you please provide and use two helpers - last_cmd_buf_clear() and
> last_cmd_buf_puts() - which both have a
> lockdep_assert_held(&rdtgroup_mutex) inside to make sure that we don't end
> up with unprotected access accidentally?

Sure. In progress. But I also need a last_cmd_printf(), which for some
reason is giving me grief.  In the header file I put:

+static inline void last_cmd_printf(const char *fmt, ...)
+{
+       va_list ap;
+
+       va_start(ap, fmt);
+       lockdep_assert_held(&rdtgroup_mutex);
+       seq_buf_printf(&last_cmd_status, fmt, ap);
+       va_end(ap);
+}

and use it like this:

+       last_cmd_printf("unknown/unsupported resource name '%s'\n", resname);

but the argument gets lost/mangled. Instead of the string that
should have appeared for the %s, I just get a \b

Also with nummeric arguments:

+               last_cmd_printf("mask %lx has non-consecutive 1-bits\n", val);

I get some kernel pointer looking value instead of "5":

	mask ffffa1ee62757c98 has non-consecutive 1-bits


Is there a limit on how many nested va_start()/va_end() can happen? Or is the
compiler confused because I made this "inline"? Or just a silly typo that I
can't see despite staring at it for a while?

-Tony

[toc] | [prev] | [next] | [standalone]

#1739303 — RE: [PATCH 2/5] x86/intel_rdt: Add diagnostics when writing the schemata file

> seq_buf_vprintf() is your friend. It takes va_list as last argument.

Reinette spotted that a couple of minutes ahead of you.

/me looks for paper bag to put over my head.

> While at it can you please make it a proper function? No point for inlining
> that.

There was a small point ... I need the function in two files ... so the inline keeps
me from polluting global name space.

But I guess I can call it rdt_last_cmd_printf() to limit the damage.

-Tony

[toc] | [prev] | [next] | [standalone]

#1739312 — Re: [PATCH 2/5] x86/intel_rdt: Add diagnostics when writing the schemata file

On Mon, 25 Sep 2017, Luck, Tony wrote:
> On Mon, Sep 25, 2017 at 04:04:07PM +0200, Thomas Gleixner wrote:
> > On Mon, 18 Sep 2017, Luck, Tony wrote:
> > > @@ -208,14 +241,19 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> > >  	char *tok, *resname;
> > >  	int closid, ret = 0;
> > >  
> > > +	seq_buf_clear(&last_cmd_status);
> > > +
> > >  	/* Valid input requires a trailing newline */
> > > -	if (nbytes == 0 || buf[nbytes - 1] != '\n')
> > > +	if (nbytes == 0 || buf[nbytes - 1] != '\n') {
> > > +		seq_buf_puts(&last_cmd_status, "no trailing newline\n");
> > >  		return -EINVAL;
> > > +	}
> > >  	buf[nbytes - 1] = '\0';
> > 
> > In all other instances you access last_cmd_status within the
> > rdtgroup_kn_lock_live() protected section, which also serializes the show()
> > function via rdtgroup_mutex. Here you do it outside for obvious reasons,
> > but that opens a can of evil worms ...
> 
> Indeed.
> 
> > Can you please provide and use two helpers - last_cmd_buf_clear() and
> > last_cmd_buf_puts() - which both have a
> > lockdep_assert_held(&rdtgroup_mutex) inside to make sure that we don't end
> > up with unprotected access accidentally?
> 
> Sure. In progress. But I also need a last_cmd_printf(), which for some
> reason is giving me grief.  In the header file I put:
> 
> +static inline void last_cmd_printf(const char *fmt, ...)
> +{
> +       va_list ap;
> +
> +       va_start(ap, fmt);
> +       lockdep_assert_held(&rdtgroup_mutex);
> +       seq_buf_printf(&last_cmd_status, fmt, ap);

seq_buf_vprintf() is your friend. It takes va_list as last argument.

While at it can you please make it a proper function? No point for inlining
that.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1734589

On Mon, 18 Sep 2017 15:18:38 -0700
"Luck, Tony" <tony.luck@intel.com> wrote:

> From: Tony Luck <tony.luck@intel.com>
> 
> Chatting online with Boris to diagnose why his test cases for RDT
> weren't working, we came up with either a good idea (in which case
> I credit Boris) or a dumb one (in which case this is all my fault).
> 
> The basic problem is that there aren't many good error codes for
> a file system interface to pass back to the user.  I'd resisted
> adding printk() calls because it is a pain to parse the console
> log, doubly so if you want to do it from a shell script that is
> actually issuing the commands to RDT.
> 
> The answer is to add new file in the "info" directory that gives
> the status of the last "command" to RDT (either a mkdir, or a
> write to one of the control files).
> 
> I used the seq_buf* framework because I initially thought a single
> command might result in multiple messages. But currently that isn't
> true and we could potentially just use "strcpy()/sprintf()" to a
> fixed buffer.  I didn't switch to that because the seq_buf* seems
> very lightweight and allows for future extra messages while including
> checking for exceeding the length of the buffer.
> 
> Tony Luck (5):
>   x86/intel_rdt: Add framework for better RDT UI diagnostics
>   x86/intel_rdt: Add diagnostics when writing the schemata file
>   x86/intel_rdt: Add diagnostics when writing the tasks file
>   x86/intel_rdt: Add diagnostics when writing the cpus file
>   x86/intel_rdt: Add diagnostics when making directories
> 
>  arch/x86/kernel/cpu/intel_rdt.h             |  6 ++
>  arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c | 61 +++++++++++++++----
>  arch/x86/kernel/cpu/intel_rdt_rdtgroup.c    | 93 +++++++++++++++++++++++++----
>  3 files changed, 137 insertions(+), 23 deletions(-)
> 

They all look fine to me.

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

[toc] | [prev] | [next] | [standalone]

#1735346 — [PATCH 6/5] x86/intel_rdt: Add documentation for "info/last_cmd_status"

From: Tony Luck <tony.luck@intel.com>

New file in the "info" directory helps diagnose what went wrong
when using the /sys/fs/resctrl file system

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
Oops ... forgot the Documentation ... here it is.

 Documentation/x86/intel_rdt_ui.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/x86/intel_rdt_ui.txt b/Documentation/x86/intel_rdt_ui.txt
index 4d8848e4e224..6851854cf69d 100644
--- a/Documentation/x86/intel_rdt_ui.txt
+++ b/Documentation/x86/intel_rdt_ui.txt
@@ -87,6 +87,17 @@ with the following files:
 			bytes) at which a previously used LLC_occupancy
 			counter can be considered for re-use.
 
+Finally, in the top level of the "info" directory there is a file
+named "last_cmd_status". This is reset with every "command" issued
+via the file system (making new directories or writing to any of the
+control files). If the command was successful, it will read as "ok".
+If the command failed, it will provide more information that can be
+conveyed in the error returns from file operations. E.g.
+
+	# echo L3:0=f7 > schemata
+	bash: echo: write error: Invalid argument
+	# cat info/last_cmd_status
+	mask f7 has non-consecutive 1-bits
 
 Resource alloc and monitor groups
 ---------------------------------
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1736605

On Mon, Sep 18, 2017 at 03:18:38PM -0700, Luck, Tony wrote:
> From: Tony Luck <tony.luck@intel.com>
> 
> Chatting online with Boris to diagnose why his test cases for RDT
> weren't working, we came up with either a good idea (in which case
> I credit Boris) or a dumb one (in which case this is all my fault).

Ha! I can share the fault, no worries :-)

I'll test them on my box when I get a chance.

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web