[PATCH 3/4] SGI Altix cross partition functionality

[PATCH 3/4] SGI Altix cross partition functionality

[Dean Nelson]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

This patch contains the communication module (XPC) for cross partition
communication on a partitioned SGI Altix.
                                                                                
XPC creates kthreads to manage the channel activity between partitions.
There is an EFI version of XPC (called XPBOOT) that polls rather than uses
multiple kthreads. It's purpose is to provide a diskless boot capability.
It is the reason for the '#if[n]def SN_PROM' code and xpc_stubs.h file.
XPBOOT is included in the PROM/SAL code and can not be released due to
Intel copyright.

[-- Attachment #2: patch-3.gz --]
[-- Type: application/x-gunzip, Size: 46798 bytes --]

Re: [PATCH 3/4] SGI Altix cross partition functionality

[Christoph Hellwig]

On Thu, Jun 17, 2004 at 01:36:38PM -0500, Dean Nelson wrote:
> This patch contains the communication module (XPC) for cross partition
> communication on a partitioned SGI Altix.
>                                                                                 
> XPC creates kthreads to manage the channel activity between partitions.
> There is an EFI version of XPC (called XPBOOT) that polls rather than uses
> multiple kthreads. It's purpose is to provide a diskless boot capability.
> It is the reason for the '#if[n]def SN_PROM' code and xpc_stubs.h file.
> XPBOOT is included in the PROM/SAL code and can not be released due to
> Intel copyright.

Please don't include all that mess.  If you want to use it in two enviroments
and do the license assignment mess that's your choice but please keep the
compat gunk out of the actual kernel files.


+obj-$(CONFIG_IA64_SGI_SN_XPC)	+= xpc.o
+xpc-y				:= xpc_main.o xpc_kdb.o xpc_channel.o xpc_partition.o
+CFLAGS_xpc_main.o += -I $(TOPDIR)/arch/$(ARCH)/kdb
+CFLAGS_xpc_kdb.o += -I $(TOPDIR)/arch/$(ARCH)/kdb
+CFLAGS_xpc_channel.o += -I $(TOPDIR)/arch/$(ARCH)/kdb
+CFLAGS_xpc_partition.o += -I $(TOPDIR)/arch/$(ARCH)/kdb

We already told you not to do this yesterday.  Als please make all that
xp* stuff into a single module instead of three and kill all the useless
layering.

+#define __XPC_MAIN__

Where is that checked?  And even if it is somewhere that's no the kind
of code we want.

+
+#ifndef	SN_PROM
+#include <asm/pgtable.h>
+#include <linux/autoconf.h>
+#include <linux/interrupt.h>
+#include <asm/sn/clksupport.h>
+#include <asm/sn/fetchop.h>
+#include <linux/sysctl.h>
+#include <asm/processor.h>
+#include <asm/sn/xp.h>
+#include "xpc_dbgtk.h"

<asm/*.h> after <linux/*.h> please, and never ever include autoconf.h
directly.

+// >>> Where does the following two macros belong?
+/*
+ * Define a macro that does exactly what wait_event_interruptible() does except
+ * that it adds the current task to the wait queue as an exclusive task (i.e.,
+ * sets the WQ_FLAG_EXCLUSIVE flag) rather than as a non-exclusive task as
+ * wait_event_interruptible() does. (Linux doesn't seem to provide this option
+ * so I've written my own.)
+ */
+#define __wait_event_exclusive_interruptible(wq, condition, ret)	\

please submit a patch to add this to <linux/wait.h> instead.

+} xpc_vars_t;

please kill all the typedef gunk.

+#define XPC_MSGQUEUE_REF(_ch)	atomic_inc(&(_ch)->references)
+#define XPC_MSGQUEUE_DEREF(_ch)						\
+	{								\
+		xpc_partition_t *_p;					\
+		s32 _refs;						\
+		_refs = atomic_dec_return(&(_ch)->references);		\
+		XP_ASSERT(_refs >= 0);					\
+		if (_refs = 0) {					\
+			_p = &xpc_partitions[(_ch)->partid];		\
+			xpc_wakeup_channel_mgr(_p);			\
+		}							\
+	}

yikes.  For monster-macros like that please use inline function instead.

+	/*
+	 * Zero out MOST of the entry for this partition. Only the fields
+	 * following `references' will be zeroed. The others must remain
+	 * `viable' across partition ups and downs, since they may be
+	 * referenced during this memset() operation.
+	 */
+	memset((u8 *)&part->references + sizeof(part->references), 0,

cast to u8 * in memset? Ummm..

+//>>> #define DBGTK_USE_DPRINTK
+#include <asm/sn/xp_dbgtk.h>

Can't find that in my tree.  And if it's the good old SGI dbgtk stuff
it's totally crap and you're better of stopping to use it.

+#define __KERNEL_SYSCALLS__		/* needed for waitpid() */
+#include <asm/unistd.h>			/* needed for waitpid() */

which you shouldn't use..

+#include "xpc.h"
+

+/* once Linux 2.4 is no longer supported, eliminate these gyrations */
+
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,0) && \
+    LINUX_VERSION_CODE <  KERNEL_VERSION(2,5,0)
+
+#define IRQ_HANDLED
+
+#define THREAD_INFO  task_struct
+#define CURRENT_THREAD_INFO()  current
+
+#define NASID_SLICE_TO_CPUID_ERROR  smp_num_cpus
+
+#define sys_sched_setscheduler sched_setscheduler
+static inline _syscall3(long, sched_setscheduler, pid_t, pid,
+			int, policy, struct sched_param *, param);
+
+#define DAEMONIZE(name...)  { daemonize(); sprintf(current->comm, name); }
+#define DEQUEUE_SIGNAL(task, mask, info)  dequeue_signal(mask, info)
+
+#define cpumask_of_cpu(cpu) \
+	({ \
+		cpumask_t __cpu_mask = CPU_MASK_NONE; \
+		__set_bit(cpu, &__cpu_mask); \
+		&__cpu_mask; \
+	})

there's not cpumask_t in 2.4.  But anyway, all this 2.4 compat gunk doesn't
belong here.

+#include <linux/syscalls.h>
+
+#define EXPORT_NO_SYMBOLS

Not needed in 2.6.

+
+#define THREAD_INFO  thread_info
+#define CURRENT_THREAD_INFO  current_thread_info
+
+#define NASID_SLICE_TO_CPUID_ERROR  NR_CPUS
+
+#define DAEMONIZE(name...)  daemonize(name)
+#define DEQUEUE_SIGNAL(task, mask, info)  dequeue_signal(task, mask, info)

Please call all this stuff directly.

+volatile static unsigned long xpc_hb_check_timeout;

please use a proper atomic datatype for this.

+	DAEMONIZE(XPC_HB_CHECK_THREAD_NAME);
+
+	set_user_nice(current, 19);
+
+	spin_lock_irq(&current->sighand->siglock);
+	/*
+	 * The following allows SIGCHLD to be delivered.  Without it,
+	 * SIGCHLD gets dropped on the floor.
+	 */
+	current->sighand->action[SIGCHLD-1].sa.sa_handler = SIG_IGN;
+	siginitsetinv(&current->blocked, sigmask(SIGCHLD) | sigmask(SIGHUP));
+	recalc_sigpending();
+	spin_unlock_irq(&current->sighand->siglock);

I think you want allow_signal(), and please drop that renicing stuff.

+			spin_lock_irq(&current->sighand->siglock);
+			sig = DEQUEUE_SIGNAL(current, &current->blocked,
+								&sig_info);
+			spin_unlock_irq(&current->sighand->siglock);

there's a nice little dequeue_signal_lock() wrapper for you.  And with that
you can even get it to compile with 2.4 more easily without all the shouting
macro junk.  (And even your 2.4 version won't even compile because in 2.4
the lock is somewhere else)

+ *
+ * There seems to be some inconsistency in the meaning and usage of
+ * the word 'slice'.  Some places equate slice with CPU, others seem to
+ * equate it with the Front Side Bus slot. There are two CPUs (numbered 0 & 1)
+ * and two FSB slots (numbered 0 & 2) per nasid. The function
+ * nasid_slice_to_cpuid() equates slice to the FSB slot.
+ */
+static int
+xpc_next_cpu(void)
+{
+	static int last_cnodeid = 0;
+	static int last_slice = 2;
+	int next_cpu;
+	nasid_t nasid;
+
+
+	next_cpu = -1;
+	while (next_cpu = -1) {
+		if (++last_cnodeid >= numnodes) {
+			last_cnodeid = 0;
+			last_slice = (last_slice + 2) % 4;

locking?  Also such an iterator is probably better placed in core SN2
code and export instead of numnodes.

+	spin_lock_irqsave(&part->act_lock, irq_flags);
+
+	pid = kernel_thread(xpc_activating, (void *) ((u64) partid), SIGCHLD);

What about using the kthread infrastructure?  With an emulation of that
on 2.4 you could get rid of all your messy version abstractions.

+MODULE_PARM(xpc_hb_interval, "1i");

please use module_param.

+/* XPC is exiting flag */
+volatile int xpc_exiting = 0;

volatile again.  Please use proper primitives.

+
+
+/* SH_IPI_ACCESS shub register value on startup */
+static u64 xpc_sh_ipi_access = 0;

no need to initialize static/global variales to 0.

Re: [PATCH 3/4] SGI Altix cross partition functionality

[Dean Nelson]

On Thu, Jun 17, 2004 at 08:22:29PM +0100, Christoph Hellwig wrote:
> We already told you not to do this yesterday.  Als please make all that
> 

This [PATCH 3/4] was sent as one text file yesterday along with the others.
It never made it because of its size. So I resent it today as a gzip'd
attachment. I didn't mean for it to represent a response to any of
the comments I've received at this point.

At the moment I'm digesting the feedback and figuring out how to make
the necessary changes required to get this accepted by the community.

Thanks,
Dean

Re: [PATCH 3/4] SGI Altix cross partition functionality

[Dean Nelson]

On Thu, Jun 17, 2004 at 08:22:29PM +0100, Christoph Hellwig wrote:
> 
> +} xpc_vars_t;
> 
> please kill all the typedef gunk.

I'm in the process of ditching all of my typedef's. But before I axed the
following two I thought I'd ask if an exception should be made in their
case? Or are all typedef's deemed offlimits without exception?

	typedef void (*xpc_channel_func_t)(enum xpc_retval, partid_t, int,
						void *, void *);

	typedef void (*xpc_notify_func_t)(enum xpc_retval, partid_t, int,
						void *);

(Note: I had nothing to do with the partid_t typedef.)

Thanks,
Dean

Re: [PATCH 3/4] SGI Altix cross partition functionality

[Christoph Hellwig]

On Tue, Jun 22, 2004 at 01:17:07PM -0500, Dean Nelson wrote:
> On Thu, Jun 17, 2004 at 08:22:29PM +0100, Christoph Hellwig wrote:
> > 
> > +} xpc_vars_t;
> > 
> > please kill all the typedef gunk.
> 
> I'm in the process of ditching all of my typedef's. But before I axed the
> following two I thought I'd ask if an exception should be made in their
> case? Or are all typedef's deemed offlimits without exception?
> 
> 	typedef void (*xpc_channel_func_t)(enum xpc_retval, partid_t, int,
> 						void *, void *);
> 
> 	typedef void (*xpc_notify_func_t)(enum xpc_retval, partid_t, int,
> 						void *);
> 
> (Note: I had nothing to do with the partid_t typedef.)

Thos are okay.  In the kernel we often don't use _t postfixes for those
either, but it doesn't really matter.

Re: [PATCH 3/4] SGI Altix cross partition functionality

[Dean Nelson]

On Thu, Jun 17, 2004 at 08:22:29PM +0100, Christoph Hellwig wrote:
> +	/*
> +	 * Zero out MOST of the entry for this partition. Only the fields
> +	 * following `references' will be zeroed. The others must remain
> +	 * `viable' across partition ups and downs, since they may be
> +	 * referenced during this memset() operation.
> +	 */
> +	memset((u8 *)&part->references + sizeof(part->references), 0,
> 
> cast to u8 * in memset? Ummm..

I'm not exactly sure what you're objecting to here. The cast is necessary for
a correct result. The following two lines do not give the same result (the
'references' field is an atomic_t):
 
        (u8 *)&part->references + sizeof(part->references)
        &part->references + sizeof(part->references)
 
So is it the use of 'u8' instead of 'unsigned char' or 'char' that you object
to? Or is it the complexity of the args to memset? The entire memset call was
originally as follows:
 
        memset((u8 *)&part->references + sizeof(part->references), 0,
                (u64)(part + 1) - (u64)((u8 *)&part->references +
                                                sizeof(part->references)));
 
Would you prefer it to be something like?
 
        u8 *addr;
        size_t nbytes;
 
 
        addr = (u8 *) &part->references + sizeof(part->references);
        nbytes = (size_t) (part + 1) - (size_t) addr;
        memset(addr, 0, nbytes);
 
Or is it something else that you object to?
 
Thanks,
Dean

Re: [PATCH 3/4] SGI Altix cross partition functionality

[Christoph Hellwig]

On Wed, Jul 07, 2004 at 02:40:17PM -0500, Dean Nelson wrote:
> > +	memset((u8 *)&part->references + sizeof(part->references), 0,
> > 
> > cast to u8 * in memset? Ummm..
> 
> I'm not exactly sure what you're objecting to here. The cast is necessary for
> a correct result. The following two lines do not give the same result (the
> 'references' field is an atomic_t):

Actually I was misreading the code to see a simple case to (u8 *) of the
variable, but the actual code looks even more fishy now that I look in-depth
;-)

What about the readable variant:

memset(&part->nchannels, 0,
       sizeof(xpc_partition_t) - offsetoff(xpc_partition_t, nchannels));