[LTP] [PATCH v2] kernel/irq: Add irqbalance01

[LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Richard Palethorpe]

Add first test specifically targeting interrupts and IRQ management.

This includes some comments inline because I think the parsing code is
unavoidably confusing.

Note on the CPU mask parsing; there is already some code for parsing
and manipulating bitmaps in the LTP. However it is absurdly
complicated and we don't need actual bitmaps. In fact an array of
bytes is more flexible.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---

V2:
* Read proc files in at most 1-page blocks

  As Cyril suggested this looks like the best way. seq_file will only
  expand the internal buffer above a page if a record does not fit in
  one page. In this case a record is the per-cpus counts for an
  irq. However it won't expand the buffer for multiple records. It
  just returns whatver fits in the buffer.

* Read in CPU masks from the end of the buffer

  If nr_cpus is not a multiple of 4 then one of the hex digits has
  some padding. We either need to pad irq_affinity as well (which
  creates complication) or ignore the padding. It's easier to ignore
  it if we start at the end.

* Print headers and some minor format changes to printing
* Add a few more parsing checks
* Remove freeing of buffers

 runtest/irq                         |   1 +
 testcases/kernel/irq/.gitignore     |   1 +
 testcases/kernel/irq/Makefile       |   7 +
 testcases/kernel/irq/irqbalance01.c | 316 ++++++++++++++++++++++++++++
 4 files changed, 325 insertions(+)
 create mode 100644 runtest/irq
 create mode 100644 testcases/kernel/irq/.gitignore
 create mode 100644 testcases/kernel/irq/Makefile
 create mode 100644 testcases/kernel/irq/irqbalance01.c

diff --git a/runtest/irq b/runtest/irq
new file mode 100644
index 000000000..56d0d23c8
--- /dev/null
+++ b/runtest/irq
@@ -0,0 +1 @@
+irqbalance01 irqbalance01
diff --git a/testcases/kernel/irq/.gitignore b/testcases/kernel/irq/.gitignore
new file mode 100644
index 000000000..8ed69a99c
--- /dev/null
+++ b/testcases/kernel/irq/.gitignore
@@ -0,0 +1 @@
+irqbalance01
diff --git a/testcases/kernel/irq/Makefile b/testcases/kernel/irq/Makefile
new file mode 100644
index 000000000..aa51da7cb
--- /dev/null
+++ b/testcases/kernel/irq/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+top_srcdir		?= ../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/irq/irqbalance01.c b/testcases/kernel/irq/irqbalance01.c
new file mode 100644
index 000000000..f32ab9495
--- /dev/null
+++ b/testcases/kernel/irq/irqbalance01.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com> */
+/*\
+ * [Description]
+ *
+ * Check that something (e.g. irqbalance daemon) is performing IRQ
+ * load balancing.
+ *
+ * On many systems userland needs to set /proc/irq/$IRQ/smp_affinity
+ * to prevent many IRQs being delivered to the same CPU.
+ *
+ * Note some drivers and IRQ controllers will distribute IRQs
+ * evenly. Some systems will have housekeeping CPUs configured. Some
+ * IRQs can not be masked etc. So this test is not appropriate for all
+ * scenarios.
+ *
+ * Furthermore, exactly how IRQs should be distributed is a
+ * performance and/or security issue. This is only a generic smoke
+ * test. It will hopefully detect misconfigured systems and total
+ * balancing failures which are often silent errors.
+ *
+ * Heuristic: Evidence of Change
+ *
+ * 1. Find IRQs with a non-zero count
+ * 2. Check if they are now disallowed
+ *
+ * There are two sources of information we need to parse:
+ *
+ * 1. /proc/interrupts
+ * 2. /proc/irq/$IRQ/smp_affinity
+ *
+ * We get the active IRQs and CPUs from /proc/interrupts. It also
+ * contains the per-CPU IRQ counts and info we do not care about.
+ *
+ * We get the IRQ masks from each active IRQ's smp_affinity file. This
+ * is a bitmask written out in hexadecimal format. It shows which CPUs
+ * an IRQ may be received by.
+ */
+
+#include <stdlib.h>
+
+#include "tst_test.h"
+#include "tst_safe_stdio.h"
+#include "tst_safe_file_at.h"
+
+enum affinity {
+	ALLOW = '+',
+	DENY = '-',
+};
+
+static unsigned int *irq_stats;
+static enum affinity *irq_affinity;
+
+static unsigned int nr_cpus;
+static unsigned int nr_irqs;
+static unsigned int *irq_ids;
+
+static char *read_proc_file(const char *const path)
+{
+	const size_t pg_len = SAFE_SYSCONF(_SC_PAGESIZE);
+	int fd = SAFE_OPEN(path, O_RDONLY);
+	size_t ret = 0, used_len = 0;
+	static size_t total_len;
+	static char *buf;
+
+	do {
+		if (used_len + 1 >= total_len) {
+			total_len += pg_len;
+			buf = SAFE_REALLOC(buf, total_len);
+		}
+
+		ret = SAFE_READ(0, fd,
+				buf + used_len,
+				total_len - used_len - 1);
+		used_len += ret;
+	} while (ret);
+
+	if (!used_len)
+		tst_brk(TBROK, "Empty %s?", path);
+
+	buf[used_len] = '\0';
+
+	SAFE_CLOSE(fd);
+
+	return buf;
+}
+
+static void collect_irq_info(void)
+{
+	char *buf, *c, *first_row;
+	char path[PATH_MAX];
+	size_t row, col;
+	long acc;
+	unsigned int cpu_total, bit;
+
+	nr_cpus = 0;
+	nr_irqs = 0;
+
+	buf = read_proc_file("/proc/interrupts");
+
+	/* Count CPUs, header columns are like /CPU[0-9]+/ */
+	for (c = buf; *c != '\0' && *c != '\n'; c++) {
+		if (!strncmp(c, "CPU", 3))
+			nr_cpus++;
+	}
+
+	c++;
+	first_row = c;
+	/* Count IRQs, real IRQs start with /[0-9]+:/ */
+	while (*c != '\0') {
+		switch (*c) {
+		case ' ':
+		case '\t':
+		case '\n':
+		case '0' ... '9':
+			c++;
+			break;
+		case ':':
+			nr_irqs++;
+			/* fall-through */
+		default:
+			while (*c != '\n' && *c != '\0')
+				c++;
+		}
+	}
+
+	tst_res(TINFO, "Found %u CPUS, %u IRQs", nr_cpus, nr_irqs);
+
+	irq_ids = SAFE_REALLOC(irq_ids, nr_irqs * sizeof(*irq_ids));
+	irq_stats = SAFE_REALLOC(irq_stats,
+				 nr_cpus * (nr_irqs + 1) * sizeof(*irq_stats));
+	irq_affinity = SAFE_REALLOC(irq_affinity,
+				    nr_cpus * nr_irqs * sizeof(*irq_affinity));
+
+	c = first_row;
+	acc = -1;
+	row = col = 0;
+	/* Parse columns containing IRQ counts and IRQ IDs into acc. Ignore
+	 * everything else.
+	 */
+	while (*c != '\0') {
+		switch (*c) {
+		case ' ':
+		case '\t':
+			if (acc >= 0) {
+				irq_stats[row * nr_cpus + col] = acc;
+				acc = -1;
+				col++;
+			}
+			break;
+		case '\n':
+			if (acc != -1)
+				tst_brk(TBROK, "Unexpected EOL");
+			col = 0;
+			row++;
+			break;
+		case '0' ... '9':
+			if (acc == -1)
+				acc = 0;
+
+			acc *= 10;
+			acc += *c - '0';
+			break;
+		case ':':
+			if (acc == -1 || col != 0)
+				tst_brk(TBROK, "Unexpected ':'");
+			irq_ids[row] = acc;
+			acc = -1;
+			break;
+		default:
+			acc = -1;
+			while (*c != '\n' && *c != '\0')
+				c++;
+			continue;
+		}
+
+		c++;
+	}
+
+	for (col = 0; col < nr_cpus; col++) {
+		cpu_total = 0;
+
+		for (row = 0; row < nr_irqs; row++)
+			cpu_total += irq_stats[row * nr_cpus + col];
+
+		irq_stats[row * nr_cpus + col] = cpu_total;
+	}
+
+	/* Read the CPU affinity masks for each IRQ. The first CPU is in the
+	 * right most (least significant) bit. See bitmap_string() in the kernel
+	 * (%*pb)
+	 */
+	for (row = 0; row < nr_irqs; row++) {
+		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
+		buf = read_proc_file(path);
+		c = buf;
+		col = 0;
+
+		while (*c != '\0')
+			c++;
+
+		while (--c >= buf) {
+			if (col > nr_cpus) {
+				tst_res(TINFO, "%u/smp_affnity: %s",
+					irq_ids[row], buf);
+				tst_brk(TBROK, "More mask char bits than cpus");
+			}
+
+			switch (*c) {
+			case '\n':
+			case ' ':
+			case ',':
+				continue;
+			case '0' ... '9':
+				acc = *c - '0';
+				break;
+			case 'a' ... 'f':
+				acc = 10 + *c - 'a';
+				break;
+			default:
+				tst_res(TINFO, "%u/smp_affnity: %s",
+					irq_ids[row], buf);
+				tst_brk(TBROK, "Wasn't expecting 0x%02x", *c);
+			}
+
+			for (bit = 0; bit < 4 && col < nr_cpus; bit++) {
+				irq_affinity[row * nr_cpus + col++] =
+					(acc & (1 << bit)) ? ALLOW : DENY;
+			}
+		}
+
+		if (col < nr_cpus) {
+			tst_res(TINFO, "%u/smp_affnity: %s", irq_ids[row], buf);
+			tst_brk(TBROK, "Only found %zu cpus", col);
+		}
+	}
+}
+
+static void print_irq_info(void)
+{
+	size_t row, col;
+	unsigned int count;
+	enum affinity aff;
+
+	tst_printf("  IRQ       ");
+	for (col = 0; col < nr_cpus; col++)
+		tst_printf("CPU%-8zu", col);
+
+	tst_printf("\n");
+
+	for (row = 0; row < nr_irqs; row++) {
+		tst_printf("%5u:", irq_ids[row]);
+
+		for (col = 0; col < nr_cpus; col++) {
+			count = irq_stats[row * nr_cpus + col];
+			aff = irq_affinity[row * nr_cpus + col];
+
+			tst_printf("%10u%c", count, aff);
+		}
+
+		tst_printf("\n");
+	}
+
+	tst_printf("Total:");
+
+	for (col = 0; col < nr_cpus; col++)
+		tst_printf("%10u ", irq_stats[row * nr_cpus + col]);
+
+	tst_printf("\n");
+}
+
+static void evidence_of_change(void)
+{
+	size_t row, col, changed = 0;
+
+	for (row = 0; row < nr_irqs; row++) {
+		for (col = 0; col < nr_cpus; col++) {
+			if (!irq_stats[row * nr_cpus + col])
+				continue;
+
+			if (irq_affinity[row * nr_cpus + col] == ALLOW)
+				continue;
+
+			changed++;
+		}
+	}
+
+	tst_res(changed ? TPASS : TFAIL,
+		"Heuristic: Detected %zu irq-cpu pairs have been dissallowed",
+		changed);
+}
+
+static void setup(void)
+{
+	collect_irq_info();
+	print_irq_info();
+
+	if (nr_cpus < 1)
+		tst_brk(TBROK, "No CPUs found in /proc/interrupts?");
+
+	if (nr_irqs < 1)
+		tst_brk(TBROK, "No IRQs found in /proc/interrupts?");
+}
+
+static void run(void)
+{
+	collect_irq_info();
+
+	evidence_of_change();
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.min_cpus = 2,
+};
-- 
2.31.1

[LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Richard Palethorpe via ltp]

Add first test specifically targeting interrupts and IRQ management.

This includes some comments inline because I think the parsing code is
unavoidably confusing.

Note on the CPU mask parsing; there is already some code for parsing
and manipulating bitmaps in the LTP. However it is absurdly
complicated and we don't need actual bitmaps. In fact an array of
bytes is more flexible.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---

V2:
* Read proc files in at most 1-page blocks

  As Cyril suggested this looks like the best way. seq_file will only
  expand the internal buffer above a page if a record does not fit in
  one page. In this case a record is the per-cpus counts for an
  irq. However it won't expand the buffer for multiple records. It
  just returns whatver fits in the buffer.

* Read in CPU masks from the end of the buffer

  If nr_cpus is not a multiple of 4 then one of the hex digits has
  some padding. We either need to pad irq_affinity as well (which
  creates complication) or ignore the padding. It's easier to ignore
  it if we start at the end.

* Print headers and some minor format changes to printing
* Add a few more parsing checks
* Remove freeing of buffers

 runtest/irq                         |   1 +
 testcases/kernel/irq/.gitignore     |   1 +
 testcases/kernel/irq/Makefile       |   7 +
 testcases/kernel/irq/irqbalance01.c | 316 ++++++++++++++++++++++++++++
 4 files changed, 325 insertions(+)
 create mode 100644 runtest/irq
 create mode 100644 testcases/kernel/irq/.gitignore
 create mode 100644 testcases/kernel/irq/Makefile
 create mode 100644 testcases/kernel/irq/irqbalance01.c

diff --git a/runtest/irq b/runtest/irq
new file mode 100644
index 000000000..56d0d23c8
--- /dev/null
+++ b/runtest/irq
@@ -0,0 +1 @@
+irqbalance01 irqbalance01
diff --git a/testcases/kernel/irq/.gitignore b/testcases/kernel/irq/.gitignore
new file mode 100644
index 000000000..8ed69a99c
--- /dev/null
+++ b/testcases/kernel/irq/.gitignore
@@ -0,0 +1 @@
+irqbalance01
diff --git a/testcases/kernel/irq/Makefile b/testcases/kernel/irq/Makefile
new file mode 100644
index 000000000..aa51da7cb
--- /dev/null
+++ b/testcases/kernel/irq/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+top_srcdir		?= ../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/irq/irqbalance01.c b/testcases/kernel/irq/irqbalance01.c
new file mode 100644
index 000000000..f32ab9495
--- /dev/null
+++ b/testcases/kernel/irq/irqbalance01.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com> */
+/*\
+ * [Description]
+ *
+ * Check that something (e.g. irqbalance daemon) is performing IRQ
+ * load balancing.
+ *
+ * On many systems userland needs to set /proc/irq/$IRQ/smp_affinity
+ * to prevent many IRQs being delivered to the same CPU.
+ *
+ * Note some drivers and IRQ controllers will distribute IRQs
+ * evenly. Some systems will have housekeeping CPUs configured. Some
+ * IRQs can not be masked etc. So this test is not appropriate for all
+ * scenarios.
+ *
+ * Furthermore, exactly how IRQs should be distributed is a
+ * performance and/or security issue. This is only a generic smoke
+ * test. It will hopefully detect misconfigured systems and total
+ * balancing failures which are often silent errors.
+ *
+ * Heuristic: Evidence of Change
+ *
+ * 1. Find IRQs with a non-zero count
+ * 2. Check if they are now disallowed
+ *
+ * There are two sources of information we need to parse:
+ *
+ * 1. /proc/interrupts
+ * 2. /proc/irq/$IRQ/smp_affinity
+ *
+ * We get the active IRQs and CPUs from /proc/interrupts. It also
+ * contains the per-CPU IRQ counts and info we do not care about.
+ *
+ * We get the IRQ masks from each active IRQ's smp_affinity file. This
+ * is a bitmask written out in hexadecimal format. It shows which CPUs
+ * an IRQ may be received by.
+ */
+
+#include <stdlib.h>
+
+#include "tst_test.h"
+#include "tst_safe_stdio.h"
+#include "tst_safe_file_at.h"
+
+enum affinity {
+	ALLOW = '+',
+	DENY = '-',
+};
+
+static unsigned int *irq_stats;
+static enum affinity *irq_affinity;
+
+static unsigned int nr_cpus;
+static unsigned int nr_irqs;
+static unsigned int *irq_ids;
+
+static char *read_proc_file(const char *const path)
+{
+	const size_t pg_len = SAFE_SYSCONF(_SC_PAGESIZE);
+	int fd = SAFE_OPEN(path, O_RDONLY);
+	size_t ret = 0, used_len = 0;
+	static size_t total_len;
+	static char *buf;
+
+	do {
+		if (used_len + 1 >= total_len) {
+			total_len += pg_len;
+			buf = SAFE_REALLOC(buf, total_len);
+		}
+
+		ret = SAFE_READ(0, fd,
+				buf + used_len,
+				total_len - used_len - 1);
+		used_len += ret;
+	} while (ret);
+
+	if (!used_len)
+		tst_brk(TBROK, "Empty %s?", path);
+
+	buf[used_len] = '\0';
+
+	SAFE_CLOSE(fd);
+
+	return buf;
+}
+
+static void collect_irq_info(void)
+{
+	char *buf, *c, *first_row;
+	char path[PATH_MAX];
+	size_t row, col;
+	long acc;
+	unsigned int cpu_total, bit;
+
+	nr_cpus = 0;
+	nr_irqs = 0;
+
+	buf = read_proc_file("/proc/interrupts");
+
+	/* Count CPUs, header columns are like /CPU[0-9]+/ */
+	for (c = buf; *c != '\0' && *c != '\n'; c++) {
+		if (!strncmp(c, "CPU", 3))
+			nr_cpus++;
+	}
+
+	c++;
+	first_row = c;
+	/* Count IRQs, real IRQs start with /[0-9]+:/ */
+	while (*c != '\0') {
+		switch (*c) {
+		case ' ':
+		case '\t':
+		case '\n':
+		case '0' ... '9':
+			c++;
+			break;
+		case ':':
+			nr_irqs++;
+			/* fall-through */
+		default:
+			while (*c != '\n' && *c != '\0')
+				c++;
+		}
+	}
+
+	tst_res(TINFO, "Found %u CPUS, %u IRQs", nr_cpus, nr_irqs);
+
+	irq_ids = SAFE_REALLOC(irq_ids, nr_irqs * sizeof(*irq_ids));
+	irq_stats = SAFE_REALLOC(irq_stats,
+				 nr_cpus * (nr_irqs + 1) * sizeof(*irq_stats));
+	irq_affinity = SAFE_REALLOC(irq_affinity,
+				    nr_cpus * nr_irqs * sizeof(*irq_affinity));
+
+	c = first_row;
+	acc = -1;
+	row = col = 0;
+	/* Parse columns containing IRQ counts and IRQ IDs into acc. Ignore
+	 * everything else.
+	 */
+	while (*c != '\0') {
+		switch (*c) {
+		case ' ':
+		case '\t':
+			if (acc >= 0) {
+				irq_stats[row * nr_cpus + col] = acc;
+				acc = -1;
+				col++;
+			}
+			break;
+		case '\n':
+			if (acc != -1)
+				tst_brk(TBROK, "Unexpected EOL");
+			col = 0;
+			row++;
+			break;
+		case '0' ... '9':
+			if (acc == -1)
+				acc = 0;
+
+			acc *= 10;
+			acc += *c - '0';
+			break;
+		case ':':
+			if (acc == -1 || col != 0)
+				tst_brk(TBROK, "Unexpected ':'");
+			irq_ids[row] = acc;
+			acc = -1;
+			break;
+		default:
+			acc = -1;
+			while (*c != '\n' && *c != '\0')
+				c++;
+			continue;
+		}
+
+		c++;
+	}
+
+	for (col = 0; col < nr_cpus; col++) {
+		cpu_total = 0;
+
+		for (row = 0; row < nr_irqs; row++)
+			cpu_total += irq_stats[row * nr_cpus + col];
+
+		irq_stats[row * nr_cpus + col] = cpu_total;
+	}
+
+	/* Read the CPU affinity masks for each IRQ. The first CPU is in the
+	 * right most (least significant) bit. See bitmap_string() in the kernel
+	 * (%*pb)
+	 */
+	for (row = 0; row < nr_irqs; row++) {
+		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
+		buf = read_proc_file(path);
+		c = buf;
+		col = 0;
+
+		while (*c != '\0')
+			c++;
+
+		while (--c >= buf) {
+			if (col > nr_cpus) {
+				tst_res(TINFO, "%u/smp_affnity: %s",
+					irq_ids[row], buf);
+				tst_brk(TBROK, "More mask char bits than cpus");
+			}
+
+			switch (*c) {
+			case '\n':
+			case ' ':
+			case ',':
+				continue;
+			case '0' ... '9':
+				acc = *c - '0';
+				break;
+			case 'a' ... 'f':
+				acc = 10 + *c - 'a';
+				break;
+			default:
+				tst_res(TINFO, "%u/smp_affnity: %s",
+					irq_ids[row], buf);
+				tst_brk(TBROK, "Wasn't expecting 0x%02x", *c);
+			}
+
+			for (bit = 0; bit < 4 && col < nr_cpus; bit++) {
+				irq_affinity[row * nr_cpus + col++] =
+					(acc & (1 << bit)) ? ALLOW : DENY;
+			}
+		}
+
+		if (col < nr_cpus) {
+			tst_res(TINFO, "%u/smp_affnity: %s", irq_ids[row], buf);
+			tst_brk(TBROK, "Only found %zu cpus", col);
+		}
+	}
+}
+
+static void print_irq_info(void)
+{
+	size_t row, col;
+	unsigned int count;
+	enum affinity aff;
+
+	tst_printf("  IRQ       ");
+	for (col = 0; col < nr_cpus; col++)
+		tst_printf("CPU%-8zu", col);
+
+	tst_printf("\n");
+
+	for (row = 0; row < nr_irqs; row++) {
+		tst_printf("%5u:", irq_ids[row]);
+
+		for (col = 0; col < nr_cpus; col++) {
+			count = irq_stats[row * nr_cpus + col];
+			aff = irq_affinity[row * nr_cpus + col];
+
+			tst_printf("%10u%c", count, aff);
+		}
+
+		tst_printf("\n");
+	}
+
+	tst_printf("Total:");
+
+	for (col = 0; col < nr_cpus; col++)
+		tst_printf("%10u ", irq_stats[row * nr_cpus + col]);
+
+	tst_printf("\n");
+}
+
+static void evidence_of_change(void)
+{
+	size_t row, col, changed = 0;
+
+	for (row = 0; row < nr_irqs; row++) {
+		for (col = 0; col < nr_cpus; col++) {
+			if (!irq_stats[row * nr_cpus + col])
+				continue;
+
+			if (irq_affinity[row * nr_cpus + col] == ALLOW)
+				continue;
+
+			changed++;
+		}
+	}
+
+	tst_res(changed ? TPASS : TFAIL,
+		"Heuristic: Detected %zu irq-cpu pairs have been dissallowed",
+		changed);
+}
+
+static void setup(void)
+{
+	collect_irq_info();
+	print_irq_info();
+
+	if (nr_cpus < 1)
+		tst_brk(TBROK, "No CPUs found in /proc/interrupts?");
+
+	if (nr_irqs < 1)
+		tst_brk(TBROK, "No IRQs found in /proc/interrupts?");
+}
+
+static void run(void)
+{
+	collect_irq_info();
+
+	evidence_of_change();
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.min_cpus = 2,
+};
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Petr Vorel]

Hi Richie,

Changes to this version (it's actually v3) LGTM.
I suppose Cyril wants to recheck it, thus not merging it yet.

Thanks!

Kind regards,
Petr

> +++ b/testcases/kernel/irq/.gitignore
> @@ -0,0 +1 @@
> +irqbalance01
Very nit: we usually put '/' at the beginning to: /irqbalance01
(to apply files only in testcases/kernel/irq/, but that's irrelevant in most
cases as there are no subdirectories). If we even bother, it can be changed during merge.

Re: [LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Petr Vorel]

Hi Richie,

Changes to this version (it's actually v3) LGTM.
I suppose Cyril wants to recheck it, thus not merging it yet.

Thanks!

Kind regards,
Petr

> +++ b/testcases/kernel/irq/.gitignore
> @@ -0,0 +1 @@
> +irqbalance01
Very nit: we usually put '/' at the beginning to: /irqbalance01
(to apply files only in testcases/kernel/irq/, but that's irrelevant in most
cases as there are no subdirectories). If we even bother, it can be changed during merge.


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Cyril Hrubis]

Hi!
> +	/* Read the CPU affinity masks for each IRQ. The first CPU is in the
> +	 * right most (least significant) bit. See bitmap_string() in the kernel
> +	 * (%*pb)
> +	 */
> +	for (row = 0; row < nr_irqs; row++) {
> +		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
> +		buf = read_proc_file(path);
> +		c = buf;
> +		col = 0;
> +
> +		while (*c != '\0')
> +			c++;

A minor nit, we alredy know the lenght, but we do not return it from the
read_proc_file() we may as well change this so taht the read_proc_file
returns both buffer pointer and the size. E.g. add size_t *len parameter
to the read_proc_file() and set it if it's not NULL.

Other than that the parsing looks good now.

> +static void evidence_of_change(void)
> +{
> +	size_t row, col, changed = 0;
> +
> +	for (row = 0; row < nr_irqs; row++) {
> +		for (col = 0; col < nr_cpus; col++) {
> +			if (!irq_stats[row * nr_cpus + col])
> +				continue;
> +
> +			if (irq_affinity[row * nr_cpus + col] == ALLOW)
> +				continue;
> +
> +			changed++;
> +		}
> +	}
> +
> +	tst_res(changed ? TPASS : TFAIL,
> +		"Heuristic: Detected %zu irq-cpu pairs have been dissallowed",
> +		changed);
> +}

I'm still unusure about this part though.

The test fails on my workstation where the IRQs are nicely distributed
by the hardware/BIOS.

Maybe we should check if IRQs are distributed somehow evenly and PASS
the test if that is the case as well.

In my case there is 27 IRQs, 18 of them have non-zero counters and there
is 12 CPUs. For active IRQs that gives 18/12 = 1.5 IRQ per CPU and there
are 7 CPUs that handle 2 IRQs, 4 CPUs that handle 1 IRQ and 1 CPU that
doesn't handle anything. I wonder if we can figure out some kind of
heuristic, but I guess that in the end it wouldn't be worth the effort.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Richard Palethorpe]

Hello Cyril,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> +	/* Read the CPU affinity masks for each IRQ. The first CPU is in the
>> +	 * right most (least significant) bit. See bitmap_string() in the kernel
>> +	 * (%*pb)
>> +	 */
>> +	for (row = 0; row < nr_irqs; row++) {
>> +		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
>> +		buf = read_proc_file(path);
>> +		c = buf;
>> +		col = 0;
>> +
>> +		while (*c != '\0')
>> +			c++;
>
> A minor nit, we alredy know the lenght, but we do not return it from the
> read_proc_file() we may as well change this so taht the read_proc_file
> returns both buffer pointer and the size. E.g. add size_t *len parameter
> to the read_proc_file() and set it if it's not NULL.

+1

>
> Other than that the parsing looks good now.
>
>> +static void evidence_of_change(void)
>> +{
>> +	size_t row, col, changed = 0;
>> +
>> +	for (row = 0; row < nr_irqs; row++) {
>> +		for (col = 0; col < nr_cpus; col++) {
>> +			if (!irq_stats[row * nr_cpus + col])
>> +				continue;
>> +
>> +			if (irq_affinity[row * nr_cpus + col] == ALLOW)
>> +				continue;
>> +
>> +			changed++;
>> +		}
>> +	}
>> +
>> +	tst_res(changed ? TPASS : TFAIL,
>> +		"Heuristic: Detected %zu irq-cpu pairs have been dissallowed",
>> +		changed);
>> +}
>
> I'm still unusure about this part though.
>
> The test fails on my workstation where the IRQs are nicely distributed
> by the hardware/BIOS.

You don't need irqbalance then :-p

The original bug this is intened to avoid was caused by the irqbalance
deamon silently failing.

>
> Maybe we should check if IRQs are distributed somehow evenly and PASS
> the test if that is the case as well.

My plan was to have a second heuristic based on the IRQ
distribution. However the intention was to make detection more sensitve
to bad setups. I think what you are suggesting is the opposite. So that
users can blindly run the test and let it figure out if it is
appropriate for the hardware.

>
> In my case there is 27 IRQs, 18 of them have non-zero counters and there
> is 12 CPUs. For active IRQs that gives 18/12 = 1.5 IRQ per CPU and there
> are 7 CPUs that handle 2 IRQs, 4 CPUs that handle 1 IRQ and 1 CPU that
> doesn't handle anything. I wonder if we can figure out some kind of
> heuristic, but I guess that in the end it wouldn't be worth the
> effort.

I general I think it is best to write tests which figure out if they are
in the right environment. This makes all our lives easier. The problem
with this test in particular is the number of assumptions we have to
make about what a good IRQ distirbution looks like.

IRQs are (usually) evenly distributed for "performance". However not all
IRQs are equal, you might get better performance through an uneven
distribution. Especially if you only care about the performance some
tasks and not others. Also there are some reasons to isolate some CPUs
from IRQs regardless of performance.

Having said that we can probably find a heuristic for what a very bad
setup looks like i.e. (almost) all NIC IRQs on one CPU when there are
30+ CPUs and multiple NICs. That is one CPU is totally saturated with
IRQs from different NICs. This is a bit speculative so I'd like some
feedback from testing/community before putting effort into that.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH] kernel/irq: Add irqbalance01

[Richard Palethorpe]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richie,
>
> LGTM, only few nits found bellow (easily fixed before merge)

Thanks, but I also changed the printf to tst_printf which should be
added in the bpf_prog05 patch set. So I can post V2 after that is
merged.

>
> ...
>> +++ b/testcases/kernel/irq/.gitignore
>> @@ -0,0 +1 @@
>> +irqbalance01
> /irqbalance01
>
>> diff --git a/testcases/kernel/irq/Makefile b/testcases/kernel/irq/Makefile
>> new file mode 100644
>> index 000000000..085e06fac
>> --- /dev/null
>> +++ b/testcases/kernel/irq/Makefile
>> @@ -0,0 +1,9 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +top_srcdir		?= ../../..
>> +
>> +include $(top_srcdir)/include/mk/testcases.mk
>> +
>> +INSTALL_TARGETS		:= *.sh
> This should be removed (probably copy paste error
>> +
>> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
>> diff --git a/testcases/kernel/irq/irqbalance01.c b/testcases/kernel/irq/irqbalance01.c
>> new file mode 100644
>> index 000000000..0a476839c
>> --- /dev/null
>> +++ b/testcases/kernel/irq/irqbalance01.c
>> @@ -0,0 +1,288 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/* Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com> */
>> +/*\
>> + * [Description]
>> + *
>> + * Check that something (e.g. irqbalance daemon) is performing IRQ
>> + * load balancing.
>> + *
>> + * On most systems userland needs to set /proc/irq/$IRQ/smp_affinity
>> + * to prevent many IRQs being delivered to the same CPU.
>> + *
>> + * Note some drivers and IRQ controllers will distribute IRQs
>> + * evenly. Some systems will have housekeeping CPUs configured. Some
>> + * IRQs can not be masked etc. So this test is not appropriate for all
>> + * scenarios.
>> + *
>> + * Furthermore, exactly how IRQs should be distributed is a
>> + * performance and/or security issue. This is only a generic smoke
>> + * test. It will hopefully detect misconfigured systems and total
>> + * balancing failures which are often silent errors.
>> + *
>> + * Heuristic: Evidence of Change
> Add blank new line here to get better docparse formatting:
>
>> + * 1. Find IRQs with a non-zero count
>> + * 2. Check if they are now disallowed
> LGTM. It'd be interesting to hear opinion of IRQ subsystem maintainer
> Thomas Gleixner.

I guess I can copy them in on V2.

-- 
Thank you,
Richard.

[LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Richard Palethorpe]

Add first test specifically targeting interrupts and IRQ management.

This includes some comments inline because I think the parsing code is
unavoidably confusing.

Note on the CPU mask parsing; there is already some code for parsing
and manipulating bitmaps in the LTP. However it is absurdly
complicated and we don't need actual bitmaps. In fact an array of
bytes is more flexible.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---

To be clear this is a Linux Test Project test.

CC'ing IRQ subsystem and irqbalance maintainers in case they are
interested.

V2:
* Use tst_printf
* Fix typos
* rm .sh from Makefile

 runtest/irq                         |   1 +
 testcases/kernel/irq/.gitignore     |   1 +
 testcases/kernel/irq/Makefile       |   7 +
 testcases/kernel/irq/irqbalance01.c | 290 ++++++++++++++++++++++++++++
 4 files changed, 299 insertions(+)
 create mode 100644 runtest/irq
 create mode 100644 testcases/kernel/irq/.gitignore
 create mode 100644 testcases/kernel/irq/Makefile
 create mode 100644 testcases/kernel/irq/irqbalance01.c

diff --git a/runtest/irq b/runtest/irq
new file mode 100644
index 000000000..56d0d23c8
--- /dev/null
+++ b/runtest/irq
@@ -0,0 +1 @@
+irqbalance01 irqbalance01
diff --git a/testcases/kernel/irq/.gitignore b/testcases/kernel/irq/.gitignore
new file mode 100644
index 000000000..8ed69a99c
--- /dev/null
+++ b/testcases/kernel/irq/.gitignore
@@ -0,0 +1 @@
+irqbalance01
diff --git a/testcases/kernel/irq/Makefile b/testcases/kernel/irq/Makefile
new file mode 100644
index 000000000..aa51da7cb
--- /dev/null
+++ b/testcases/kernel/irq/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+top_srcdir		?= ../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/irq/irqbalance01.c b/testcases/kernel/irq/irqbalance01.c
new file mode 100644
index 000000000..56f3aad1d
--- /dev/null
+++ b/testcases/kernel/irq/irqbalance01.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com> */
+/*\
+ * [Description]
+ *
+ * Check that something (e.g. irqbalance daemon) is performing IRQ
+ * load balancing.
+ *
+ * On many systems userland needs to set /proc/irq/$IRQ/smp_affinity
+ * to prevent many IRQs being delivered to the same CPU.
+ *
+ * Note some drivers and IRQ controllers will distribute IRQs
+ * evenly. Some systems will have housekeeping CPUs configured. Some
+ * IRQs can not be masked etc. So this test is not appropriate for all
+ * scenarios.
+ *
+ * Furthermore, exactly how IRQs should be distributed is a
+ * performance and/or security issue. This is only a generic smoke
+ * test. It will hopefully detect misconfigured systems and total
+ * balancing failures which are often silent errors.
+ *
+ * Heuristic: Evidence of Change
+ *
+ * 1. Find IRQs with a non-zero count
+ * 2. Check if they are now disallowed
+ *
+ * There are two sources of information we need to parse:
+ *
+ * 1. /proc/interrupts
+ * 2. /proc/irq/$IRQ/smp_affinity
+ *
+ * We get the active IRQs and CPUs from /proc/interrupts. It also
+ * contains the per-CPU IRQ counts and info we do not care about.
+ *
+ * We get the IRQ masks from each active IRQ's smp_affinity file. This
+ * is a bitmask written out in hexadecimal format. It shows which CPUs
+ * an IRQ may be received by.
+ */
+
+#include <stdlib.h>
+
+#include "tst_test.h"
+#include "tst_safe_stdio.h"
+#include "tst_safe_file_at.h"
+
+enum affinity {
+	ALLOW,
+	DENY,
+};
+
+static unsigned int *irq_stats;
+static enum affinity *irq_affinity;
+
+static unsigned int nr_cpus;
+static unsigned int nr_irqs;
+static unsigned int *irq_ids;
+
+static void collect_irq_info(void)
+{
+	char *buf = NULL, *c, *first_row;
+	char path[PATH_MAX];
+	size_t size = 1024;
+	size_t ret, row, col;
+	long acc;
+	unsigned int cpu_total, mask_pos;
+	int fd = SAFE_OPEN("/proc/interrupts", O_RDONLY);
+
+	nr_cpus = 0;
+	nr_irqs = 0;
+
+	do {
+		size *= 2;
+		buf = SAFE_REALLOC(buf, size);
+		SAFE_LSEEK(fd, 0, SEEK_SET);
+		ret = SAFE_READ(0, fd, buf, size - 1);
+	} while (ret >= size - 1);
+
+	SAFE_CLOSE(fd);
+
+	if (ret < 1)
+		tst_brk(TBROK, "Empty /proc/interrupts?");
+
+	buf[ret] = '\0';
+
+	/* Count CPUs, header columns are like /CPU[0-9]+/ */
+	for (c = buf; *c != '\0' && *c != '\n'; c++) {
+		if (!strncmp(c, "CPU", 3))
+			nr_cpus++;
+	}
+
+	c++;
+	first_row = c;
+	/* Count IRQs, real IRQs start with /[0-9]+:/ */
+	while (*c != '\0') {
+		switch (*c) {
+		case ' ':
+		case '\t':
+		case '\n':
+		case '0' ... '9':
+			c++;
+			break;
+		case ':':
+			nr_irqs++;
+			/* fall-through */
+		default:
+			while (*c != '\n' && *c != '\0')
+				c++;
+		}
+	}
+
+	tst_res(TINFO, "Found %u CPUS, %u IRQs", nr_cpus, nr_irqs);
+
+	irq_ids = SAFE_REALLOC(irq_ids, nr_irqs * sizeof(*irq_ids));
+	irq_stats = SAFE_REALLOC(irq_stats, nr_cpus * (nr_irqs + 1) * sizeof(*irq_stats));
+	irq_affinity = SAFE_REALLOC(irq_affinity, nr_cpus * nr_irqs * sizeof(*irq_affinity));
+
+	c = first_row;
+	acc = -1;
+	row = col = 0;
+	/* Parse columns containing IRQ counts and IRQ IDs into acc. Ignore everything else. */
+	while (*c != '\0') {
+		switch (*c) {
+		case ' ':
+		case '\t':
+			if (acc >= 0) {
+				irq_stats[row * nr_cpus + col] = acc;
+				acc = -1;
+				col++;
+			}
+			break;
+		case '\n':
+			col = 0;
+			row++;
+			break;
+		case '0' ... '9':
+			if (acc == -1)
+				acc = 0;
+
+			acc *= 10;
+			acc += *c - '0';
+			break;
+		case ':':
+			irq_ids[row] = acc;
+			acc = -1;
+			break;
+		default:
+			acc = -1;
+			while (*c != '\n' && *c != '\0')
+				c++;
+			continue;
+		}
+
+		c++;
+	}
+
+	for (col = 0; col < nr_cpus; col++) {
+		cpu_total = 0;
+
+		for (row = 0; row < nr_irqs; row++)
+			cpu_total += irq_stats[row * nr_cpus + col];
+
+		irq_stats[row * nr_cpus + col] = cpu_total;
+	}
+
+	/* Read the CPU affinity masks for each IRQ. See bitmap_string() in the kernel (%*pb) */
+	for (row = 0; row < nr_irqs; row++) {
+		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
+		ret = SAFE_FILE_READAT(0, path, buf, size);
+		if (ret < 1)
+			tst_brk(TBROK, "Empty /proc/irq/%u/smp_affinity?", irq_ids[row]);
+		c = buf;
+		col = 0;
+
+		while (*c != '\0') {
+			switch (*c) {
+			case '\n':
+			case ' ':
+			case ',':
+				c++;
+				continue;
+			case '0' ... '9':
+				acc = *c - '0';
+				break;
+			case 'a' ... 'f':
+				acc = 10 + *c - 'a';
+				break;
+			default:
+				tst_res(TINFO, "%u/smp_affnity: %s", irq_ids[row], buf);
+				tst_brk(TBROK, "Wasn't expecting 0x%02x", *c);
+			}
+
+			for (mask_pos = 0; mask_pos < 4; mask_pos++) {
+				if (mask_pos + col >= nr_cpus)
+					break;
+
+				irq_affinity[row * nr_cpus + (nr_cpus - 1 - col - mask_pos)] =
+					(acc & (8 >> mask_pos)) ? ALLOW : DENY;
+			}
+
+			col += mask_pos;
+			c++;
+		}
+	}
+
+	free(buf);
+}
+
+static void print_irq_info(void)
+{
+	size_t row, col;
+	unsigned int count;
+	enum affinity aff;
+
+	for (row = 0; row < nr_irqs; row++) {
+		tst_printf("%5u:", irq_ids[row]);
+
+		for (col = 0; col < nr_cpus; col++) {
+			count = irq_stats[row * nr_cpus + col];
+			aff = irq_affinity[row * nr_cpus + col] == ALLOW ? '+' : '-';
+
+			tst_printf("%10u%c", count, aff);
+		}
+
+		tst_printf("\n");
+	}
+
+	tst_printf("Total:");
+
+	for (col = 0; col < nr_cpus; col++)
+		tst_printf("%11u", irq_stats[row * nr_cpus + col]);
+
+	tst_printf("\n");
+}
+
+static void evidence_of_change(void)
+{
+	size_t row, col, changed = 0;
+
+	for (row = 0; row < nr_irqs; row++) {
+		for (col = 0; col < nr_cpus; col++) {
+			if (!irq_stats[row * nr_cpus + col])
+				continue;
+
+			if (irq_affinity[row * nr_cpus + col] == ALLOW)
+				continue;
+
+			changed++;
+		}
+	}
+
+	tst_res(changed ? TPASS : TFAIL,
+		"Heuristic: Detected %zu irq-cpu pairs have been dissallowed",
+		changed);
+}
+
+static void setup(void)
+{
+	collect_irq_info();
+	print_irq_info();
+
+	if (nr_cpus < 1)
+		tst_brk(TBROK, "No CPUs found in /proc/interrupts?");
+
+	if (nr_irqs < 1)
+		tst_brk(TBROK, "No IRQs found in /proc/interrupts?");
+}
+
+static void run(void)
+{
+	collect_irq_info();
+
+	evidence_of_change();
+}
+
+static void cleanup(void)
+{
+	if (irq_ids)
+		free(irq_ids);
+	if (irq_stats)
+		free(irq_stats);
+	if (irq_affinity)
+		free(irq_affinity);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.cleanup = cleanup,
+	.min_cpus = 2,
+};
-- 
2.31.1

[LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Petr Vorel]

Hi Richie,

> V2:
> * Use tst_printf
> * Fix typos
> * rm .sh from Makefile
Thanks for a quick update.

> diff --git a/testcases/kernel/irq/irqbalance01.c b/testcases/kernel/irq/irqbalance01.c
...
> +	tst_res(changed ? TPASS : TFAIL,
> +		"Heuristic: Detected %zu irq-cpu pairs have been dissallowed",
                                                         ^ disallowed
(will be fixed during merge)


Kind regards,
Petr

[LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Cyril Hrubis]

Hi!
> +	/* Read the CPU affinity masks for each IRQ. See bitmap_string() in the kernel (%*pb) */
> +	for (row = 0; row < nr_irqs; row++) {
> +		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
> +		ret = SAFE_FILE_READAT(0, path, buf, size);
> +		if (ret < 1)
> +			tst_brk(TBROK, "Empty /proc/irq/%u/smp_affinity?", irq_ids[row]);
> +		c = buf;
> +		col = 0;
> +
> +		while (*c != '\0') {
> +			switch (*c) {
> +			case '\n':
> +			case ' ':
> +			case ',':
> +				c++;
> +				continue;
> +			case '0' ... '9':
> +				acc = *c - '0';
> +				break;
> +			case 'a' ... 'f':
> +				acc = 10 + *c - 'a';
> +				break;
> +			default:
> +				tst_res(TINFO, "%u/smp_affnity: %s", irq_ids[row], buf);
> +				tst_brk(TBROK, "Wasn't expecting 0x%02x", *c);
> +			}
> +
> +			for (mask_pos = 0; mask_pos < 4; mask_pos++) {
> +				if (mask_pos + col >= nr_cpus)
> +					break;
> +
> +				irq_affinity[row * nr_cpus + (nr_cpus - 1 - col - mask_pos)] =
> +					(acc & (8 >> mask_pos)) ? ALLOW : DENY;
> +			}

The mask here is actually wrong, since we go backward in the
irq_affinity array the bitmask has to grow not shrink so this should be
(acc & (1<<mask_pos))

> +			col += mask_pos;
> +			c++;
> +		}
> +	}
> +
> +	free(buf);
> +}
> +
> +static void print_irq_info(void)
> +{
> +	size_t row, col;
> +	unsigned int count;
> +	enum affinity aff;

It would be nice if we printed the CPUX header here as well so that it's
clear which column describes which CPU.

> +	for (row = 0; row < nr_irqs; row++) {
> +		tst_printf("%5u:", irq_ids[row]);
> +
> +		for (col = 0; col < nr_cpus; col++) {
> +			count = irq_stats[row * nr_cpus + col];
> +			aff = irq_affinity[row * nr_cpus + col] == ALLOW ? '+' : '-';
> +
> +			tst_printf("%10u%c", count, aff);
> +		}
> +
> +		tst_printf("\n");
> +	}
> +
> +	tst_printf("Total:");
> +
> +	for (col = 0; col < nr_cpus; col++)
> +		tst_printf("%11u", irq_stats[row * nr_cpus + col]);
                               ^
			       And I would change this to "%10u " so
			       that the value aligns with the rest of
			       the numbers in the columns.
> +	tst_printf("\n");
> +}

Otherwise the rest looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Cyril Hrubis]

Hi!
> +	/* Read the CPU affinity masks for each IRQ. See bitmap_string() in the kernel (%*pb) */
> +	for (row = 0; row < nr_irqs; row++) {
> +		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
> +		ret = SAFE_FILE_READAT(0, path, buf, size);
> +		if (ret < 1)
> +			tst_brk(TBROK, "Empty /proc/irq/%u/smp_affinity?", irq_ids[row]);
> +		c = buf;
> +		col = 0;
> +
> +		while (*c != '\0') {
> +			switch (*c) {
> +			case '\n':
> +			case ' ':
> +			case ',':
> +				c++;
> +				continue;
> +			case '0' ... '9':
> +				acc = *c - '0';
> +				break;
> +			case 'a' ... 'f':
> +				acc = 10 + *c - 'a';
> +				break;
> +			default:
> +				tst_res(TINFO, "%u/smp_affnity: %s", irq_ids[row], buf);
> +				tst_brk(TBROK, "Wasn't expecting 0x%02x", *c);
> +			}
> +
> +			for (mask_pos = 0; mask_pos < 4; mask_pos++) {
> +				if (mask_pos + col >= nr_cpus)
> +					break;
> +
> +				irq_affinity[row * nr_cpus + (nr_cpus - 1 - col - mask_pos)] =
> +					(acc & (8 >> mask_pos)) ? ALLOW : DENY;
> +			}

The mask here is actually wrong, since we go backward in the
irq_affinity array the bitmask has to grow not shrink so this should be
(acc & (1<<mask_pos))

> +			col += mask_pos;
> +			c++;
> +		}
> +	}
> +
> +	free(buf);
> +}
> +
> +static void print_irq_info(void)
> +{
> +	size_t row, col;
> +	unsigned int count;
> +	enum affinity aff;

It would be nice if we printed the CPUX header here as well so that it's
clear which column describes which CPU.

> +	for (row = 0; row < nr_irqs; row++) {
> +		tst_printf("%5u:", irq_ids[row]);
> +
> +		for (col = 0; col < nr_cpus; col++) {
> +			count = irq_stats[row * nr_cpus + col];
> +			aff = irq_affinity[row * nr_cpus + col] == ALLOW ? '+' : '-';
> +
> +			tst_printf("%10u%c", count, aff);
> +		}
> +
> +		tst_printf("\n");
> +	}
> +
> +	tst_printf("Total:");
> +
> +	for (col = 0; col < nr_cpus; col++)
> +		tst_printf("%11u", irq_stats[row * nr_cpus + col]);
                               ^
			       And I would change this to "%10u " so
			       that the value aligns with the rest of
			       the numbers in the columns.
> +	tst_printf("\n");
> +}

Otherwise the rest looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Cyril Hrubis]

Hi!
> +static void collect_irq_info(void)
> +{
> +	char *buf = NULL, *c, *first_row;
> +	char path[PATH_MAX];
> +	size_t size = 1024;
> +	size_t ret, row, col;
> +	long acc;
> +	unsigned int cpu_total, mask_pos;
> +	int fd = SAFE_OPEN("/proc/interrupts", O_RDONLY);
> +
> +	nr_cpus = 0;
> +	nr_irqs = 0;
> +
> +	do {
> +		size *= 2;
> +		buf = SAFE_REALLOC(buf, size);
> +		SAFE_LSEEK(fd, 0, SEEK_SET);
> +		ret = SAFE_READ(0, fd, buf, size - 1);
> +	} while (ret >= size - 1);

And actually this does not seem to work as expected for me, it reads
about half of the table, stops shortly before 4096. I guess that we
cannot really read it in one read() like this and that we are supposed
to read it one $PAGE_SIZE at a time instead. So we need a loop that
increases by $PAGE_SIZE and we would pass buf+len to the read, until the
read returns bytes read.

> +	SAFE_CLOSE(fd);
> +
> +	if (ret < 1)
> +		tst_brk(TBROK, "Empty /proc/interrupts?");
> +
> +	buf[ret] = '\0';
> +
> +	/* Count CPUs, header columns are like /CPU[0-9]+/ */
> +	for (c = buf; *c != '\0' && *c != '\n'; c++) {
> +		if (!strncmp(c, "CPU", 3))
> +			nr_cpus++;
> +	}
> +
> +	c++;
> +	first_row = c;
> +	/* Count IRQs, real IRQs start with /[0-9]+:/ */
> +	while (*c != '\0') {
> +		switch (*c) {
> +		case ' ':
> +		case '\t':
> +		case '\n':
> +		case '0' ... '9':
> +			c++;
> +			break;
> +		case ':':
> +			nr_irqs++;
> +			/* fall-through */
> +		default:
> +			while (*c != '\n' && *c != '\0')
> +				c++;
> +		}
> +	}
> +
> +	tst_res(TINFO, "Found %u CPUS, %u IRQs", nr_cpus, nr_irqs);
> +
> +	irq_ids = SAFE_REALLOC(irq_ids, nr_irqs * sizeof(*irq_ids));
> +	irq_stats = SAFE_REALLOC(irq_stats, nr_cpus * (nr_irqs + 1) * sizeof(*irq_stats));
> +	irq_affinity = SAFE_REALLOC(irq_affinity, nr_cpus * nr_irqs * sizeof(*irq_affinity));
> +
> +	c = first_row;
> +	acc = -1;
> +	row = col = 0;
> +	/* Parse columns containing IRQ counts and IRQ IDs into acc. Ignore everything else. */
> +	while (*c != '\0') {
> +		switch (*c) {
> +		case ' ':
> +		case '\t':
> +			if (acc >= 0) {
> +				irq_stats[row * nr_cpus + col] = acc;
> +				acc = -1;
> +				col++;
> +			}
> +			break;
> +		case '\n':
> +			col = 0;
> +			row++;
> +			break;
> +		case '0' ... '9':
> +			if (acc == -1)
> +				acc = 0;
> +
> +			acc *= 10;
> +			acc += *c - '0';
> +			break;
> +		case ':':
> +			irq_ids[row] = acc;
> +			acc = -1;
> +			break;
> +		default:
> +			acc = -1;
> +			while (*c != '\n' && *c != '\0')
> +				c++;
> +			continue;
> +		}
> +
> +		c++;
> +	}
> +
> +	for (col = 0; col < nr_cpus; col++) {
> +		cpu_total = 0;
> +
> +		for (row = 0; row < nr_irqs; row++)
> +			cpu_total += irq_stats[row * nr_cpus + col];
> +
> +		irq_stats[row * nr_cpus + col] = cpu_total;
> +	}
> +
> +	/* Read the CPU affinity masks for each IRQ. See bitmap_string() in the kernel (%*pb) */
> +	for (row = 0; row < nr_irqs; row++) {
> +		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
> +		ret = SAFE_FILE_READAT(0, path, buf, size);
> +		if (ret < 1)
> +			tst_brk(TBROK, "Empty /proc/irq/%u/smp_affinity?", irq_ids[row]);
> +		c = buf;
> +		col = 0;
> +
> +		while (*c != '\0') {
> +			switch (*c) {
> +			case '\n':
> +			case ' ':
> +			case ',':
> +				c++;
> +				continue;
> +			case '0' ... '9':
> +				acc = *c - '0';
> +				break;
> +			case 'a' ... 'f':
> +				acc = 10 + *c - 'a';
> +				break;
> +			default:
> +				tst_res(TINFO, "%u/smp_affnity: %s", irq_ids[row], buf);
> +				tst_brk(TBROK, "Wasn't expecting 0x%02x", *c);
> +			}
> +
> +			for (mask_pos = 0; mask_pos < 4; mask_pos++) {
> +				if (mask_pos + col >= nr_cpus)
> +					break;
> +
> +				irq_affinity[row * nr_cpus + (nr_cpus - 1 - col - mask_pos)] =
> +					(acc & (8 >> mask_pos)) ? ALLOW : DENY;
> +			}
> +
> +			col += mask_pos;
> +			c++;
> +		}
> +	}

Actually having a closer look this whole parsing looks wrong and the
reason why it does not work for me is that my machine has nr_cpus that
is not power of two i.e. 12 in this case and the size of the mask seems
to be rounded to 32 bits when presented in the proc files. I would have
expected that the kernel would choose the closest power of two i.e. 16,
but that does not seem to be the case here.

I guess that it would be actually easier to read the whole affinity into
a string and then parse it from the end instead.

> +	free(buf);
> +}
> +
> +static void print_irq_info(void)
> +{
> +	size_t row, col;
> +	unsigned int count;
> +	enum affinity aff;
> +
> +	for (row = 0; row < nr_irqs; row++) {
> +		tst_printf("%5u:", irq_ids[row]);
> +
> +		for (col = 0; col < nr_cpus; col++) {
> +			count = irq_stats[row * nr_cpus + col];
> +			aff = irq_affinity[row * nr_cpus + col] == ALLOW ? '+' : '-';
> +
> +			tst_printf("%10u%c", count, aff);
> +		}
> +
> +		tst_printf("\n");
> +	}
> +
> +	tst_printf("Total:");
> +
> +	for (col = 0; col < nr_cpus; col++)
> +		tst_printf("%11u", irq_stats[row * nr_cpus + col]);
> +
> +	tst_printf("\n");
> +}
> +
> +static void evidence_of_change(void)
> +{
> +	size_t row, col, changed = 0;
> +
> +	for (row = 0; row < nr_irqs; row++) {
> +		for (col = 0; col < nr_cpus; col++) {
> +			if (!irq_stats[row * nr_cpus + col])
> +				continue;
> +
> +			if (irq_affinity[row * nr_cpus + col] == ALLOW)
> +				continue;
> +
> +			changed++;
> +		}
> +	}
> +
> +	tst_res(changed ? TPASS : TFAIL,
> +		"Heuristic: Detected %zu irq-cpu pairs have been dissallowed",
> +		changed);
> +}
> +
> +static void setup(void)
> +{
> +	collect_irq_info();
> +	print_irq_info();
> +
> +	if (nr_cpus < 1)
> +		tst_brk(TBROK, "No CPUs found in /proc/interrupts?");
> +
> +	if (nr_irqs < 1)
> +		tst_brk(TBROK, "No IRQs found in /proc/interrupts?");
> +}
> +
> +static void run(void)
> +{
> +	collect_irq_info();
> +
> +	evidence_of_change();
> +}
> +
> +static void cleanup(void)
> +{
> +	if (irq_ids)
> +		free(irq_ids);
> +	if (irq_stats)
> +		free(irq_stats);
> +	if (irq_affinity)
> +		free(irq_affinity);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.min_cpus = 2,
> +};
> -- 
> 2.31.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Cyril Hrubis]

Hi!
> +static void collect_irq_info(void)
> +{
> +	char *buf = NULL, *c, *first_row;
> +	char path[PATH_MAX];
> +	size_t size = 1024;
> +	size_t ret, row, col;
> +	long acc;
> +	unsigned int cpu_total, mask_pos;
> +	int fd = SAFE_OPEN("/proc/interrupts", O_RDONLY);
> +
> +	nr_cpus = 0;
> +	nr_irqs = 0;
> +
> +	do {
> +		size *= 2;
> +		buf = SAFE_REALLOC(buf, size);
> +		SAFE_LSEEK(fd, 0, SEEK_SET);
> +		ret = SAFE_READ(0, fd, buf, size - 1);
> +	} while (ret >= size - 1);

And actually this does not seem to work as expected for me, it reads
about half of the table, stops shortly before 4096. I guess that we
cannot really read it in one read() like this and that we are supposed
to read it one $PAGE_SIZE at a time instead. So we need a loop that
increases by $PAGE_SIZE and we would pass buf+len to the read, until the
read returns bytes read.

> +	SAFE_CLOSE(fd);
> +
> +	if (ret < 1)
> +		tst_brk(TBROK, "Empty /proc/interrupts?");
> +
> +	buf[ret] = '\0';
> +
> +	/* Count CPUs, header columns are like /CPU[0-9]+/ */
> +	for (c = buf; *c != '\0' && *c != '\n'; c++) {
> +		if (!strncmp(c, "CPU", 3))
> +			nr_cpus++;
> +	}
> +
> +	c++;
> +	first_row = c;
> +	/* Count IRQs, real IRQs start with /[0-9]+:/ */
> +	while (*c != '\0') {
> +		switch (*c) {
> +		case ' ':
> +		case '\t':
> +		case '\n':
> +		case '0' ... '9':
> +			c++;
> +			break;
> +		case ':':
> +			nr_irqs++;
> +			/* fall-through */
> +		default:
> +			while (*c != '\n' && *c != '\0')
> +				c++;
> +		}
> +	}
> +
> +	tst_res(TINFO, "Found %u CPUS, %u IRQs", nr_cpus, nr_irqs);
> +
> +	irq_ids = SAFE_REALLOC(irq_ids, nr_irqs * sizeof(*irq_ids));
> +	irq_stats = SAFE_REALLOC(irq_stats, nr_cpus * (nr_irqs + 1) * sizeof(*irq_stats));
> +	irq_affinity = SAFE_REALLOC(irq_affinity, nr_cpus * nr_irqs * sizeof(*irq_affinity));
> +
> +	c = first_row;
> +	acc = -1;
> +	row = col = 0;
> +	/* Parse columns containing IRQ counts and IRQ IDs into acc. Ignore everything else. */
> +	while (*c != '\0') {
> +		switch (*c) {
> +		case ' ':
> +		case '\t':
> +			if (acc >= 0) {
> +				irq_stats[row * nr_cpus + col] = acc;
> +				acc = -1;
> +				col++;
> +			}
> +			break;
> +		case '\n':
> +			col = 0;
> +			row++;
> +			break;
> +		case '0' ... '9':
> +			if (acc == -1)
> +				acc = 0;
> +
> +			acc *= 10;
> +			acc += *c - '0';
> +			break;
> +		case ':':
> +			irq_ids[row] = acc;
> +			acc = -1;
> +			break;
> +		default:
> +			acc = -1;
> +			while (*c != '\n' && *c != '\0')
> +				c++;
> +			continue;
> +		}
> +
> +		c++;
> +	}
> +
> +	for (col = 0; col < nr_cpus; col++) {
> +		cpu_total = 0;
> +
> +		for (row = 0; row < nr_irqs; row++)
> +			cpu_total += irq_stats[row * nr_cpus + col];
> +
> +		irq_stats[row * nr_cpus + col] = cpu_total;
> +	}
> +
> +	/* Read the CPU affinity masks for each IRQ. See bitmap_string() in the kernel (%*pb) */
> +	for (row = 0; row < nr_irqs; row++) {
> +		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
> +		ret = SAFE_FILE_READAT(0, path, buf, size);
> +		if (ret < 1)
> +			tst_brk(TBROK, "Empty /proc/irq/%u/smp_affinity?", irq_ids[row]);
> +		c = buf;
> +		col = 0;
> +
> +		while (*c != '\0') {
> +			switch (*c) {
> +			case '\n':
> +			case ' ':
> +			case ',':
> +				c++;
> +				continue;
> +			case '0' ... '9':
> +				acc = *c - '0';
> +				break;
> +			case 'a' ... 'f':
> +				acc = 10 + *c - 'a';
> +				break;
> +			default:
> +				tst_res(TINFO, "%u/smp_affnity: %s", irq_ids[row], buf);
> +				tst_brk(TBROK, "Wasn't expecting 0x%02x", *c);
> +			}
> +
> +			for (mask_pos = 0; mask_pos < 4; mask_pos++) {
> +				if (mask_pos + col >= nr_cpus)
> +					break;
> +
> +				irq_affinity[row * nr_cpus + (nr_cpus - 1 - col - mask_pos)] =
> +					(acc & (8 >> mask_pos)) ? ALLOW : DENY;
> +			}
> +
> +			col += mask_pos;
> +			c++;
> +		}
> +	}

Actually having a closer look this whole parsing looks wrong and the
reason why it does not work for me is that my machine has nr_cpus that
is not power of two i.e. 12 in this case and the size of the mask seems
to be rounded to 32 bits when presented in the proc files. I would have
expected that the kernel would choose the closest power of two i.e. 16,
but that does not seem to be the case here.

I guess that it would be actually easier to read the whole affinity into
a string and then parse it from the end instead.

> +	free(buf);
> +}
> +
> +static void print_irq_info(void)
> +{
> +	size_t row, col;
> +	unsigned int count;
> +	enum affinity aff;
> +
> +	for (row = 0; row < nr_irqs; row++) {
> +		tst_printf("%5u:", irq_ids[row]);
> +
> +		for (col = 0; col < nr_cpus; col++) {
> +			count = irq_stats[row * nr_cpus + col];
> +			aff = irq_affinity[row * nr_cpus + col] == ALLOW ? '+' : '-';
> +
> +			tst_printf("%10u%c", count, aff);
> +		}
> +
> +		tst_printf("\n");
> +	}
> +
> +	tst_printf("Total:");
> +
> +	for (col = 0; col < nr_cpus; col++)
> +		tst_printf("%11u", irq_stats[row * nr_cpus + col]);
> +
> +	tst_printf("\n");
> +}
> +
> +static void evidence_of_change(void)
> +{
> +	size_t row, col, changed = 0;
> +
> +	for (row = 0; row < nr_irqs; row++) {
> +		for (col = 0; col < nr_cpus; col++) {
> +			if (!irq_stats[row * nr_cpus + col])
> +				continue;
> +
> +			if (irq_affinity[row * nr_cpus + col] == ALLOW)
> +				continue;
> +
> +			changed++;
> +		}
> +	}
> +
> +	tst_res(changed ? TPASS : TFAIL,
> +		"Heuristic: Detected %zu irq-cpu pairs have been dissallowed",
> +		changed);
> +}
> +
> +static void setup(void)
> +{
> +	collect_irq_info();
> +	print_irq_info();
> +
> +	if (nr_cpus < 1)
> +		tst_brk(TBROK, "No CPUs found in /proc/interrupts?");
> +
> +	if (nr_irqs < 1)
> +		tst_brk(TBROK, "No IRQs found in /proc/interrupts?");
> +}
> +
> +static void run(void)
> +{
> +	collect_irq_info();
> +
> +	evidence_of_change();
> +}
> +
> +static void cleanup(void)
> +{
> +	if (irq_ids)
> +		free(irq_ids);
> +	if (irq_stats)
> +		free(irq_stats);
> +	if (irq_affinity)
> +		free(irq_affinity);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.min_cpus = 2,
> +};
> -- 
> 2.31.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp