x86/intel_rdt: Better diagnostics

PATCH New Luck, Tony 1 participant 7 messages Sep 25, 2017

Subsystems (2)

Documentation x86

lore

Share & tools

lore

Luck, Tony

author

2017-09-25 23:39 UTC #0 lore raw

v2 cover x86/intel_rdt: Better diagnostics

From: Tony Luck <[email protected]>

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.

V1->V2: tglx: Add wrappers around functions that clear and append
to the seq_buf to assert that we hold the rdtgroup_mutex [and drop the diagnostic from the place where we didn't]

Tony Luck (6):
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 x86/intel_rdt: Add documentation for "info/last_cmd_status"

 Documentation/x86/intel_rdt_ui.txt          |  11 +++
 arch/x86/kernel/cpu/intel_rdt.h             |   7 ++
 arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c |  45 ++++++++---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c    | 112 +++++++++++++++++++++++++---
 4 files changed, 153 insertions(+), 22 deletions(-)

Signature · 1 line

2.11.0

Luck, Tony

author

2017-09-25 23:40 UTC #1 lore raw

PATCH v2 1/6 x86/intel_rdt: Add framework for better RDT UI diagnostics

From: Tony Luck <[email protected]>

Commands are given to the resctrl file system by making/removing directories, or by writing to files. When something goes wrong the user is generally left wondering why they got:

bash: echo: write error: Invalid argument

Add a new file "last_cmd_status" to the "info" directory that will give the user some better clues on what went wrong.

Provide functions to clear and update last_cmd_status that check that we hold the rdtgroup_mutex.

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/intel_rdt.h          |  7 ++++
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 56 ++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

arch/x86/kernel/cpu/intel_rdt.h +7 -0

index ebaddaeef023..9d3148d88ec8 100644
@@ -126,12 +126,15 @@ struct rdtgroup {
 #define RFTYPE_BASE			BIT(1)
 #define RF_CTRLSHIFT			4
 #define RF_MONSHIFT			5
+#define RF_TOPSHIFT			6
 #define RFTYPE_CTRL			BIT(RF_CTRLSHIFT)
 #define RFTYPE_MON			BIT(RF_MONSHIFT)
+#define RFTYPE_TOP			BIT(RF_TOPSHIFT)
 #define RFTYPE_RES_CACHE		BIT(8)
 #define RFTYPE_RES_MB			BIT(9)
 #define RF_CTRL_INFO			(RFTYPE_INFO | RFTYPE_CTRL)
 #define RF_MON_INFO			(RFTYPE_INFO | RFTYPE_MON)
+#define RF_TOP_INFO			(RFTYPE_INFO | RFTYPE_TOP)
 #define RF_CTRL_BASE			(RFTYPE_BASE | RFTYPE_CTRL)
 
 /* List of all resource groups */

@@ -408,6 +411,10 @@ union cpuid_0x10_x_edx {
 	unsigned int full;
 };
 
+void rdt_last_cmd_clear(void);
+void rdt_last_cmd_puts(const char *s);
+void rdt_last_cmd_printf(const char *fmt, ...);
+
 void rdt_ctrl_update(void *arg);
 struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn);
 void rdtgroup_kn_unlock(struct kernfs_node *kn);

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +56 -0

98 lines not on this page · raw message

Signature · 1 line

2.11.0

Luck, Tony

author

2017-09-25 23:40 UTC #2 lore raw

PATCH v2 2/6 x86/intel_rdt: Add diagnostics when writing the schemata file

In reply to Luck, Tony

From: Tony Luck <[email protected]>

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

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c | 49 ++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 11 deletions(-)

arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c +38 ~~-11~~

index f6ea94f8954a..f29b4c21e7d4 100644
@@ -42,15 +42,22 @@ 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) {
+		rdt_last_cmd_puts("No support for non-linear MB domains\n");
 		return false;
+	}
 
 	ret = kstrtoul(buf, 10, &bw);
-	if (ret)
+	if (ret) {
+		rdt_last_cmd_printf("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) {
+		rdt_last_cmd_printf("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 +67,10 @@ 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) {
+		rdt_last_cmd_printf("duplicate domain %d\n", d->id);
 		return -EINVAL;
+	}
 
 	if (!bw_validate(buf, &data, r))
 		return -EINVAL;

@@ -84,20 +93,29 @@ static bool cbm_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 	int ret;
 
 	ret = kstrtoul(buf, 16, &val);
-	if (ret)
+	if (ret) {
+		rdt_last_cmd_printf("non-hex character in mask %s\n", buf);
 		return false;
+	}
 
-	if (val == 0 || val > r->default_ctrl)
+	if (val == 0 || val > r->default_ctrl) {
+		rdt_last_cmd_puts("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) {
+		rdt_last_cmd_printf("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) {
+		rdt_last_cmd_printf("Need at least %d bits in mask\n",
+				    r->cache.min_cbm_bits);
 		return false;
+	}
 
 	*data = val;
 	return true;

@@ -111,8 +129,10 @@ 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) {
+		rdt_last_cmd_printf("duplicate domain %d\n", d->id);
 		return -EINVAL;
+	}
 
 	if(!cbm_validate(buf, &data, r))
 		return -EINVAL;

@@ -139,8 +159,10 @@ 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)) {
+		rdt_last_cmd_puts("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 +218,7 @@ 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);
 	}
+	rdt_last_cmd_printf("unknown/unsupported resource name '%s'\n", resname);
 	return -EINVAL;
 }

@@ -209,8 +232,10 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 	int closid, ret = 0;
 
 	/* 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);

@@ -218,6 +243,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 		rdtgroup_kn_unlock(of->kn);
 		return -ENOENT;
 	}
+	rdt_last_cmd_clear();
 
 	closid = rdtgrp->closid;

@@ -229,6 +255,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 	while ((tok = strsep(&buf, "\n")) != NULL) {
 		resname = strim(strsep(&tok, ":"));
 		if (!tok) {
+			rdt_last_cmd_puts("Missing ':'\n");
 			ret = -EINVAL;
 			goto out;
 		}

Signature · 1 line

2.11.0

Luck, Tony

author

2017-09-25 23:39 UTC #3 lore raw

PATCH v2 3/6 x86/intel_rdt: Add diagnostics when writing the tasks file

In reply to Luck, Tony

From: Tony Luck <[email protected]>

About the only tricky case is trying to move a task into a monitor group that is a subdirectory of a different control group. But cover the simple cases too.

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c |  4 +---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c    | 15 +++++++++++----
 2 files changed, 12 insertions(+), 7 deletions(-)

arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c +1 -3

index f29b4c21e7d4..30aeb267cbd2 100644
@@ -232,10 +232,8 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 	int closid, ret = 0;
 
 	/* Valid input requires a trailing newline */
-	if (nbytes == 0 || buf[nbytes - 1] != '\n') {
-		seq_buf_puts(&last_cmd_status, "no trailing newline\n");
+	if (nbytes == 0 || buf[nbytes - 1] != '\n')
 		return -EINVAL;
-	}
 	buf[nbytes - 1] = '\0';
 
 	rdtgrp = rdtgroup_kn_lock_live(of->kn);

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +11 -4

index 9268122eb149..df6373680a59 100644
@@ -478,6 +478,7 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
 		 */
 		atomic_dec(&rdtgrp->waitcount);
 		kfree(callback);
+		rdt_last_cmd_puts("task exited\n");
 	} else {
 		/*
 		 * For ctrl_mon groups move both closid and rmid.

@@ -488,10 +489,12 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
 			tsk->closid = rdtgrp->closid;
 			tsk->rmid = rdtgrp->mon.rmid;
 		} else if (rdtgrp->type == RDTMON_GROUP) {
-			if (rdtgrp->mon.parent->closid == tsk->closid)
+			if (rdtgrp->mon.parent->closid == tsk->closid) {
 				tsk->rmid = rdtgrp->mon.rmid;
-			else
+			} else {
+				rdt_last_cmd_puts("Can't move task to different control group\n");
 				ret = -EINVAL;
+			}
 		}
 	}
 	return ret;

@@ -510,8 +513,10 @@ static int rdtgroup_task_write_permission(struct task_struct *task,
 	 */
 	if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
 	    !uid_eq(cred->euid, tcred->uid) &&
-	    !uid_eq(cred->euid, tcred->suid))
+	    !uid_eq(cred->euid, tcred->suid)) {
+		rdt_last_cmd_printf("No permission to move task %d\n", task->pid);
 		ret = -EPERM;
+	}
 
 	put_cred(tcred);
 	return ret;

@@ -528,6 +533,7 @@ static int rdtgroup_move_task(pid_t pid, struct rdtgroup *rdtgrp,
 		tsk = find_task_by_vpid(pid);
 		if (!tsk) {
 			rcu_read_unlock();
+			rdt_last_cmd_printf("No task %d\n", pid);
 			return -ESRCH;
 		}
 	} else {

@@ -555,6 +561,7 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
 	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
 		return -EINVAL;
 	rdtgrp = rdtgroup_kn_lock_live(of->kn);
+	rdt_last_cmd_clear();
 
 	if (rdtgrp)
 		ret = rdtgroup_move_task(pid, rdtgrp, of);

@@ -1208,7 +1215,7 @@ static struct dentry *rdt_mount(struct file_system_type *fs_type,
 out_cdp:
 	cdp_disable();
 out:
-	seq_buf_clear(&last_cmd_status);
+	rdt_last_cmd_clear();
 	mutex_unlock(&rdtgroup_mutex);
 
 	return dentry;

Signature · 1 line

2.11.0

Luck, Tony

author

2017-09-25 23:39 UTC #4 lore raw

PATCH v2 4/6 x86/intel_rdt: Add diagnostics when writing the cpus file

In reply to Luck, Tony

From: Tony Luck <[email protected]>

Can't add a cpu to a monitor group unless it belongs to parent group. Can't delete cpus from the default group.

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +12 -3

index df6373680a59..1674f7871b06 100644
@@ -264,8 +264,10 @@ static int cpus_mon_write(struct rdtgroup *rdtgrp, cpumask_var_t newmask,
 
 	/* Check whether cpus belong to parent ctrl group */
 	cpumask_andnot(tmpmask, newmask, &prgrp->cpu_mask);
-	if (cpumask_weight(tmpmask))
+	if (cpumask_weight(tmpmask)) {
+		rdt_last_cmd_puts("can only add CPUs to mongroup that belong to parent\n");
 		return -EINVAL;
+	}
 
 	/* Check whether cpus are dropped from this group */
 	cpumask_andnot(tmpmask, &rdtgrp->cpu_mask, newmask);

@@ -317,8 +319,10 @@ static int cpus_ctrl_write(struct rdtgroup *rdtgrp, cpumask_var_t newmask,
 	cpumask_andnot(tmpmask, &rdtgrp->cpu_mask, newmask);
 	if (cpumask_weight(tmpmask)) {
 		/* Can't drop from default group */
-		if (rdtgrp == &rdtgroup_default)
+		if (rdtgrp == &rdtgroup_default) {
+			rdt_last_cmd_puts("Can't drop CPUs from default group\n");
 			return -EINVAL;
+		}
 
 		/* Give any dropped cpus to rdtgroup_default */
 		cpumask_or(&rdtgroup_default.cpu_mask,

@@ -383,8 +387,10 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
 	}
 
 	rdtgrp = rdtgroup_kn_lock_live(of->kn);
+	rdt_last_cmd_clear();
 	if (!rdtgrp) {
 		ret = -ENOENT;
+		rdt_last_cmd_puts("directory was removed\n");
 		goto unlock;
 	}

@@ -393,13 +399,16 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
 	else
 		ret = cpumask_parse(buf, newmask);
 
-	if (ret)
+	if (ret) {
+		rdt_last_cmd_puts("bad cpu list/mask\n");
 		goto unlock;
+	}
 
 	/* check that user didn't specify any offline cpus */
 	cpumask_andnot(tmpmask, newmask, cpu_online_mask);
 	if (cpumask_weight(tmpmask)) {
 		ret = -EINVAL;
+		rdt_last_cmd_puts("can only assign online cpus\n");
 		goto unlock;
 	}

Signature · 1 line

2.11.0

Luck, Tony

author

2017-09-25 23:40 UTC #5 lore raw

PATCH v2 5/6 x86/intel_rdt: Add diagnostics when making directories

In reply to Luck, Tony

From: Tony Luck <[email protected]>

Mostly this is about running out of RMIDs or CLOSIDs. Other errors are various internal errors.

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +22 -6

index 1674f7871b06..c015cc1c2e8e 100644
@@ -1593,8 +1593,10 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 	int ret;
 
 	prdtgrp = rdtgroup_kn_lock_live(prgrp_kn);
+	rdt_last_cmd_clear();
 	if (!prdtgrp) {
 		ret = -ENODEV;
+		rdt_last_cmd_puts("directory was removed\n");
 		goto out_unlock;
 	}

@@ -1602,6 +1604,7 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 	rdtgrp = kzalloc(sizeof(*rdtgrp), GFP_KERNEL);
 	if (!rdtgrp) {
 		ret = -ENOSPC;
+		rdt_last_cmd_puts("kernel out of memory\n");
 		goto out_unlock;
 	}
 	*r = rdtgrp;

@@ -1613,6 +1616,7 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 	kn = kernfs_create_dir(parent_kn, name, mode, rdtgrp);
 	if (IS_ERR(kn)) {
 		ret = PTR_ERR(kn);
+		rdt_last_cmd_puts("kernfs create error\n");
 		goto out_free_rgrp;
 	}
 	rdtgrp->kn = kn;

@@ -1626,24 +1630,32 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 	kernfs_get(kn);
 
 	ret = rdtgroup_kn_set_ugid(kn);
-	if (ret)
+	if (ret) {
+		rdt_last_cmd_puts("kernfs perm error\n");
 		goto out_destroy;
+	}
 
 	files = RFTYPE_BASE | RFTYPE_CTRL;
 	files = RFTYPE_BASE | BIT(RF_CTRLSHIFT + rtype);
 	ret = rdtgroup_add_files(kn, files);
-	if (ret)
+	if (ret) {
+		rdt_last_cmd_puts("kernfs fill error\n");
 		goto out_destroy;
+	}
 
 	if (rdt_mon_capable) {
 		ret = alloc_rmid();
-		if (ret < 0)
+		if (ret < 0) {
+			rdt_last_cmd_puts("out of RMIDs\n");
 			goto out_destroy;
+		}
 		rdtgrp->mon.rmid = ret;
 
 		ret = mkdir_mondata_all(kn, rdtgrp, &rdtgrp->mon.mon_data_kn);
-		if (ret)
+		if (ret) {
+			rdt_last_cmd_puts("kernfs subdir error\n");
 			goto out_idfree;
+		}
 	}
 	kernfs_activate(kn);

@@ -1721,8 +1733,10 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
 
 	kn = rdtgrp->kn;
 	ret = closid_alloc();
-	if (ret < 0)
+	if (ret < 0) {
+		rdt_last_cmd_puts("out of CLOSIDs\n");
 		goto out_common_fail;
+	}
 	closid = ret;
 
 	rdtgrp->closid = closid;

@@ -1734,8 +1748,10 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
 		 * of tasks and cpus to monitor.
 		 */
 		ret = mongroup_create_dir(kn, NULL, "mon_groups", NULL);
-		if (ret)
+		if (ret) {
+			rdt_last_cmd_puts("kernfs subdir error\n");
 			goto out_id_free;
+		}
 	}
 
 	goto out_unlock;

Signature · 1 line

2.11.0

Luck, Tony

author

2017-09-25 23:40 UTC #6 lore raw

PATCH v2 6/6 x86/intel_rdt: Add documentation for "info/last_cmd_status"

In reply to Luck, Tony

From: Tony Luck <[email protected]>

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

Signed-off-by Tony Luck <[email protected]>

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

Documentation/x86/intel_rdt_ui.txt +11 -0

index 4d8848e4e224..6851854cf69d 100644
@@ -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
 ---------------------------------

Signature · 1 line

2.11.0