[PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Chris Metcalf]

This network (the "UDN") connects all the cpus on the chip in a
wormhole-routed dynamic network.  Subrectangles of the chip can
be allocated by a "create" ioctl on /dev/hardwall, and then to access the
UDN in that rectangle, tasks must perform an "activate" ioctl on that
same file object after affinitizing themselves to a single cpu in
the region.  Sending a wormhole-routed message that tries to leave
that subrectangle causes all activated tasks to receive a SIGILL
(just as they would if they tried to access the UDN without first
activating themselves to a hardwall rectangle).

The original submission of this code to LKML had the driver
instantiated under /proc/tile/hardwall.  Now we just use a character
device for this, conventionally /dev/hardwall.  Some futures planning
for the TILE-Gx chip suggests that we may want to have other types of
devices that share the general model of "bind a task to a cpu, then
'activate' a file descriptor on a pseudo-device that gives access to
some hardware resource".  As such, we are using a device rather
than, for example, a syscall, to set up and activate this code.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 arch/tile/Kconfig                 |    4 +
 arch/tile/include/asm/hardwall.h  |   57 +++
 arch/tile/include/asm/processor.h |   20 +-
 arch/tile/kernel/Makefile         |    1 +
 arch/tile/kernel/hardwall.c       |  817 +++++++++++++++++++++++++++++++++++++
 arch/tile/kernel/intvec_32.S      |    6 +-
 6 files changed, 901 insertions(+), 4 deletions(-)
 create mode 100644 arch/tile/include/asm/hardwall.h
 create mode 100644 arch/tile/kernel/hardwall.c

diff --git a/arch/tile/Kconfig b/arch/tile/Kconfig
index 290ef41..1eb308c 100644
--- a/arch/tile/Kconfig
+++ b/arch/tile/Kconfig
@@ -310,6 +310,10 @@ config VMALLOC_RESERVE
 	hex
 	default 0x1000000
 
+config HARDWALL
+	bool "Hardwall support to allow access to user dynamic network"
+	default y
+
 endmenu  # Tilera-specific configuration
 
 menu "Bus options"
diff --git a/arch/tile/include/asm/hardwall.h b/arch/tile/include/asm/hardwall.h
new file mode 100644
index 0000000..631a24f
--- /dev/null
+++ b/arch/tile/include/asm/hardwall.h
@@ -0,0 +1,57 @@
+/*
+ * Copyright 2010 Tilera Corporation. All Rights Reserved.
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful, but
+ *   WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ *   NON INFRINGEMENT.  See the GNU General Public License for
+ *   more details.
+ *
+ * Provide methods for the HARDWALL_FILE for accessing the UDN.
+ */
+
+#ifndef _ASM_TILE_HARDWALL_H
+#define _ASM_TILE_HARDWALL_H
+
+#include <linux/ioctl.h>
+
+#define HARDWALL_IOCTL_BASE 0xa2
+
+/*
+ * The HARDWALL_CREATE() ioctl is a macro with a "size" argument.
+ * The resulting ioctl value is passed to the kernel in conjunction
+ * with a pointer to a little-endian bitmask of cpus, which must be
+ * physically in a rectangular configuration on the chip.
+ * The "size" is the number of bytes of cpu mask data and must be a
+ * multiple of sizeof(long).
+ */
+#define _HARDWALL_CREATE 1
+#define HARDWALL_CREATE(size) \
+  _IOC(_IOC_READ, HARDWALL_IOCTL_BASE, _HARDWALL_CREATE, (size))
+
+#define _HARDWALL_ACTIVATE 2
+#define HARDWALL_ACTIVATE \
+  _IO(HARDWALL_IOCTL_BASE, _HARDWALL_ACTIVATE)
+
+#define _HARDWALL_DEACTIVATE 3
+#define HARDWALL_DEACTIVATE \
+ _IO(HARDWALL_IOCTL_BASE, _HARDWALL_DEACTIVATE)
+
+#ifndef __KERNEL__
+
+/* This is the canonical name expected by userspace. */
+#define HARDWALL_FILE "/dev/hardwall"
+
+#else
+
+/* Hook for /proc/tile/hardwall. */
+struct seq_file;
+int proc_tile_hardwall_show(struct seq_file *sf, void *v);
+
+#endif
+
+#endif /* _ASM_TILE_HARDWALL_H */
diff --git a/arch/tile/include/asm/processor.h b/arch/tile/include/asm/processor.h
index 96c50d2..95b54bc 100644
--- a/arch/tile/include/asm/processor.h
+++ b/arch/tile/include/asm/processor.h
@@ -74,6 +74,14 @@ struct async_tlb {
 	unsigned long address;   /* what address faulted? */
 };
 
+#ifdef CONFIG_HARDWALL
+struct hardwall_info;
+
+/* Can't use a normal list_head here due to header-file inclusion issues. */
+struct hardwall_list {
+	struct list_head *next, *prev;
+};
+#endif
 
 struct thread_struct {
 	/* kernel stack pointer */
@@ -100,6 +108,12 @@ struct thread_struct {
 	/* Any other miscellaneous processor state bits */
 	unsigned long proc_status;
 #endif
+#ifdef CONFIG_HARDWALL
+	/* Is this task tied to an activated hardwall? */
+	struct hardwall_info *hardwall;
+	/* Chains this task into the list at hardwall->list. */
+	struct hardwall_list hardwall_list;
+#endif
 #if CHIP_HAS_TILE_DMA()
 	/* Async DMA TLB fault information */
 	struct async_tlb dma_async_tlb;
@@ -194,8 +208,6 @@ static inline void release_thread(struct task_struct *dead_task)
 
 extern int kernel_thread(int (*fn)(void *), void *arg, unsigned long flags);
 
-/* Helper routines for setting home cache modes at exec() time. */
-
 
 /*
  * Return saved (kernel) PC of a blocked thread.
@@ -240,6 +252,10 @@ struct siginfo;
 extern void arch_coredump_signal(struct siginfo *, struct pt_regs *);
 #define arch_coredump_signal arch_coredump_signal
 
+/* Info on this processor (see fs/proc/cpuinfo.c) */
+struct seq_operations;
+extern const struct seq_operations cpuinfo_op;
+
 /* Provide information about the chip model. */
 extern char chip_model[64];
 
diff --git a/arch/tile/kernel/Makefile b/arch/tile/kernel/Makefile
index 756e6ec..112b1e2 100644
--- a/arch/tile/kernel/Makefile
+++ b/arch/tile/kernel/Makefile
@@ -8,6 +8,7 @@ obj-y := backtrace.o entry.o init_task.o irq.o messaging.o \
 	setup.o signal.o single_step.o stack.o sys.o time.o traps.o \
 	intvec_$(BITS).o regs_$(BITS).o tile-desc_$(BITS).o
 
+obj-$(CONFIG_HARDWALL)		+= hardwall.o
 obj-$(CONFIG_TILEGX)		+= futex_64.o
 obj-$(CONFIG_COMPAT)		+= compat.o compat_signal.o
 obj-$(CONFIG_SMP)		+= smpboot.o smp.o tlb.o
diff --git a/arch/tile/kernel/hardwall.c b/arch/tile/kernel/hardwall.c
new file mode 100644
index 0000000..27f2856
--- /dev/null
+++ b/arch/tile/kernel/hardwall.c
@@ -0,0 +1,817 @@
+/*
+ * Copyright 2010 Tilera Corporation. All Rights Reserved.
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful, but
+ *   WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ *   NON INFRINGEMENT.  See the GNU General Public License for
+ *   more details.
+ */
+
+#include <linux/fs.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+#include <linux/rwsem.h>
+#include <linux/kprobes.h>
+#include <linux/sched.h>
+#include <linux/hardirq.h>
+#include <linux/uaccess.h>
+#include <linux/smp.h>
+#include <linux/cdev.h>
+#include <asm/hardwall.h>
+#include <asm/traps.h>
+#include <asm/siginfo.h>
+#include <asm/irq_regs.h>
+
+#include <arch/interrupts.h>
+#include <arch/spr_def.h>
+
+
+/*
+ * This data structure tracks the rectangle data, etc., associated
+ * one-to-one with a "struct file *" from opening HARDWALL_FILE.
+ * Note that the file's private data points back to this structure.
+ */
+struct hardwall_info {
+	struct list_head list;             /* "rectangles" list */
+	struct list_head task_head;        /* head of tasks in this hardwall */
+	int ulhc_x;                        /* upper left hand corner x coord */
+	int ulhc_y;                        /* upper left hand corner y coord */
+	int width;                         /* rectangle width */
+	int height;                        /* rectangle height */
+	int teardown_in_progress;          /* are we tearing this one down? */
+};
+
+/* Work around list_head vs hardwall_list. */
+#define hardwall_for_each_entry(pos, head, member)			\
+	for (pos = list_entry((struct hardwall_list *)((head)->next),	\
+			      typeof(*pos), member);			\
+	     (struct list_head *)&pos->member != (head);		\
+	     pos = list_entry((struct hardwall_list *)(pos->member.next),\
+			      typeof(*pos), member))
+#define for_each_hardwall_task(pos, head) \
+	hardwall_for_each_entry(pos, head, thread.hardwall_list)
+
+#define hardwall_for_each_entry_safe(pos, n, head, member)		\
+	for (pos = list_entry((struct hardwall_list *)((head)->next),	\
+			      typeof(*pos), member),			\
+	     n = list_entry((struct hardwall_list *)(pos->member.next), \
+			      typeof(*pos), member);			\
+	     (struct list_head *)&pos->member != (head);		\
+	     pos = n,							\
+	     n = list_entry((struct hardwall_list *)(pos->member.next), \
+			      typeof(*n), member))
+#define for_each_hardwall_task_safe(pos, n, head) \
+	hardwall_for_each_entry_safe(pos, n, head, thread.hardwall_list)
+
+
+/* Currently allocated hardwall rectangles */
+static LIST_HEAD(rectangles);
+
+/*
+ * Guard changes to the hardwall data structures.
+ * This could be finer grained (e.g. one lock for the list of hardwall
+ * rectangles, then separate embedded locks for each one's list of tasks),
+ * but there are subtle correctness issues when trying to start with
+ * a task's "hardwall" pointer and lock the correct rectangle's embedded
+ * lock in the presence of a simultaneous deactivation, so it seems
+ * easier to have a single lock, given that none of these data
+ * structures are touched very frequently during normal operation.
+ */
+static DEFINE_SPINLOCK(hardwall_lock);
+
+/* Allow disabling UDN access. */
+static int udn_disabled;
+static int __init noudn(char *str)
+{
+	pr_info("User-space UDN access is disabled\n");
+	udn_disabled = 1;
+	return 0;
+}
+early_param("noudn", noudn);
+
+
+/*
+ * Low-level primitives
+ */
+
+/* Map a HARDWALL_FILE file to the matching hardwall info structure */
+#define _file_to_hardwall(file) ((file)->private_data)
+#define file_to_hardwall(file) \
+	((struct hardwall_info *)_file_to_hardwall(file))
+
+/* Set a CPU bit if the CPU is online. */
+#define cpu_online_set(cpu, dst) do { \
+	if (cpu_online(cpu))          \
+		cpumask_set_cpu(cpu, dst);    \
+} while (0)
+
+
+/* Does the given rectangle contain the given x,y coordinate? */
+static int contains(struct hardwall_info *r, int x, int y)
+{
+	return (x >= r->ulhc_x && x < r->ulhc_x + r->width) &&
+		(y >= r->ulhc_y && y < r->ulhc_y + r->height);
+}
+
+/* Compute the rectangle parameters and validate the cpumask. */
+static int setup_rectangle(struct hardwall_info *r, struct cpumask *mask)
+{
+	int x, y, cpu, ulhc, lrhc;
+
+	/* The first cpu is the ULHC, the last the LRHC. */
+	ulhc = find_first_bit(cpumask_bits(mask), nr_cpumask_bits);
+	lrhc = find_last_bit(cpumask_bits(mask), nr_cpumask_bits);
+
+	/* Compute the rectangle attributes from the cpus. */
+	r->ulhc_x = cpu_x(ulhc);
+	r->ulhc_y = cpu_y(ulhc);
+	r->width = cpu_x(lrhc) - r->ulhc_x + 1;
+	r->height = cpu_y(lrhc) - r->ulhc_y + 1;
+
+	/* Width and height must be positive */
+	if (r->width <= 0 || r->height <= 0)
+		return -EINVAL;
+
+	/* Confirm that the cpumask is exactly the rectangle. */
+	for (y = 0, cpu = 0; y < smp_height; ++y)
+		for (x = 0; x < smp_width; ++x, ++cpu)
+			if (cpumask_test_cpu(cpu, mask) != contains(r, x, y))
+				return -EINVAL;
+
+	/*
+	 * Note that offline cpus can't be drained when this UDN
+	 * rectangle eventually closes.  We used to detect this
+	 * situation and print a warning, but it annoyed users and
+	 * they ignored it anyway, so now we just return without a
+	 * warning.
+	 */
+	return 0;
+}
+
+/* Do the two given rectangles overlap on any cpu? */
+static int overlaps(struct hardwall_info *a, struct hardwall_info *b)
+{
+	return a->ulhc_x + a->width > b->ulhc_x &&    /* A not to the left */
+		b->ulhc_x + b->width > a->ulhc_x &&   /* B not to the left */
+		a->ulhc_y + a->height > b->ulhc_y &&  /* A not above */
+		b->ulhc_y + b->height > a->ulhc_y;    /* B not above */
+}
+
+
+/*
+ * Hardware management of hardwall setup, teardown, trapping,
+ * and enabling/disabling PL0 access to the networks.
+ */
+
+/* Bit field values to mask together for writes to SPR_XDN_DIRECTION_PROTECT */
+enum direction_protect {
+	N_PROTECT = (1 << 0),
+	E_PROTECT = (1 << 1),
+	S_PROTECT = (1 << 2),
+	W_PROTECT = (1 << 3)
+};
+
+static void enable_firewall_interrupts(void)
+{
+	raw_local_irq_unmask_now(INT_UDN_FIREWALL);
+}
+
+static void disable_firewall_interrupts(void)
+{
+	raw_local_irq_mask_now(INT_UDN_FIREWALL);
+}
+
+/* Set up hardwall on this cpu based on the passed hardwall_info. */
+static void hardwall_setup_ipi_func(void *info)
+{
+	struct hardwall_info *r = info;
+	int cpu = smp_processor_id();
+	int x = cpu % smp_width;
+	int y = cpu / smp_width;
+	int bits = 0;
+	if (x == r->ulhc_x)
+		bits |= W_PROTECT;
+	if (x == r->ulhc_x + r->width - 1)
+		bits |= E_PROTECT;
+	if (y == r->ulhc_y)
+		bits |= N_PROTECT;
+	if (y == r->ulhc_y + r->height - 1)
+		bits |= S_PROTECT;
+	BUG_ON(bits == 0);
+	__insn_mtspr(SPR_UDN_DIRECTION_PROTECT, bits);
+	enable_firewall_interrupts();
+
+}
+
+/* Set up all cpus on edge of rectangle to enable/disable hardwall SPRs. */
+static void hardwall_setup(struct hardwall_info *r)
+{
+	int x, y, cpu, delta;
+	struct cpumask rect_cpus;
+
+	cpumask_clear(&rect_cpus);
+
+	/* First include the top and bottom edges */
+	cpu = r->ulhc_y * smp_width + r->ulhc_x;
+	delta = (r->height - 1) * smp_width;
+	for (x = 0; x < r->width; ++x, ++cpu) {
+		cpu_online_set(cpu, &rect_cpus);
+		cpu_online_set(cpu + delta, &rect_cpus);
+	}
+
+	/* Then the left and right edges */
+	cpu -= r->width;
+	delta = r->width - 1;
+	for (y = 0; y < r->height; ++y, cpu += smp_width) {
+		cpu_online_set(cpu, &rect_cpus);
+		cpu_online_set(cpu + delta, &rect_cpus);
+	}
+
+	/* Then tell all the cpus to set up their protection SPR */
+	on_each_cpu_mask(&rect_cpus, hardwall_setup_ipi_func, r, 1);
+}
+
+void __kprobes do_hardwall_trap(struct pt_regs* regs, int fault_num)
+{
+	struct hardwall_info *rect;
+	struct task_struct *p;
+	struct siginfo info;
+	int x, y;
+	int cpu = smp_processor_id();
+	int found_processes;
+	unsigned long flags;
+
+	struct pt_regs *old_regs = set_irq_regs(regs);
+	irq_enter();
+
+	/* This tile trapped a network access; find the rectangle. */
+	x = cpu % smp_width;
+	y = cpu / smp_width;
+	spin_lock_irqsave(&hardwall_lock, flags);
+	list_for_each_entry(rect, &rectangles, list) {
+		if (contains(rect, x, y))
+			break;
+	}
+
+	/*
+	 * It shouldn't be possible not to find this cpu on the
+	 * rectangle list, since only cpus in rectangles get hardwalled.
+	 * The hardwall is only removed after the UDN is drained.
+	 */
+	BUG_ON(&rect->list == &rectangles);
+
+	/*
+	 * If we already started teardown on this hardwall, don't worry;
+	 * the abort signal has been sent and we are just waiting for things
+	 * to quiesce.
+	 */
+	if (rect->teardown_in_progress) {
+		pr_notice("cpu %d: detected hardwall violation %#lx"
+		       " while teardown already in progress\n",
+		       cpu, (long) __insn_mfspr(SPR_UDN_DIRECTION_PROTECT));
+		goto done;
+	}
+
+	/*
+	 * Kill off any process that is activated in this rectangle.
+	 * We bypass security to deliver the signal, since it must be
+	 * one of the activated processes that generated the UDN
+	 * message that caused this trap, and all the activated
+	 * processes shared a single open file so are pretty tightly
+	 * bound together from a security point of view to begin with.
+	 */
+	rect->teardown_in_progress = 1;
+	wmb(); /* Ensure visibility of rectangle before notifying processes. */
+	pr_notice("cpu %d: detected hardwall violation %#lx...\n",
+	       cpu, (long) __insn_mfspr(SPR_UDN_DIRECTION_PROTECT));
+	info.si_signo = SIGILL;
+	info.si_errno = 0;
+	info.si_code = ILL_HARDWALL;
+	found_processes = 0;
+	for_each_hardwall_task(p, &rect->task_head) {
+		BUG_ON(p->thread.hardwall != rect);
+		if (p->sighand) {
+			found_processes = 1;
+			pr_notice("hardwall: killing %d\n", p->pid);
+			spin_lock(&p->sighand->siglock);
+			__group_send_sig_info(info.si_signo, &info, p);
+			spin_unlock(&p->sighand->siglock);
+		}
+	}
+	if (!found_processes)
+		pr_notice("hardwall: no associated processes!\n");
+
+ done:
+	spin_unlock_irqrestore(&hardwall_lock, flags);
+
+	/*
+	 * We have to disable firewall interrupts now, or else when we
+	 * return from this handler, we will simply re-interrupt back to
+	 * it.  However, we can't clear the protection bits, since we
+	 * haven't yet drained the network, and that would allow packets
+	 * to cross out of the hardwall region.
+	 */
+	disable_firewall_interrupts();
+
+	irq_exit();
+	set_irq_regs(old_regs);
+}
+
+/* Allow access from user space to the UDN. */
+void grant_network_mpls(void)
+{
+	__insn_mtspr(SPR_MPL_UDN_ACCESS_SET_0, 1);
+	__insn_mtspr(SPR_MPL_UDN_AVAIL_SET_0, 1);
+	__insn_mtspr(SPR_MPL_UDN_COMPLETE_SET_0, 1);
+	__insn_mtspr(SPR_MPL_UDN_TIMER_SET_0, 1);
+#if !CHIP_HAS_REV1_XDN()
+	__insn_mtspr(SPR_MPL_UDN_REFILL_SET_0, 1);
+	__insn_mtspr(SPR_MPL_UDN_CA_SET_0, 1);
+#endif
+}
+
+/* Deny access from user space to the UDN. */
+void restrict_network_mpls(void)
+{
+	__insn_mtspr(SPR_MPL_UDN_ACCESS_SET_1, 1);
+	__insn_mtspr(SPR_MPL_UDN_AVAIL_SET_1, 1);
+	__insn_mtspr(SPR_MPL_UDN_COMPLETE_SET_1, 1);
+	__insn_mtspr(SPR_MPL_UDN_TIMER_SET_1, 1);
+#if !CHIP_HAS_REV1_XDN()
+	__insn_mtspr(SPR_MPL_UDN_REFILL_SET_1, 1);
+	__insn_mtspr(SPR_MPL_UDN_CA_SET_1, 1);
+#endif
+}
+
+
+/*
+ * Code to create, activate, deactivate, and destroy hardwall rectangles.
+ */
+
+/* Create a hardwall for the given rectangle */
+static struct hardwall_info *hardwall_create(
+	size_t size, const unsigned long __user *bits)
+{
+	struct hardwall_info *iter, *rect;
+	struct cpumask mask;
+	unsigned long flags;
+	int rc, cpu, maxcpus = smp_height * smp_width;
+
+	/* If "size" is not a multiple of long, it's invalid. */
+	if (size % sizeof(long))
+		return ERR_PTR(-EINVAL);
+
+	/* Validate that all the bits we are given fit in our coordinates. */
+	cpumask_clear(&mask);
+	for (cpu = 0; size > 0; ++bits, size -= sizeof(long)) {
+		int i;
+		unsigned long m;
+		if (get_user(m, bits) != 0)
+			return ERR_PTR(-EFAULT);
+		for (i = 0; i < BITS_PER_LONG; ++i, ++cpu)
+			if (m & (1UL << i)) {
+				if (cpu >= maxcpus)
+					return ERR_PTR(-EINVAL);
+				cpumask_set_cpu(cpu, &mask);
+			}
+	}
+
+	/* Allocate a new rectangle optimistically. */
+	rect = kmalloc(sizeof(struct hardwall_info),
+			GFP_KERNEL | __GFP_ZERO);
+	INIT_LIST_HEAD(&rect->task_head);
+
+	/* Compute the rectangle size and validate that it's plausible. */
+	rc = setup_rectangle(rect, &mask);
+	if (rc != 0) {
+		kfree(rect);
+		return ERR_PTR(rc);
+	}
+
+	/* Confirm it doesn't overlap and add it to the list. */
+	spin_lock_irqsave(&hardwall_lock, flags);
+	list_for_each_entry(iter, &rectangles, list) {
+		if (overlaps(iter, rect)) {
+			spin_unlock_irqrestore(&hardwall_lock, flags);
+			kfree(rect);
+			return ERR_PTR(-EBUSY);
+		}
+	}
+	list_add_tail(&rect->list, &rectangles);
+	spin_unlock_irqrestore(&hardwall_lock, flags);
+
+	/* Set up appropriate hardwalling on all affected cpus. */
+	hardwall_setup(rect);
+
+	return rect;
+}
+
+/* Activate a given hardwall on this cpu for this process. */
+static int hardwall_activate(struct hardwall_info *rect)
+{
+	int cpu, x, y;
+	unsigned long flags;
+	struct task_struct *p = current;
+	struct thread_struct *ts = &p->thread;
+
+	/* Require a rectangle. */
+	if (rect == NULL)
+		return -ENODATA;
+
+	/* Not allowed to activate a rectangle that is being torn down. */
+	if (rect->teardown_in_progress)
+		return -EINVAL;
+
+	/*
+	 * Get our affinity; if we're not bound to this tile uniquely,
+	 * we can't access the network registers.
+	 */
+	if (cpumask_weight(&p->cpus_allowed) != 1)
+		return -EPERM;
+
+	/* Make sure we are bound to a cpu in this rectangle. */
+	cpu = smp_processor_id();
+	BUG_ON(cpumask_first(&p->cpus_allowed) != cpu);
+	x = cpu_x(cpu);
+	y = cpu_y(cpu);
+	if (!contains(rect, x, y))
+		return -EINVAL;
+
+	/* If we are already bound to this hardwall, it's a no-op. */
+	if (ts->hardwall) {
+		BUG_ON(ts->hardwall != rect);
+		return 0;
+	}
+
+	/* Success!  This process gets to use the user networks on this cpu. */
+	ts->hardwall = rect;
+	spin_lock_irqsave(&hardwall_lock, flags);
+	list_add((struct list_head *)&ts->hardwall_list, &rect->task_head);
+	spin_unlock_irqrestore(&hardwall_lock, flags);
+	grant_network_mpls();
+	printk(KERN_DEBUG "Pid %d (%s) activated for hardwall: cpu %d\n",
+	       p->pid, p->comm, cpu);
+	return 0;
+}
+
+/*
+ * Deactivate a task's hardwall.  Must hold hardwall_lock.
+ * This method may be called from free_task(), so we don't want to
+ * rely on too many fields of struct task_struct still being valid.
+ * We assume the cpus_allowed, pid, and comm fields are still valid.
+ */
+static void _hardwall_deactivate(struct task_struct *task)
+{
+	struct thread_struct *ts = &task->thread;
+
+	if (cpumask_weight(&task->cpus_allowed) != 1) {
+		pr_err("pid %d (%s) releasing networks with"
+		       " an affinity mask containing %d cpus!\n",
+		       task->pid, task->comm,
+		       cpumask_weight(&task->cpus_allowed));
+		BUG();
+	}
+
+	BUG_ON(ts->hardwall == NULL);
+	ts->hardwall = NULL;
+	list_del((struct list_head *)&ts->hardwall_list);
+	if (task == current)
+		restrict_network_mpls();
+}
+
+/* Deactivate a task's hardwall. */
+int hardwall_deactivate(struct task_struct *task)
+{
+	unsigned long flags;
+	int activated;
+
+	spin_lock_irqsave(&hardwall_lock, flags);
+	activated = (task->thread.hardwall != NULL);
+	if (activated)
+		_hardwall_deactivate(task);
+	spin_unlock_irqrestore(&hardwall_lock, flags);
+
+	if (!activated)
+		return -EINVAL;
+
+	printk(KERN_DEBUG "Pid %d (%s) deactivated for hardwall: cpu %d\n",
+	       task->pid, task->comm, smp_processor_id());
+	return 0;
+}
+
+/* Stop a UDN switch before draining the network. */
+static void stop_udn_switch(void *ignored)
+{
+#if !CHIP_HAS_REV1_XDN()
+	/* Freeze the switch and the demux. */
+	__insn_mtspr(SPR_UDN_SP_FREEZE,
+		     SPR_UDN_SP_FREEZE__SP_FRZ_MASK |
+		     SPR_UDN_SP_FREEZE__DEMUX_FRZ_MASK |
+		     SPR_UDN_SP_FREEZE__NON_DEST_EXT_MASK);
+#endif
+}
+
+/* Drain all the state from a stopped switch. */
+static void drain_udn_switch(void *ignored)
+{
+#if !CHIP_HAS_REV1_XDN()
+	int i;
+	int from_tile_words, ca_count;
+
+	/* Empty out the 5 switch point fifos. */
+	for (i = 0; i < 5; i++) {
+		int words, j;
+		__insn_mtspr(SPR_UDN_SP_FIFO_SEL, i);
+		words = __insn_mfspr(SPR_UDN_SP_STATE) & 0xF;
+		for (j = 0; j < words; j++)
+			(void) __insn_mfspr(SPR_UDN_SP_FIFO_DATA);
+		BUG_ON((__insn_mfspr(SPR_UDN_SP_STATE) & 0xF) != 0);
+	}
+
+	/* Dump out the 3 word fifo at top. */
+	from_tile_words = (__insn_mfspr(SPR_UDN_DEMUX_STATUS) >> 10) & 0x3;
+	for (i = 0; i < from_tile_words; i++)
+		(void) __insn_mfspr(SPR_UDN_DEMUX_WRITE_FIFO);
+
+	/* Empty out demuxes. */
+	while (__insn_mfspr(SPR_UDN_DATA_AVAIL) & (1 << 0))
+		(void) __tile_udn0_receive();
+	while (__insn_mfspr(SPR_UDN_DATA_AVAIL) & (1 << 1))
+		(void) __tile_udn1_receive();
+	while (__insn_mfspr(SPR_UDN_DATA_AVAIL) & (1 << 2))
+		(void) __tile_udn2_receive();
+	while (__insn_mfspr(SPR_UDN_DATA_AVAIL) & (1 << 3))
+		(void) __tile_udn3_receive();
+	BUG_ON((__insn_mfspr(SPR_UDN_DATA_AVAIL) & 0xF) != 0);
+
+	/* Empty out catch all. */
+	ca_count = __insn_mfspr(SPR_UDN_DEMUX_CA_COUNT);
+	for (i = 0; i < ca_count; i++)
+		(void) __insn_mfspr(SPR_UDN_CA_DATA);
+	BUG_ON(__insn_mfspr(SPR_UDN_DEMUX_CA_COUNT) != 0);
+
+	/* Clear demux logic. */
+	__insn_mtspr(SPR_UDN_DEMUX_CTL, 1);
+
+	/*
+	 * Write switch state; experimentation indicates that 0xc3000
+	 * is an idle switch point.
+	 */
+	for (i = 0; i < 5; i++) {
+		__insn_mtspr(SPR_UDN_SP_FIFO_SEL, i);
+		__insn_mtspr(SPR_UDN_SP_STATE, 0xc3000);
+	}
+#endif
+}
+
+/* Reset random UDN state registers at boot up and during hardwall teardown. */
+void reset_network_state(void)
+{
+#if !CHIP_HAS_REV1_XDN()
+	/* Reset UDN coordinates to their standard value */
+	unsigned int cpu = smp_processor_id();
+	unsigned int x = cpu % smp_width;
+	unsigned int y = cpu / smp_width;
+#endif
+
+	if (udn_disabled)
+		return;
+
+#if !CHIP_HAS_REV1_XDN()
+	__insn_mtspr(SPR_UDN_TILE_COORD, (x << 18) | (y << 7));
+
+	/* Set demux tags to predefined values and enable them. */
+	__insn_mtspr(SPR_UDN_TAG_VALID, 0xf);
+	__insn_mtspr(SPR_UDN_TAG_0, (1 << 0));
+	__insn_mtspr(SPR_UDN_TAG_1, (1 << 1));
+	__insn_mtspr(SPR_UDN_TAG_2, (1 << 2));
+	__insn_mtspr(SPR_UDN_TAG_3, (1 << 3));
+#endif
+
+	/* Clear out other random registers so we have a clean slate. */
+	__insn_mtspr(SPR_UDN_AVAIL_EN, 0);
+	__insn_mtspr(SPR_UDN_DEADLOCK_TIMEOUT, 0);
+#if !CHIP_HAS_REV1_XDN()
+	__insn_mtspr(SPR_UDN_REFILL_EN, 0);
+	__insn_mtspr(SPR_UDN_DEMUX_QUEUE_SEL, 0);
+	__insn_mtspr(SPR_UDN_SP_FIFO_SEL, 0);
+#endif
+
+	/* Start the switch and demux. */
+#if !CHIP_HAS_REV1_XDN()
+	__insn_mtspr(SPR_UDN_SP_FREEZE, 0);
+#endif
+}
+
+/* Restart a UDN switch after draining. */
+static void restart_udn_switch(void *ignored)
+{
+	reset_network_state();
+
+	/* Disable firewall interrupts. */
+	__insn_mtspr(SPR_UDN_DIRECTION_PROTECT, 0);
+	disable_firewall_interrupts();
+}
+
+/* Build a struct cpumask containing all valid tiles in bounding rectangle. */
+static void fill_mask(struct hardwall_info *r, struct cpumask *result)
+{
+	int x, y, cpu;
+
+	cpumask_clear(result);
+
+	cpu = r->ulhc_y * smp_width + r->ulhc_x;
+	for (y = 0; y < r->height; ++y, cpu += smp_width - r->width) {
+		for (x = 0; x < r->width; ++x, ++cpu)
+			cpu_online_set(cpu, result);
+	}
+}
+
+/* Last reference to a hardwall is gone, so clear the network. */
+static void hardwall_destroy(struct hardwall_info *rect)
+{
+	struct task_struct *task;
+	unsigned long flags;
+	struct cpumask mask;
+
+	/* Make sure this file actually represents a rectangle. */
+	if (rect == NULL)
+		return;
+
+	/*
+	 * Deactivate any remaining tasks.  It's possible to race with
+	 * some other thread that is exiting and hasn't yet called
+	 * deactivate (when freeing its thread_info), so we carefully
+	 * deactivate any remaining tasks before freeing the
+	 * hardwall_info object itself.
+	 */
+	spin_lock_irqsave(&hardwall_lock, flags);
+	for_each_hardwall_task(task, &rect->task_head)
+		_hardwall_deactivate(task);
+	spin_unlock_irqrestore(&hardwall_lock, flags);
+
+	/* Drain the UDN. */
+	printk(KERN_DEBUG "Clearing hardwall rectangle %dx%d %d,%d\n",
+	       rect->width, rect->height, rect->ulhc_x, rect->ulhc_y);
+	fill_mask(rect, &mask);
+	on_each_cpu_mask(&mask, stop_udn_switch, NULL, 1);
+	on_each_cpu_mask(&mask, drain_udn_switch, NULL, 1);
+
+	/* Restart switch and disable firewall. */
+	on_each_cpu_mask(&mask, restart_udn_switch, NULL, 1);
+
+	/* Now free the rectangle from the list. */
+	spin_lock_irqsave(&hardwall_lock, flags);
+	BUG_ON(!list_empty(&rect->task_head));
+	list_del(&rect->list);
+	spin_unlock_irqrestore(&hardwall_lock, flags);
+	kfree(rect);
+}
+
+
+/*
+ * Dump hardwall state via /proc; initialized in arch/tile/sys/proc.c.
+ */
+int proc_tile_hardwall_show(struct seq_file *sf, void *v)
+{
+	struct hardwall_info *r;
+
+	if (udn_disabled) {
+		seq_printf(sf, "%dx%d 0,0 pids:\n", smp_width, smp_height);
+		return 0;
+	}
+
+	spin_lock_irq(&hardwall_lock);
+	list_for_each_entry(r, &rectangles, list) {
+		struct task_struct *p;
+		seq_printf(sf, "%dx%d %d,%d pids:",
+			   r->width, r->height, r->ulhc_x, r->ulhc_y);
+		for_each_hardwall_task(p, &r->task_head) {
+			unsigned int cpu = cpumask_first(&p->cpus_allowed);
+			unsigned int x = cpu % smp_width;
+			unsigned int y = cpu / smp_width;
+			seq_printf(sf, " %d@%d,%d", p->pid, x, y);
+		}
+		seq_printf(sf, "\n");
+	}
+	spin_unlock_irq(&hardwall_lock);
+	return 0;
+}
+
+
+/*
+ * Character device support via ioctl/close.
+ */
+
+static long hardwall_ioctl(struct file *file, unsigned int a, unsigned long b)
+{
+	struct hardwall_info *rect = file_to_hardwall(file);
+
+	if (_IOC_TYPE(a) != HARDWALL_IOCTL_BASE)
+		return -EINVAL;
+
+	switch (_IOC_NR(a)) {
+	case _HARDWALL_CREATE:
+		if (udn_disabled)
+			return -ENOSYS;
+		if (rect != NULL)
+			return -EALREADY;
+		rect = hardwall_create(_IOC_SIZE(a),
+					(const unsigned long __user *)b);
+		if (IS_ERR(rect))
+			return PTR_ERR(rect);
+		_file_to_hardwall(file) = rect;
+		return 0;
+
+	case _HARDWALL_ACTIVATE:
+		return hardwall_activate(rect);
+
+	case _HARDWALL_DEACTIVATE:
+		if (current->thread.hardwall != rect)
+			return -EINVAL;
+		return hardwall_deactivate(current);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+#ifdef CONFIG_COMPAT
+static long hardwall_compat_ioctl(struct file *file,
+				  unsigned int a, unsigned long b)
+{
+	/* Sign-extend the argument so it can be used as a pointer. */
+	return hardwall_ioctl(file, a, (int)(long)b);
+}
+#endif
+
+/* The user process closed the file; revoke access to user networks. */
+static int hardwall_flush(struct file *file, fl_owner_t owner)
+{
+	struct hardwall_info *rect = file_to_hardwall(file);
+	struct task_struct *task, *tmp;
+	unsigned long flags;
+
+	if (rect) {
+		/*
+		 * NOTE: if multiple threads are activated on this hardwall
+		 * file, the other threads will continue having access to the
+		 * UDN until they are context-switched out and back in again.
+		 *
+		 * NOTE: A NULL files pointer means the task is being torn
+		 * down, so in that case we also deactivate it.
+		 */
+		spin_lock_irqsave(&hardwall_lock, flags);
+		for_each_hardwall_task_safe(task, tmp, &rect->task_head) {
+			if (task->files == owner || task->files == NULL)
+				_hardwall_deactivate(task);
+		}
+		spin_unlock_irqrestore(&hardwall_lock, flags);
+	}
+
+	return 0;
+}
+
+/* This hardwall is gone, so destroy it. */
+static int hardwall_release(struct inode *inode, struct file *file)
+{
+	hardwall_destroy(file_to_hardwall(file));
+	return 0;
+}
+
+static const struct file_operations dev_hardwall_fops = {
+	.unlocked_ioctl = hardwall_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl   = hardwall_compat_ioctl,
+#endif
+	.flush          = hardwall_flush,
+	.release        = hardwall_release,
+};
+
+static struct cdev hardwall_dev;
+
+static int __init dev_hardwall_init(void)
+{
+	int rc;
+	dev_t dev;
+
+	rc = alloc_chrdev_region(&dev, 0, 1, "hardwall");
+	if (rc < 0)
+		return rc;
+	cdev_init(&hardwall_dev, &dev_hardwall_fops);
+	rc = cdev_add(&hardwall_dev, dev, 1);
+	if (rc < 0)
+		return rc;
+
+	/* Verify that hardwall_list can stand in for list_head. */
+	BUILD_BUG_ON(sizeof(struct list_head) !=
+		     sizeof(current->thread.hardwall_list));
+
+	return 0;
+}
+late_initcall(dev_hardwall_init);
diff --git a/arch/tile/kernel/intvec_32.S b/arch/tile/kernel/intvec_32.S
index 207271f..3404c75 100644
--- a/arch/tile/kernel/intvec_32.S
+++ b/arch/tile/kernel/intvec_32.S
@@ -17,9 +17,9 @@
 #include <linux/linkage.h>
 #include <linux/errno.h>
 #include <linux/init.h>
+#include <linux/unistd.h>
 #include <asm/ptrace.h>
 #include <asm/thread_info.h>
-#include <asm/unistd.h>
 #include <asm/irqflags.h>
 #include <asm/atomic.h>
 #include <asm/asm-offsets.h>
@@ -32,7 +32,7 @@
 # error "No support for kernel preemption currently"
 #endif
 
-#if INT_INTCTRL_1 < 32 || INT_INTCTL_1 >= 48
+#if INT_INTCTRL_1 < 32 || INT_INTCTRL_1 >= 48
 # error INT_INTCTRL_1 coded to set high interrupt mask
 #endif
 
@@ -1941,7 +1941,9 @@ int_unalign:
 #define op_handle_perf_interrupt bad_intr
 #define op_handle_aux_perf_interrupt bad_intr
 
+#ifndef CONFIG_HARDWALL
 #define do_hardwall_trap bad_intr
+#endif
 
 	int_hand     INT_ITLB_MISS, ITLB_MISS, \
 		     do_page_fault, handle_interrupt_no_single_step
-- 
1.6.5.2

Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Arnd Bergmann]

On Friday 25 June 2010, Chris Metcalf wrote:

> The original submission of this code to LKML had the driver
> instantiated under /proc/tile/hardwall.  Now we just use a character
> device for this, conventionally /dev/hardwall.  Some futures planning
> for the TILE-Gx chip suggests that we may want to have other types of
> devices that share the general model of "bind a task to a cpu, then
> 'activate' a file descriptor on a pseudo-device that gives access to
> some hardware resource".  As such, we are using a device rather
> than, for example, a syscall, to set up and activate this code.

The character device looks much better than the proc file. I still
think a system call would be a more appropriate abstraction here,
but the two are equivalent after all.

> diff --git a/arch/tile/include/asm/processor.h b/arch/tile/include/asm/processor.h
> index 96c50d2..95b54bc 100644
> --- a/arch/tile/include/asm/processor.h
> +++ b/arch/tile/include/asm/processor.h
> @@ -74,6 +74,14 @@ struct async_tlb {
>  	unsigned long address;   /* what address faulted? */
>  };
>  
> +#ifdef CONFIG_HARDWALL
> +struct hardwall_info;
> +
> +/* Can't use a normal list_head here due to header-file inclusion issues. */
> +struct hardwall_list {
> +	struct list_head *next, *prev;
> +};
> +#endif

It seems strange that you need this. Why does linux/list.h
depend on asm/processor.h?
It certainly seems that the code would get simpler if this
can be avoided.

> +/*
> + * Guard changes to the hardwall data structures.
> + * This could be finer grained (e.g. one lock for the list of hardwall
> + * rectangles, then separate embedded locks for each one's list of tasks),
> + * but there are subtle correctness issues when trying to start with
> + * a task's "hardwall" pointer and lock the correct rectangle's embedded
> + * lock in the presence of a simultaneous deactivation, so it seems
> + * easier to have a single lock, given that none of these data
> + * structures are touched very frequently during normal operation.
> + */
> +static DEFINE_SPINLOCK(hardwall_lock);

I think instead of making the lock more fine-grained, a better optimization
might be to use RCU to protect the updates. AFAICT, the updates to the
structure are rare but you need to read it a lot.

> +/*
> + * Low-level primitives
> + */
> +
> +/* Map a HARDWALL_FILE file to the matching hardwall info structure */
> +#define _file_to_hardwall(file) ((file)->private_data)
> +#define file_to_hardwall(file) \
> +	((struct hardwall_info *)_file_to_hardwall(file))

I wouldn't bother with these abstractions, you can simply write
file->private_data in every place where they are used.

> +/*
> + * Code to create, activate, deactivate, and destroy hardwall rectangles.
> + */
> +
> +/* Create a hardwall for the given rectangle */
> +static struct hardwall_info *hardwall_create(
> +	size_t size, const unsigned long __user *bits)
> +{

To make this a system call, I'd wrap this function with one that roughly
does

asmlinkage long sys_hardwall_create(size_t size,
			 const unsigned long __user *bits)
{
	int fd;
	struct hardwall_info *info;

	info = hardwall_create(size, bits);
	if (IS_ERR(info))
		return PTR_ERR(info);

	fd = anon_inode_getfd("hardwall", &hardwall_fops, info, O_CREAT | O_RDWR);
	if (fd < 0)
		hardwall_destroy(info);
	
	return fd;
}

The main difference between this and the ioctl approach would be that
using chardev/ioctl allows you to give access to the hardwall functionality
to a specific group or user with file permissions, while the system call
would be less fine-grained, either associating access with a specific
capability (e.g. CAP_SYS_ADMIN) or giving it to all users.

> +#ifdef CONFIG_COMPAT
> +static long hardwall_compat_ioctl(struct file *file,
> +				  unsigned int a, unsigned long b)
> +{
> +	/* Sign-extend the argument so it can be used as a pointer. */
> +	return hardwall_ioctl(file, a, (int)(long)b);
> +}
> +#endif

Better use compat_ptr(b) instead of the manual cast.

	Arnd

Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Chris Metcalf]

On 6/26/2010 7:16 AM, Arnd Bergmann wrote:
> On Friday 25 June 2010, Chris Metcalf wrote:
>   
>> The original submission of this code to LKML had the driver
>> instantiated under /proc/tile/hardwall.  Now we just use a character
>> device for this, conventionally /dev/hardwall.  Some futures planning
>> for the TILE-Gx chip suggests that we may want to have other types of
>> devices that share the general model of "bind a task to a cpu, then
>> 'activate' a file descriptor on a pseudo-device that gives access to
>> some hardware resource".  As such, we are using a device rather
>> than, for example, a syscall, to set up and activate this code.
>>     
> The character device looks much better than the proc file. I still
> think a system call would be a more appropriate abstraction here,
> but the two are equivalent after all.
>   

I'm going to argue for a character device, though.  There are a few
reasons why I prefer it:

1. It allows a straightforward model for adding additional types of
"device" like this in the future.  It would be natural to support
multiple minor numbers, perhaps with different ones having different "is
the cpumask valid?" criteria.  Our next release will have two user
networks, and a notion of userspace IPIs, all of which fit the life
cycle of "create nonoverlapping cpumask, bind to cpu via affinity,
activate, deactivate".  You could imagine passing constants to the
syscall you propose, but it feels like it's just turning into a device
switch at that point.

2. You can easily imagine multiple classes of users, where different
classes are allowed to use different resources -- perhaps one user
network reserved for root, one for some privileged group, while the
userspace IPI network is available to anyone.  Or something different. 
We don't really know all the use cases yet, and having user/group
permissions built into the model seems like it may be helpful.

3. More subjectively -- the user dynamic networks have a "devicey" feel
to me.  Normal open-source code never has to deal with these things; you
only use them if your program is ready to open them up somehow, then
read and write data to them, which feels like a device kind of
abstraction (even though the reading and writing is via GPRs, not
iomem).  And the userspace IPI stuff will be done by mapping IOMEM into
process space and poking at it, once you've registered on the "userspace
IPI" device, so again it has a kind of devicey feel to it.

I don't think it would be strictly wrong to use a syscall, and you say
the two are interchangeable at a high level, but I think character
devices fit a bit better.

>> diff --git a/arch/tile/include/asm/processor.h b/arch/tile/include/asm/processor.h
>> index 96c50d2..95b54bc 100644
>> --- a/arch/tile/include/asm/processor.h
>> +++ b/arch/tile/include/asm/processor.h
>> @@ -74,6 +74,14 @@ struct async_tlb {
>>  	unsigned long address;   /* what address faulted? */
>>  };
>>  
>> +#ifdef CONFIG_HARDWALL
>> +struct hardwall_info;
>> +
>> +/* Can't use a normal list_head here due to header-file inclusion issues. */
>> +struct hardwall_list {
>> +	struct list_head *next, *prev;
>> +};
>> +#endif
>>     
> It seems strange that you need this. Why does linux/list.h
> depend on asm/processor.h?
>   

<linux/list.h> -> <linux/poison.h> -> <linux/prefetch.h> ->
<asm/processor.h>.  There doesn't seem to be any good way around this. 
I could, I suppose, use an opaque "struct list_head;" declaration in
<asm/processor.h>, then create one with kmalloc on demand, but that
seemed like overkill, so I embed the made-up version here, then validate
it as a BUILD_BUG_ON() to be the same size as a real list_head.  I never
actually use the "hardwall_list" structure directly.

> It certainly seems that the code would get simpler if this
> can be avoided.
>
>   
>> +/*
>> + * Guard changes to the hardwall data structures.
>> + * This could be finer grained (e.g. one lock for the list of hardwall
>> + * rectangles, then separate embedded locks for each one's list of tasks),
>> + * but there are subtle correctness issues when trying to start with
>> + * a task's "hardwall" pointer and lock the correct rectangle's embedded
>> + * lock in the presence of a simultaneous deactivation, so it seems
>> + * easier to have a single lock, given that none of these data
>> + * structures are touched very frequently during normal operation.
>> + */
>> +static DEFINE_SPINLOCK(hardwall_lock);
>>     
> I think instead of making the lock more fine-grained, a better optimization
> might be to use RCU to protect the updates. AFAICT, the updates to the
> structure are rare but you need to read it a lot.
>   

Actually, we neither read nor write the list very much.  We take the
lock when a new hardwall is created or a new process enables user
network access, but these are both "once per process lifetime" events
typically.  After that, the user process is just granted direct hardware
access when it is context switched in (based on a non-NULL hardwall_info
pointer) and that's it.  So the big spinlock approach is actually good
from a simplicity point of view.

>> +/*
>> + * Low-level primitives
>> + */
>> +
>> +/* Map a HARDWALL_FILE file to the matching hardwall info structure */
>> +#define _file_to_hardwall(file) ((file)->private_data)
>> +#define file_to_hardwall(file) \
>> +	((struct hardwall_info *)_file_to_hardwall(file))
>>     
> I wouldn't bother with these abstractions, you can simply write
> file->private_data in every place where they are used.
>   

Good point, fixed.  The abstractions were more useful when the file was
a seq_file and I had to work harder to get private data.

>> +#ifdef CONFIG_COMPAT
>> +static long hardwall_compat_ioctl(struct file *file,
>> +				  unsigned int a, unsigned long b)
>> +{
>> +	/* Sign-extend the argument so it can be used as a pointer. */
>> +	return hardwall_ioctl(file, a, (int)(long)b);
>> +}
>> +#endif
>>     
> Better use compat_ptr(b) instead of the manual cast.
>   

Yes, although hardwall_ioctl() wants an unsigned long, so it would have
to be "(unsigned long)compat_ptr(b)", so it's not obviously better
(certainly less succinct!).  But I concur that it's slightly less
obscure :-)

Thanks!

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Arnd Bergmann]

On Sunday 27 June 2010, Chris Metcalf wrote:
> On 6/26/2010 7:16 AM, Arnd Bergmann wrote:
> > On Friday 25 June 2010, Chris Metcalf wrote:
> > The character device looks much better than the proc file. I still
> > think a system call would be a more appropriate abstraction here,
> > but the two are equivalent after all.
> >   
> 
> I'm going to argue for a character device, though.  There are a few
> reasons why I prefer it:
> 
> 1. It allows a straightforward model for adding additional types of
> "device" like this in the future.  It would be natural to support
> multiple minor numbers, perhaps with different ones having different "is
> the cpumask valid?" criteria.  Our next release will have two user
> networks, and a notion of userspace IPIs, all of which fit the life
> cycle of "create nonoverlapping cpumask, bind to cpu via affinity,
> activate, deactivate".  You could imagine passing constants to the
> syscall you propose, but it feels like it's just turning into a device
> switch at that point.

Many system calls have flags for things like this, I don't think
this is a strong argument in favor of a device.

> 2. You can easily imagine multiple classes of users, where different
> classes are allowed to use different resources -- perhaps one user
> network reserved for root, one for some privileged group, while the
> userspace IPI network is available to anyone.  Or something different. 
> We don't really know all the use cases yet, and having user/group
> permissions built into the model seems like it may be helpful.

Ok.

When you get to this complexity though, it may be worthwhile to
integrate into the cgroups subsystem, which might already provide
some of this. Have you looked into this?

> 3. More subjectively -- the user dynamic networks have a "devicey" feel
> to me.  Normal open-source code never has to deal with these things; you
> only use them if your program is ready to open them up somehow, then
> read and write data to them, which feels like a device kind of
> abstraction (even though the reading and writing is via GPRs, not
> iomem).  And the userspace IPI stuff will be done by mapping IOMEM into
> process space and poking at it, once you've registered on the "userspace
> IPI" device, so again it has a kind of devicey feel to it.
>
> I don't think it would be strictly wrong to use a syscall, and you say
> the two are interchangeable at a high level, but I think character
> devices fit a bit better.

Fair enough. This is purely subjective, so while it feels like it
should be a syscall to me, you will have to maintain it in the end
and it sounds like you have thought this through more than I have.

I just wanted to make sure you didn't just use the character device
because it was easier to convert from the proc file than it would
have been to use a better alternative.

> > It seems strange that you need this. Why does linux/list.h
> > depend on asm/processor.h?
> >   
> 
> <linux/list.h> -> <linux/poison.h> -> <linux/prefetch.h> ->
> <asm/processor.h>.  There doesn't seem to be any good way around this. 
> I could, I suppose, use an opaque "struct list_head;" declaration in
> <asm/processor.h>, then create one with kmalloc on demand, but that
> seemed like overkill, so I embed the made-up version here, then validate
> it as a BUILD_BUG_ON() to be the same size as a real list_head.  I never
> actually use the "hardwall_list" structure directly.

We could break the dependency by turning prefetch_range into a macro
or an extern function. There is only one user, and it's in a staging
driver, so the impact would be minimal.

> >> +/*
> >> + * Guard changes to the hardwall data structures.
> >> + * This could be finer grained (e.g. one lock for the list of hardwall
> >> + * rectangles, then separate embedded locks for each one's list of tasks),
> >> + * but there are subtle correctness issues when trying to start with
> >> + * a task's "hardwall" pointer and lock the correct rectangle's embedded
> >> + * lock in the presence of a simultaneous deactivation, so it seems
> >> + * easier to have a single lock, given that none of these data
> >> + * structures are touched very frequently during normal operation.
> >> + */
> >> +static DEFINE_SPINLOCK(hardwall_lock);
> >>     
> > I think instead of making the lock more fine-grained, a better optimization
> > might be to use RCU to protect the updates. AFAICT, the updates to the
> > structure are rare but you need to read it a lot.
> >   
> 
> Actually, we neither read nor write the list very much.  We take the
> lock when a new hardwall is created or a new process enables user
> network access, but these are both "once per process lifetime" events
> typically.  After that, the user process is just granted direct hardware
> access when it is context switched in (based on a non-NULL hardwall_info
> pointer) and that's it.  So the big spinlock approach is actually good
> from a simplicity point of view.

Yes, if there is no contention, a spinlock is fine.

> >> +#ifdef CONFIG_COMPAT
> >> +static long hardwall_compat_ioctl(struct file *file,
> >> +				  unsigned int a, unsigned long b)
> >> +{
> >> +	/* Sign-extend the argument so it can be used as a pointer. */
> >> +	return hardwall_ioctl(file, a, (int)(long)b);
> >> +}
> >> +#endif
> >>     
> > Better use compat_ptr(b) instead of the manual cast.
> >   
> 
> Yes, although hardwall_ioctl() wants an unsigned long, so it would have
> to be "(unsigned long)compat_ptr(b)", so it's not obviously better
> (certainly less succinct!).  But I concur that it's slightly less
> obscure :-)

Most importantly, it's architecture independent. A lot of drivers do this
and I still think that we should eventually get a 

long generic_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
	if (file->f_ops->unlocked_ioctl)
		return file->f_ops->unlocked_ioctl(file, cmd,
					(unsigned long)compat_ptr(arg));
	return -ENOTTY;
}

Once we have killed off the last fops->ioctl() user, I'll try to
come up with a semantic patch to convert all drivers with a trivial
compat_ioctl function to this.

	Arnd

Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Chris Metcalf]

On 6/28/2010 7:12 AM, Arnd Bergmann wrote:
> On Sunday 27 June 2010, Chris Metcalf wrote:
>   
>> On 6/26/2010 7:16 AM, Arnd Bergmann wrote:
>>     
>>> It seems strange that you need this. Why does linux/list.h
>>> depend on asm/processor.h?
>>>       
>> <linux/list.h> -> <linux/poison.h> -> <linux/prefetch.h> ->
>> <asm/processor.h>.  There doesn't seem to be any good way around this. 
>> I could, I suppose, use an opaque "struct list_head;" declaration in
>> <asm/processor.h>, then create one with kmalloc on demand, but that
>> seemed like overkill, so I embed the made-up version here, then validate
>> it as a BUILD_BUG_ON() to be the same size as a real list_head.  I never
>> actually use the "hardwall_list" structure directly.
>>     
> We could break the dependency by turning prefetch_range into a macro
> or an extern function. There is only one user, and it's in a staging
> driver, so the impact would be minimal.
>   

I don't think so.  The problem is that users of <linux/list.h> expect to
be able to #include that one header, then use things like
list_for_each() (which uses prefetch, as defined in <asm/processor.h>),
but without also being required to #include <asm/processor.h> themselves
explicitly.

I think the only "true" fix would be to have a new <linux/list_types.h>
header that provides list_head (and presumably hlist_head and
hlist_node), which <linux/list.h> would include, as would our
<asm/processor.h>.  This is certainly in line with recent
header-separation changes (e.g. mm_types.h).  Would there be interest in
a change like this?  I implemented it in my tree, and if it sounds
plausible to you, I'll send out a git diff, but it looks pretty much
exactly like this description :-)

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Arnd Bergmann]

Cc'ing linux-arch on the question of how to use a list_head in
processor.h.

On Monday 28 June 2010 17:23:16 Chris Metcalf wrote:
> > We could break the dependency by turning prefetch_range into a macro
> > or an extern function. There is only one user, and it's in a staging
> > driver, so the impact would be minimal.
> >   
> 
> I don't think so.  The problem is that users of <linux/list.h> expect to
> be able to #include that one header, then use things like
> list_for_each() (which uses prefetch, as defined in <asm/processor.h>),
> but without also being required to #include <asm/processor.h> themselves
> explicitly.

Right.
 
> I think the only "true" fix would be to have a new <linux/list_types.h>
> header that provides list_head (and presumably hlist_head and
> hlist_node), which <linux/list.h> would include, as would our
> <asm/processor.h>.  This is certainly in line with recent
> header-separation changes (e.g. mm_types.h).  Would there be interest in
> a change like this?  I implemented it in my tree, and if it sounds
> plausible to you, I'll send out a git diff, but it looks pretty much
> exactly like this description :-)

Yes, I think that would be a reasonable change.

Another alternative might be to move the prefetch stuff from asm/processor.h
to asm/prefetch.h on all architectures, which also breaks the dependency loop,
unless I'm mistaken again.

	Arnd

Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Chris Metcalf]

[The question is how to use <linux/list.h> from within <asm/processor.h>.]

On 6/28/2010 3:34 PM, Arnd Bergmann wrote:
>> I think the only "true" fix would be to have a new <linux/list_types.h>
>> > header that provides list_head (and presumably hlist_head and
>> > hlist_node), which <linux/list.h> would include, as would our
>> > <asm/processor.h>.  This is certainly in line with recent
>> > header-separation changes (e.g. mm_types.h).  Would there be interest in
>> > a change like this?  I implemented it in my tree, and if it sounds
>> > plausible to you, I'll send out a git diff, but it looks pretty much
>> > exactly like this description :-)
>>     
> Yes, I think that would be a reasonable change.
>
> Another alternative might be to move the prefetch stuff from asm/processor.h
> to asm/prefetch.h on all architectures, which also breaks the dependency loop,
> unless I'm mistaken again

In principle I like the <asm/prefetch.h> idea, but I'm concerned that
the #include of <asm/system.h> from <linux/list.h> will recursively
include <asm/processor.h> on some platforms; for example, s390 and
xtensa include it directly.  We (tile) were including it indirectly via
<asm/irqflags.h>, though this seems to be a spurious include on our
part, but other platforms may also include it indirectly.  To be fair,
I'm not sure why <asm/system.h> is included from <linux/list.h>.  It
doesn't seem required for a tile build, at least, but no doubt it was
put there for some reason.

So, if there's a good reason for it to be there, I'd say that pushes us
back toward a separate <linux/list_types.h>.  Otherwise we can
investigate splitting out the prefetch content on every platform to
<asm/prefetch.h> (presumably creating some empty <asm/prefetch.h>
headers on architectures that just use the gcc builtin) and adding new
#includes of <asm/prefetch.h> to files that reference the prefetch
functionality.  Arnd and other list folks, what's your instinct?

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Chris Metcalf]

[The question is how to use <linux/list.h> from within <asm/processor.h>.]

On 6/28/2010 3:34 PM, Arnd Bergmann wrote:
>> I think the only "true" fix would be to have a new <linux/list_types.h>
>> > header that provides list_head (and presumably hlist_head and
>> > hlist_node), which <linux/list.h> would include, as would our
>> > <asm/processor.h>.  This is certainly in line with recent
>> > header-separation changes (e.g. mm_types.h).  Would there be interest in
>> > a change like this?  I implemented it in my tree, and if it sounds
>> > plausible to you, I'll send out a git diff, but it looks pretty much
>> > exactly like this description :-)
>>     
> Yes, I think that would be a reasonable change.
>
> Another alternative might be to move the prefetch stuff from asm/processor.h
> to asm/prefetch.h on all architectures, which also breaks the dependency loop,
> unless I'm mistaken again

In principle I like the <asm/prefetch.h> idea, but I'm concerned that
the #include of <asm/system.h> from <linux/list.h> will recursively
include <asm/processor.h> on some platforms; for example, s390 and
xtensa include it directly.  We (tile) were including it indirectly via
<asm/irqflags.h>, though this seems to be a spurious include on our
part, but other platforms may also include it indirectly.  To be fair,
I'm not sure why <asm/system.h> is included from <linux/list.h>.  It
doesn't seem required for a tile build, at least, but no doubt it was
put there for some reason.

So, if there's a good reason for it to be there, I'd say that pushes us
back toward a separate <linux/list_types.h>.  Otherwise we can
investigate splitting out the prefetch content on every platform to
<asm/prefetch.h> (presumably creating some empty <asm/prefetch.h>
headers on architectures that just use the gcc builtin) and adding new
#includes of <asm/prefetch.h> to files that reference the prefetch
functionality.  Arnd and other list folks, what's your instinct?

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Arnd Bergmann]

On Friday 02 July 2010, Chris Metcalf wrote:
> So, if there's a good reason for it to be there, I'd say that pushes us
> back toward a separate <linux/list_types.h>.  Otherwise we can
> investigate splitting out the prefetch content on every platform to
> <asm/prefetch.h> (presumably creating some empty <asm/prefetch.h>
> headers on architectures that just use the gcc builtin) and adding new
> #includes of <asm/prefetch.h> to files that reference the prefetch
> functionality.  Arnd and other list folks, what's your instinct?

Makes sense. Splitting out the list types from list.h does seem to be
safest option. We might actually be able to do some header file
untangling that way, by using list_types.h in all headers that
use a list_head by none of the macros and functions associated with it.

	Arnd

[PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Chris Metcalf]

This allows a list_head (or hlist_head, etc.) to be used from places
that used to be impractical, in particular <asm/processor.h>, which
used to cause include file recursion: <linux/list.h> includes
<linux/prefetch.h>, which always includes <asm/processor.h> for the
prefetch macros, as well as <asm/system.h>, which often includes
<asm/processor.h> directly or indirectly.

This avoids a lot of painful workaround hackery on the tile
architecture, where we use a list_head in the thread_struct to chain
together tasks that are activated on a particular hardwall.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---

 include/linux/list.h       |   13 +------------
 include/linux/list_types.h |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 12 deletions(-)
 create mode 100644 include/linux/list_types.h

diff --git a/include/linux/list.h b/include/linux/list.h
index 8392884..3a42003 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -4,6 +4,7 @@
 #include <linux/stddef.h>
 #include <linux/poison.h>
 #include <linux/prefetch.h>
+#include <linux/list_types.h>
 #include <asm/system.h>
 
 /*
@@ -16,10 +17,6 @@
  * using the generic single-entry routines.
  */
 
-struct list_head {
-	struct list_head *next, *prev;
-};
-
 #define LIST_HEAD_INIT(name) { &(name), &(name) }
 
 #define LIST_HEAD(name) \
@@ -551,14 +548,6 @@ static inline void list_splice_tail_init(struct list_head *list,
  * You lose the ability to access the tail in O(1).
  */
 
-struct hlist_head {
-	struct hlist_node *first;
-};
-
-struct hlist_node {
-	struct hlist_node *next, **pprev;
-};
-
 #define HLIST_HEAD_INIT { .first = NULL }
 #define HLIST_HEAD(name) struct hlist_head name = {  .first = NULL }
 #define INIT_HLIST_HEAD(ptr) ((ptr)->first = NULL)
diff --git a/include/linux/list_types.h b/include/linux/list_types.h
new file mode 100644
index 0000000..c991e28
--- /dev/null
+++ b/include/linux/list_types.h
@@ -0,0 +1,16 @@
+#ifndef _LINUX_LIST_TYPES_H
+#define _LINUX_LIST_TYPES_H
+
+struct list_head {
+	struct list_head *next, *prev;
+};
+
+struct hlist_head {
+	struct hlist_node *first;
+};
+
+struct hlist_node {
+	struct hlist_node *next, **pprev;
+};
+
+#endif
-- 
1.6.5.2

[PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Chris Metcalf]

This allows a list_head (or hlist_head, etc.) to be used from places
that used to be impractical, in particular <asm/processor.h>, which
used to cause include file recursion: <linux/list.h> includes
<linux/prefetch.h>, which always includes <asm/processor.h> for the
prefetch macros, as well as <asm/system.h>, which often includes
<asm/processor.h> directly or indirectly.

This avoids a lot of painful workaround hackery on the tile
architecture, where we use a list_head in the thread_struct to chain
together tasks that are activated on a particular hardwall.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---

 include/linux/list.h       |   13 +------------
 include/linux/list_types.h |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 12 deletions(-)
 create mode 100644 include/linux/list_types.h

diff --git a/include/linux/list.h b/include/linux/list.h
index 8392884..3a42003 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -4,6 +4,7 @@
 #include <linux/stddef.h>
 #include <linux/poison.h>
 #include <linux/prefetch.h>
+#include <linux/list_types.h>
 #include <asm/system.h>
 
 /*
@@ -16,10 +17,6 @@
  * using the generic single-entry routines.
  */
 
-struct list_head {
-	struct list_head *next, *prev;
-};
-
 #define LIST_HEAD_INIT(name) { &(name), &(name) }
 
 #define LIST_HEAD(name) \
@@ -551,14 +548,6 @@ static inline void list_splice_tail_init(struct list_head *list,
  * You lose the ability to access the tail in O(1).
  */
 
-struct hlist_head {
-	struct hlist_node *first;
-};
-
-struct hlist_node {
-	struct hlist_node *next, **pprev;
-};
-
 #define HLIST_HEAD_INIT { .first = NULL }
 #define HLIST_HEAD(name) struct hlist_head name = {  .first = NULL }
 #define INIT_HLIST_HEAD(ptr) ((ptr)->first = NULL)
diff --git a/include/linux/list_types.h b/include/linux/list_types.h
new file mode 100644
index 0000000..c991e28
--- /dev/null
+++ b/include/linux/list_types.h
@@ -0,0 +1,16 @@
+#ifndef _LINUX_LIST_TYPES_H
+#define _LINUX_LIST_TYPES_H
+
+struct list_head {
+	struct list_head *next, *prev;
+};
+
+struct hlist_head {
+	struct hlist_node *first;
+};
+
+struct hlist_node {
+	struct hlist_node *next, **pprev;
+};
+
+#endif
-- 
1.6.5.2

Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Chris Metcalf]

On 7/2/2010 12:11 PM, Arnd Bergmann wrote:
> On Friday 02 July 2010, Chris Metcalf wrote:
>   
>> So, if there's a good reason for it to be there, I'd say that pushes us
>> back toward a separate <linux/list_types.h>.  Otherwise we can
>> investigate splitting out the prefetch content on every platform to
>> <asm/prefetch.h> (presumably creating some empty <asm/prefetch.h>
>> headers on architectures that just use the gcc builtin) and adding new
>> #includes of <asm/prefetch.h> to files that reference the prefetch
>> functionality.  Arnd and other list folks, what's your instinct?
>>     
> Makes sense. Splitting out the list types from list.h does seem to be
> safest option. We might actually be able to do some header file
> untangling that way, by using list_types.h in all headers that
> use a list_head by none of the macros and functions associated with it.
>   

For now I'll just stick with the straight splitting-out (see recent git
email).  There may be kernel code that is getting the list macros and
functions "by accident" by including some header that in itself only
needs the structs and so could use <linux/list_types.h>, and would need
to #include <linux/list.h> itself to avoid breaking.  There would
probably be a long tail of complaints that developers' obscure
architecture, driver, etc., had been broken by an aggressive
"untangling" change.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: Add driver to enable access to the user dynamic network.

[Chris Metcalf]

On 7/2/2010 12:11 PM, Arnd Bergmann wrote:
> On Friday 02 July 2010, Chris Metcalf wrote:
>   
>> So, if there's a good reason for it to be there, I'd say that pushes us
>> back toward a separate <linux/list_types.h>.  Otherwise we can
>> investigate splitting out the prefetch content on every platform to
>> <asm/prefetch.h> (presumably creating some empty <asm/prefetch.h>
>> headers on architectures that just use the gcc builtin) and adding new
>> #includes of <asm/prefetch.h> to files that reference the prefetch
>> functionality.  Arnd and other list folks, what's your instinct?
>>     
> Makes sense. Splitting out the list types from list.h does seem to be
> safest option. We might actually be able to do some header file
> untangling that way, by using list_types.h in all headers that
> use a list_head by none of the macros and functions associated with it.
>   

For now I'll just stick with the straight splitting-out (see recent git
email).  There may be kernel code that is getting the list macros and
functions "by accident" by including some header that in itself only
needs the structs and so could use <linux/list_types.h>, and would need
to #include <linux/list.h> itself to avoid breaking.  There would
probably be a long tail of complaints that developers' obscure
architecture, driver, etc., had been broken by an aggressive
"untangling" change.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Matthew Wilcox]

On Fri, Jul 02, 2010 at 01:41:14PM -0400, Chris Metcalf wrote:
> This allows a list_head (or hlist_head, etc.) to be used from places
> that used to be impractical, in particular <asm/processor.h>, which
> used to cause include file recursion: <linux/list.h> includes
> <linux/prefetch.h>, which always includes <asm/processor.h> for the
> prefetch macros, as well as <asm/system.h>, which often includes
> <asm/processor.h> directly or indirectly.

Why a new header file instead of linux/types.h?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Chris Metcalf]

On 7/2/2010 3:19 PM, Matthew Wilcox wrote:
> On Fri, Jul 02, 2010 at 01:41:14PM -0400, Chris Metcalf wrote:
>   
>> This allows a list_head (or hlist_head, etc.) to be used from places
>> that used to be impractical, in particular <asm/processor.h>, which
>> used to cause include file recursion: <linux/list.h> includes
>> <linux/prefetch.h>, which always includes <asm/processor.h> for the
>> prefetch macros, as well as <asm/system.h>, which often includes
>> <asm/processor.h> directly or indirectly.
>>     
> Why a new header file instead of linux/types.h?
>   

I was working from analogy to kvm_types.h, mm_types.h, rwlock_types.h,
spinlock_types.h.  My impression is that linux/types.h is generally for
basic (non-struct) types, with atomic_t/atomic64_t being added as
"almost non-struct types", and of course the historical exception of
"struct ustat", which has been there since the dawn of time (0.97 anyway).

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Chris Metcalf]

On 7/2/2010 3:19 PM, Matthew Wilcox wrote:
> On Fri, Jul 02, 2010 at 01:41:14PM -0400, Chris Metcalf wrote:
>   
>> This allows a list_head (or hlist_head, etc.) to be used from places
>> that used to be impractical, in particular <asm/processor.h>, which
>> used to cause include file recursion: <linux/list.h> includes
>> <linux/prefetch.h>, which always includes <asm/processor.h> for the
>> prefetch macros, as well as <asm/system.h>, which often includes
>> <asm/processor.h> directly or indirectly.
>>     
> Why a new header file instead of linux/types.h?
>   

I was working from analogy to kvm_types.h, mm_types.h, rwlock_types.h,
spinlock_types.h.  My impression is that linux/types.h is generally for
basic (non-struct) types, with atomic_t/atomic64_t being added as
"almost non-struct types", and of course the historical exception of
"struct ustat", which has been there since the dawn of time (0.97 anyway).

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Arnd Bergmann]

On Friday 02 July 2010 21:19:11 Matthew Wilcox wrote:
> Why a new header file instead of linux/types.h?

I think it mostly makes sense because a list_head by itself usually
isn't all that useful, you also want the list_add/list_for_each/...
macros, so you end up including linux/list.h anyway.

linux/list_types.h is really a special case which can get included
by other headers when they have a reason for doing that.

	Arnd

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Matthew Wilcox]

On Fri, Jul 02, 2010 at 03:33:52PM -0400, Chris Metcalf wrote:
> On 7/2/2010 3:19 PM, Matthew Wilcox wrote:
> > Why a new header file instead of linux/types.h?
> 
> I was working from analogy to kvm_types.h, mm_types.h, rwlock_types.h,
> spinlock_types.h.  My impression is that linux/types.h is generally for
> basic (non-struct) types, with atomic_t/atomic64_t being added as
> "almost non-struct types", and of course the historical exception of
> "struct ustat", which has been there since the dawn of time (0.97 anyway).

I think list_head, hlist_head and hlist_node qualify as "almost non-struct
types", don't you?  :-)

I wouldn't mind seeing kvm_types.h, rwlock_types.h and spinlock_types.h
merged into types.h, personally.  They're all pretty fundamental kernel
kind of types.  It's a matter of taste, and I'm not particularly fussed
one way or the other.

mm_types.h is complex and full of mm-specific information, so keeping
it separate makes sense to me.

I just object to the unnecessary creation of tiny files like this.
Which is how we ended up with atomic_t and atomic64_t in there in the
first place :-)

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Chris Metcalf]

On 7/2/2010 4:48 PM, Matthew Wilcox wrote:
> On Fri, Jul 02, 2010 at 03:33:52PM -0400, Chris Metcalf wrote:
>   
>> On 7/2/2010 3:19 PM, Matthew Wilcox wrote:
>>     
>>> Why a new header file instead of linux/types.h?
>>>       
>> I was working from analogy to kvm_types.h, mm_types.h, rwlock_types.h,
>> spinlock_types.h.  My impression is that linux/types.h is generally for
>> basic (non-struct) types, with atomic_t/atomic64_t being added as
>> "almost non-struct types", and of course the historical exception of
>> "struct ustat", which has been there since the dawn of time (0.97 anyway).
>>     
> I think list_head, hlist_head and hlist_node qualify as "almost non-struct
> types", don't you?  :-)
>   

I see the smiley, but to reply seriously, the distinction I was making
was that atomic_t is really just an integer type, but with typing magic
to protect it from implicit conversion -- unlike list_head, which really
is a more complex type.

I suppose one could make a kind of "intent of the founders"
constitutional law-type argument suggesting that the presence of "struct
ustat" suggests more complex types are in fact appropriate in
<linux/types.h>.  :-)

> I wouldn't mind seeing kvm_types.h, rwlock_types.h and spinlock_types.h
> merged into types.h, personally.  They're all pretty fundamental kernel
> kind of types.  It's a matter of taste, and I'm not particularly fussed
> one way or the other.
>   

Somehow it's hard to see kvm_ioapic_redirect_entry on a par with size_t :-)

> I just object to the unnecessary creation of tiny files like this.
> Which is how we ended up with atomic_t and atomic64_t in there in the
> first place :-)
>   

In any case, I think this either way is plausible, but in the absence of
more folks weighing in, I think "avoid adding a complex type to
<linux/types.h>" sounds more convincing to me than "avoid adding a new
tiny file", though I certainly do buy the latter argument.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Chris Metcalf]

On 7/2/2010 4:48 PM, Matthew Wilcox wrote:
> On Fri, Jul 02, 2010 at 03:33:52PM -0400, Chris Metcalf wrote:
>   
>> On 7/2/2010 3:19 PM, Matthew Wilcox wrote:
>>     
>>> Why a new header file instead of linux/types.h?
>>>       
>> I was working from analogy to kvm_types.h, mm_types.h, rwlock_types.h,
>> spinlock_types.h.  My impression is that linux/types.h is generally for
>> basic (non-struct) types, with atomic_t/atomic64_t being added as
>> "almost non-struct types", and of course the historical exception of
>> "struct ustat", which has been there since the dawn of time (0.97 anyway).
>>     
> I think list_head, hlist_head and hlist_node qualify as "almost non-struct
> types", don't you?  :-)
>   

I see the smiley, but to reply seriously, the distinction I was making
was that atomic_t is really just an integer type, but with typing magic
to protect it from implicit conversion -- unlike list_head, which really
is a more complex type.

I suppose one could make a kind of "intent of the founders"
constitutional law-type argument suggesting that the presence of "struct
ustat" suggests more complex types are in fact appropriate in
<linux/types.h>.  :-)

> I wouldn't mind seeing kvm_types.h, rwlock_types.h and spinlock_types.h
> merged into types.h, personally.  They're all pretty fundamental kernel
> kind of types.  It's a matter of taste, and I'm not particularly fussed
> one way or the other.
>   

Somehow it's hard to see kvm_ioapic_redirect_entry on a par with size_t :-)

> I just object to the unnecessary creation of tiny files like this.
> Which is how we ended up with atomic_t and atomic64_t in there in the
> first place :-)
>   

In any case, I think this either way is plausible, but in the absence of
more folks weighing in, I think "avoid adding a complex type to
<linux/types.h>" sounds more convincing to me than "avoid adding a new
tiny file", though I certainly do buy the latter argument.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Christoph Hellwig]

On Fri, Jul 02, 2010 at 10:43:13PM +0200, Arnd Bergmann wrote:
> On Friday 02 July 2010 21:19:11 Matthew Wilcox wrote:
> > Why a new header file instead of linux/types.h?
> 
> I think it mostly makes sense because a list_head by itself usually
> isn't all that useful, you also want the list_add/list_for_each/...
> macros, so you end up including linux/list.h anyway.

It's useful for headers.  You can assume linux/types.h is already
included and don't have to bother to include list{_types}.h everywhere.

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Alexey Dobriyan]

On Fri, Jul 02, 2010 at 02:48:17PM -0600, Matthew Wilcox wrote:
> On Fri, Jul 02, 2010 at 03:33:52PM -0400, Chris Metcalf wrote:
> > On 7/2/2010 3:19 PM, Matthew Wilcox wrote:
> > > Why a new header file instead of linux/types.h?
> > 
> > I was working from analogy to kvm_types.h, mm_types.h, rwlock_types.h,
> > spinlock_types.h.  My impression is that linux/types.h is generally for
> > basic (non-struct) types, with atomic_t/atomic64_t being added as
> > "almost non-struct types", and of course the historical exception of
> > "struct ustat", which has been there since the dawn of time (0.97 anyway).
> 
> I think list_head, hlist_head and hlist_node qualify as "almost non-struct
> types", don't you?  :-)
> 
> I wouldn't mind seeing kvm_types.h, rwlock_types.h

> and spinlock_types.h

*cough*

You may want to run spinlock_types.h through preprocessor and see how
much garbage it will produce.

> merged into types.h, personally.  They're all pretty fundamental kernel
> kind of types.

Also we care about compilation speed.

> It's a matter of taste, and I'm not particularly fussed one way or the other.
> 
> mm_types.h is complex and full of mm-specific information, so keeping
> it separate makes sense to me.
> 
> I just object to the unnecessary creation of tiny files like this.

Me too. Also jumping over one file to understand what's going on is
better than jumping over multiple files.

> Which is how we ended up with atomic_t and atomic64_t in there in the
> first place :-)

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Arnd Bergmann]

On Friday 02 July 2010 22:48:17 Matthew Wilcox wrote:
> I wouldn't mind seeing kvm_types.h, rwlock_types.h and spinlock_types.h
> merged into types.h, personally.  They're all pretty fundamental kernel
> kind of types.  It's a matter of taste, and I'm not particularly fussed
> one way or the other.
> 
> mm_types.h is complex and full of mm-specific information, so keeping
> it separate makes sense to me.
> 
> I just object to the unnecessary creation of tiny files like this.
> Which is how we ended up with atomic_t and atomic64_t in there in the
> first place :-)

Ah, I didn't notice you had moved the atomic types in there. I agree that
the list types are in the same general category and it makes sense
to treat them the same way.

For rwlock_types.h and spinlock_types.h, I think including them in types.h
would really cause too much other crap to be pulled in through lockdep
and other things we might need in there in the future, which would in turn
cause the same problems with types.h that Chris is trying to avoid
in the first place by moving stuff out of list.h.

	Arnd

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Chris Metcalf]

On 7/3/2010 5:00 AM, Arnd Bergmann wrote:
> On Friday 02 July 2010 22:48:17 Matthew Wilcox wrote:
>   
>> I wouldn't mind seeing kvm_types.h, rwlock_types.h and spinlock_types.h
>> merged into types.h, personally.  They're all pretty fundamental kernel
>> kind of types.  It's a matter of taste, and I'm not particularly fussed
>> one way or the other.
>>
>> mm_types.h is complex and full of mm-specific information, so keeping
>> it separate makes sense to me.
>>
>> I just object to the unnecessary creation of tiny files like this.
>> Which is how we ended up with atomic_t and atomic64_t in there in the
>> first place :-)
>>     
> Ah, I didn't notice you had moved the atomic types in there. I agree that
> the list types are in the same general category and it makes sense
> to treat them the same way.
>
> For rwlock_types.h and spinlock_types.h, I think including them in types.h
> would really cause too much other crap to be pulled in through lockdep
> and other things we might need in there in the future, which would in turn
> cause the same problems with types.h that Chris is trying to avoid
> in the first place by moving stuff out of list.h.
>   

Sounds like we have a consensus on moving the list_head, hlist_head, and
hlist_node types to <linux/types.h>.  I assume everyone is agreed that
initializers, etc., should stay in <linux/list.h>.  I will send out a
revised git patch on Sunday.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Chris Metcalf]

On 7/3/2010 5:00 AM, Arnd Bergmann wrote:
> On Friday 02 July 2010 22:48:17 Matthew Wilcox wrote:
>   
>> I wouldn't mind seeing kvm_types.h, rwlock_types.h and spinlock_types.h
>> merged into types.h, personally.  They're all pretty fundamental kernel
>> kind of types.  It's a matter of taste, and I'm not particularly fussed
>> one way or the other.
>>
>> mm_types.h is complex and full of mm-specific information, so keeping
>> it separate makes sense to me.
>>
>> I just object to the unnecessary creation of tiny files like this.
>> Which is how we ended up with atomic_t and atomic64_t in there in the
>> first place :-)
>>     
> Ah, I didn't notice you had moved the atomic types in there. I agree that
> the list types are in the same general category and it makes sense
> to treat them the same way.
>
> For rwlock_types.h and spinlock_types.h, I think including them in types.h
> would really cause too much other crap to be pulled in through lockdep
> and other things we might need in there in the future, which would in turn
> cause the same problems with types.h that Chris is trying to avoid
> in the first place by moving stuff out of list.h.
>   

Sounds like we have a consensus on moving the list_head, hlist_head, and
hlist_node types to <linux/types.h>.  I assume everyone is agreed that
initializers, etc., should stay in <linux/list.h>.  I will send out a
revised git patch on Sunday.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] Break out types from <linux/list.h> to <linux/list_types.h>.

[Matthew Wilcox]

On Sat, Jul 03, 2010 at 09:47:58PM -0400, Chris Metcalf wrote:
> Sounds like we have a consensus on moving the list_head, hlist_head, and
> hlist_node types to <linux/types.h>.  I assume everyone is agreed that
> initializers, etc., should stay in <linux/list.h>.  I will send out a
> revised git patch on Sunday.

Yes, I think that's reasonable.  By way of comparison, ATOMIC_INIT,
etc. stayed in atomic.h.  It makes sense -- you only need the types
for structure definitions, you don't need the initialisers.

I suppose if you have a header which defines an initialiser like sched.h
does, you'll want the list.h initialisers, but anything that complex
should be able to handle including the full list.h header.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."