[PULL] typesafe callbacks for kthread and stop_machine

[PULL] typesafe callbacks for kthread and stop_machine

[Rusty Russell]

Just the two places I look after.  And this time the conglomerate patch is
included below for more random commentry.

The following changes since commit 94ad374a0751f40d25e22e036c37f7263569d24c:
  Linus Torvalds (1):
        Fix off-by-one error in iov_iter_advance()

are available in the git repository at:

  ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.git master

Rusty Russell (4):
      cast_if_type: allow macros functions which take more than one type.
      typesafe_cb: wrappers for typesafe callbacks.
      typesafe: kthread_create and kthread_run
      typesafe: stop_machine

 include/linux/compiler-gcc.h   |   18 ++++++++++++++++++
 include/linux/compiler-intel.h |    2 ++
 include/linux/kernel.h         |   35 +++++++++++++++++++++++++++++++++++
 include/linux/kthread.h        |   28 +++++++++++++++++++++++++---
 include/linux/stop_machine.h   |   13 ++++++++-----
 kernel/kthread.c               |   29 +++++------------------------
 kernel/stop_machine.c          |    4 ++--
 7 files changed, 95 insertions(+), 34 deletions(-)


diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 5c8351b..3cdd22e 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -61,3 +61,21 @@
 #define  noinline			__attribute__((noinline))
 #define __attribute_const__		__attribute__((__const__))
 #define __maybe_unused			__attribute__((unused))
+
+/**
+ * cast_if_type - allow an alternate type
+ * @expr: the expression to optionally cast
+ * @oktype: the type to allow.
+ * @desttype: the type to cast to.
+ *
+ * This is used to accept a particular alternate type for an expression:
+ * because any other types will not be cast, they will cause a warning as
+ * normal.
+ *
+ * Note that the unnecessary trinary forces functions to devolve into
+ * function pointers as users expect, but means @expr must be a pointer or
+ * integer.
+ */
+#define cast_if_type(expr, oktype, desttype)	__builtin_choose_expr(	\
+	__builtin_types_compatible_p(typeof(1?(expr):(expr)), oktype),	\
+	(desttype)(expr), (expr))
diff --git a/include/linux/compiler-intel.h b/include/linux/compiler-intel.h
index d8e636e..7e704e6 100644
--- a/include/linux/compiler-intel.h
+++ b/include/linux/compiler-intel.h
@@ -29,3 +29,5 @@
 #endif
 
 #define uninitialized_var(x) x
+
+#define cast_if_type(expr, oktype, desttype) ((desttype)(expr))
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index aaa998f..8ea3c49 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -486,4 +486,39 @@ struct sysinfo {
 #define NUMA_BUILD 0
 #endif
 
+/* If fn is of type ok1 or ok2, cast to desttype */
+#define __typesafe_cb(desttype, fn, ok1, ok2) \
+	cast_if_type(cast_if_type((fn), ok1, desttype), ok2, desttype)
+
+/**
+ * typesafe_cb - cast a callback function if it matches the arg
+ * @rettype: the return type of the callback function
+ * @fn: the callback function to cast
+ * @arg: the (pointer) argument to hand to the callback function.
+ *
+ * If a callback function takes a single argument, this macro does
+ * appropriate casts to a function which takes a single void * argument if the
+ * callback provided matches the @arg (or a const version).
+ *
+ * It is assumed that @arg is of pointer type: usually @arg is passed
+ * or assigned to a void * elsewhere anyway.
+ */
+#define typesafe_cb(rettype, fn, arg)					\
+	__typesafe_cb(rettype (*)(void *), (fn),			\
+		      rettype (*)(const typeof(arg)),			\
+		      rettype (*)(typeof(arg)))
+
+/**
+ * typesafe_cb_preargs - cast a callback function if it matches the arg
+ * @rettype: the return type of the callback function
+ * @fn: the callback function to cast
+ * @arg: the (pointer) argument to hand to the callback function.
+ *
+ * This is a version of typesafe_cb() for callbacks that take other arguments
+ * before the @arg.
+ */
+#define typesafe_cb_preargs(rettype, fn, arg, ...)			\
+	__typesafe_cb(rettype (*)(__VA_ARGS__, void *), (fn),		\
+		      rettype (*)(__VA_ARGS__, const typeof(arg)),	\
+		      rettype (*)(__VA_ARGS__, typeof(arg)))
 #endif
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index aabc8a1..3152c1e 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -4,9 +4,31 @@
 #include <linux/err.h>
 #include <linux/sched.h>
 
-struct task_struct *kthread_create(int (*threadfn)(void *data),
-				   void *data,
-				   const char namefmt[], ...)
+/**
+ * kthread_create - create a kthread.
+ * @threadfn: the function to run until signal_pending(current).
+ * @data: data ptr for @threadfn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: This helper function creates and names a kernel
+ * thread.  The thread will be stopped: use wake_up_process() to start
+ * it.  See also kthread_run(), kthread_create_on_cpu().
+ *
+ * When woken, the thread will run @threadfn() with @data as its
+ * argument. @threadfn() can either call do_exit() directly if it is a
+ * standalone thread for which noone will call kthread_stop(), or
+ * return when 'kthread_should_stop()' is true (which means
+ * kthread_stop() has been called).  The return value should be zero
+ * or a negative error number; it will be passed to kthread_stop().
+ *
+ * Returns a task_struct or ERR_PTR(-ENOMEM).
+ */
+#define kthread_create(threadfn, data, namefmt...)			\
+	__kthread_create(typesafe_cb(int,(threadfn),(data)), (data), namefmt)
+
+struct task_struct *__kthread_create(int (*threadfn)(void *data),
+				     void *data,
+				     const char namefmt[], ...)
 	__attribute__((format(printf, 3, 4)));
 
 /**
diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
index f1cb0ba..7c62754 100644
--- a/include/linux/stop_machine.h
+++ b/include/linux/stop_machine.h
@@ -6,10 +6,9 @@
    diables preeempt. */
 #include <linux/cpu.h>
 #include <linux/cpumask.h>
+#include <linux/compiler.h>
 #include <asm/system.h>
 
-#if defined(CONFIG_STOP_MACHINE) && defined(CONFIG_SMP)
-
 /* Deprecated, but useful for transition. */
 #define ALL_CPUS ~0U
 
@@ -26,8 +25,10 @@
  *
  * This can be thought of as a very heavy write lock, equivalent to
  * grabbing every spinlock in the kernel. */
-int stop_machine(int (*fn)(void *), void *data, const cpumask_t *cpus);
+#define stop_machine(fn, data, cpus)					\
+	stop_machine_notype(typesafe_cb(int, (fn), (data)), (data), (cpus))
 
+#if defined(CONFIG_STOP_MACHINE) && defined(CONFIG_SMP)
 /**
  * __stop_machine: freeze the machine on all CPUs and run this function
  * @fn: the function to run
@@ -38,10 +39,12 @@ int stop_machine(int (*fn)(void *), void *data, const cpumask_t *cpus);
  * won't come or go while it's being called.  Used by hotplug cpu.
  */
 int __stop_machine(int (*fn)(void *), void *data, const cpumask_t *cpus);
+
+int stop_machine_notype(int (*fn)(void *), void *data, const cpumask_t *cpus);
 #else
 
-static inline int stop_machine(int (*fn)(void *), void *data,
-			       const cpumask_t *cpus)
+static inline int stop_machine_notype(int (*fn)(void *), void *data,
+				      const cpumask_t *cpus)
 {
 	int ret;
 	local_irq_disable();
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 96cff2f..822d64d 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -111,29 +111,10 @@ static void create_kthread(struct kthread_create_info *create)
 	complete(&create->done);
 }
 
-/**
- * kthread_create - create a kthread.
- * @threadfn: the function to run until signal_pending(current).
- * @data: data ptr for @threadfn.
- * @namefmt: printf-style name for the thread.
- *
- * Description: This helper function creates and names a kernel
- * thread.  The thread will be stopped: use wake_up_process() to start
- * it.  See also kthread_run(), kthread_create_on_cpu().
- *
- * When woken, the thread will run @threadfn() with @data as its
- * argument. @threadfn() can either call do_exit() directly if it is a
- * standalone thread for which noone will call kthread_stop(), or
- * return when 'kthread_should_stop()' is true (which means
- * kthread_stop() has been called).  The return value should be zero
- * or a negative error number; it will be passed to kthread_stop().
- *
- * Returns a task_struct or ERR_PTR(-ENOMEM).
- */
-struct task_struct *kthread_create(int (*threadfn)(void *data),
-				   void *data,
-				   const char namefmt[],
-				   ...)
+struct task_struct *__kthread_create(int (*threadfn)(void *data),
+				     void *data,
+				     const char namefmt[],
+				     ...)
 {
 	struct kthread_create_info create;
 
@@ -158,7 +139,7 @@ struct task_struct *kthread_create(int (*threadfn)(void *data),
 	}
 	return create.result;
 }
-EXPORT_SYMBOL(kthread_create);
+EXPORT_SYMBOL(__kthread_create);
 
 /**
  * kthread_bind - bind a just-created kthread to a cpu.
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index e446c7c..82d72ae 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -176,7 +176,7 @@ kill_threads:
 	return err;
 }
 
-int stop_machine(int (*fn)(void *), void *data, const cpumask_t *cpus)
+int stop_machine_notype(int (*fn)(void *), void *data, const cpumask_t *cpus)
 {
 	int ret;
 
@@ -187,4 +187,4 @@ int stop_machine(int (*fn)(void *), void *data, const cpumask_t *cpus)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(stop_machine);
+EXPORT_SYMBOL_GPL(stop_machine_notype);

Re: [PULL] typesafe callbacks for kthread and stop_machine

[Andi Kleen]

On Thu, Jul 31, 2008 at 02:52:35PM +1000, Rusty Russell wrote:
> Just the two places I look after.  And this time the conglomerate patch is
> included below for more random commentry.

I must say I personally don't like the wrapper macros that you require
for each function that uses this. A wrapper macro has a large impact
on code readability because everyone following a call chain has
to do an additional grep/open file etc. step. I have my doubts not having casts 
outweights that disadvantage.

I know that gcc has this funky transparent union extension that
glibc socket() uses to allow different address types without casts.
It has the advantage of not needing wrapper macros. Any chance of 
using that instead? Or has that one been considered already and
discarded? 

-Andi

Re: [PULL] typesafe callbacks for kthread and stop_machine

[Rusty Russell]

On Friday 01 August 2008 00:06:43 Andi Kleen wrote:
> On Thu, Jul 31, 2008 at 02:52:35PM +1000, Rusty Russell wrote:
> > Just the two places I look after.  And this time the conglomerate patch
> > is included below for more random commentry.
>
> I must say I personally don't like the wrapper macros that you require
> for each function that uses this. A wrapper macro has a large impact
> on code readability because everyone following a call chain has
> to do an additional grep/open file etc. step. I have my doubts not having
> casts outweights that disadvantage.

Yes, but the benefits of using them everywhere is that they do become part of 
the landscape.  "Oh, that's a typesafe callback, OK".

If this were just about neatness, I'd share your doubts.  But I want to be 
able to change the type of a var and have the compiler complain when I hand 
it to a function which expects the old type.  This bit me a few months back, 
leading to this experiment.

> I know that gcc has this funky transparent union extension that
> glibc socket() uses to allow different address types without casts.
> It has the advantage of not needing wrapper macros. Any chance of
> using that instead? Or has that one been considered already and
> discarded?

That's aimed at a slightly different case, where the function knows what types 
it can get.  But that doesn't work for truly generic callbacks: the type is 
completely controlled by the caller. ie. we want to allow whatever type 
matches the arg.

Hope that clarifies,
Rusty.

Re: [PULL] typesafe callbacks for kthread and stop_machine

[Andi Kleen]

On Fri, Aug 01, 2008 at 10:39:29AM +1000, Rusty Russell wrote:
> On Friday 01 August 2008 00:06:43 Andi Kleen wrote:
> > On Thu, Jul 31, 2008 at 02:52:35PM +1000, Rusty Russell wrote:
> > > Just the two places I look after.  And this time the conglomerate patch
> > > is included below for more random commentry.
> >
> > I must say I personally don't like the wrapper macros that you require
> > for each function that uses this. A wrapper macro has a large impact
> > on code readability because everyone following a call chain has
> > to do an additional grep/open file etc. step. I have my doubts not having
> > casts outweights that disadvantage.
> 
> Yes, but the benefits of using them everywhere is that they do become part of 
> the landscape.  "Oh, that's a typesafe callback, OK".

It's still an additional step slowing the reader/debugger/maintainer/etc.
down even when he recognizes that pattern.

> > I know that gcc has this funky transparent union extension that
> > glibc socket() uses to allow different address types without casts.
> > It has the advantage of not needing wrapper macros. Any chance of
> > using that instead? Or has that one been considered already and
> > discarded?
> 
> That's aimed at a slightly different case, where the function knows what types 
> it can get.  But that doesn't work for truly generic callbacks: the type is 
> completely controlled by the caller. ie. we want to allow whatever type 
> matches the arg.

In my experience often call backs are just numbers. Wouldn't we get a significant
part of the benefit by just allowing void * and unsigned long by default? (that
can be done with the gcc extension)

Or alternatively perhaps just teach sparse about this common pattern by adding
new annotations (new annotations would be fine for me, i just don't like
the additional indirection)

-Andi

Re: [PULL] typesafe callbacks for kthread and stop_machine

[Rusty Russell]

On Saturday 02 August 2008 00:28:06 Andi Kleen wrote:
> On Fri, Aug 01, 2008 at 10:39:29AM +1000, Rusty Russell wrote:
> > Yes, but the benefits of using them everywhere is that they do become
> > part of the landscape.  "Oh, that's a typesafe callback, OK".
>
> It's still an additional step slowing the reader/debugger/maintainer/etc.
> down even when he recognizes that pattern.

I agree, and if it didn't have real benefits I'd oppose it.

> > That's aimed at a slightly different case, where the function knows what
> > types it can get.  But that doesn't work for truly generic callbacks: the
> > type is completely controlled by the caller. ie. we want to allow
> > whatever type matches the arg.
>
> In my experience often call backs are just numbers.

Really?

> Wouldn't we get a significant part of the benefit by just allowing void *
> and unsigned long by default? (that can be done with the gcc extension)

No.  See below patch for how these patches are used.

> Or alternatively perhaps just teach sparse about this common pattern by
> adding new annotations (new annotations would be fine for me, i just don't
> like the additional indirection)

But we'd have to make callbacks take void * for the function param.  That
would be a big step backwards.

---
 drivers/char/hw_random/intel-rng.c |    3 +--
 kernel/module.c                    |   10 +++-------
 2 files changed, 4 insertions(+), 9 deletions(-)

diff -r e279190b7b43 drivers/char/hw_random/intel-rng.c
--- a/drivers/char/hw_random/intel-rng.c	Mon Jan 21 14:42:54 2008 +1100
+++ b/drivers/char/hw_random/intel-rng.c	Mon Jan 21 15:04:00 2008 +1100
@@ -227,9 +227,8 @@ struct intel_rng_hw {
 	u8 fwh_dec_en1_val;
 };
 
-static int __init intel_rng_hw_init(void *_intel_rng_hw)
+static int __init intel_rng_hw_init(struct intel_rng_hw *intel_rng_hw)
 {
-	struct intel_rng_hw *intel_rng_hw = _intel_rng_hw;
 	u8 mfc, dvc;
 
 	/* interrupts disabled in stop_machine_run call */
diff -r e279190b7b43 kernel/module.c
--- a/kernel/module.c	Mon Jan 21 14:42:54 2008 +1100
+++ b/kernel/module.c	Mon Jan 21 15:04:00 2008 +1100
@@ -623,10 +623,8 @@ struct stopref
 };
 
 /* Whole machine is stopped with interrupts off when this runs. */
-static int __try_stop_module(void *_sref)
+static int __try_stop_module(struct stopref *sref)
 {
-	struct stopref *sref = _sref;
-
 	/* If it's not unused, quit unless we are told to block. */
 	if ((sref->flags & O_NONBLOCK) && module_refcount(sref->mod) != 0) {
 		if (!(*sref->forced = try_force_unload(sref->flags)))
@@ -1305,9 +1303,8 @@ static void mod_kobject_remove(struct mo
  * link the module with the whole machine is stopped with interrupts off
  * - this defends against kallsyms not taking locks
  */
-static int __link_module(void *_mod)
+static int __link_module(struct module *mod)
 {
-	struct module *mod = _mod;
 	list_add(&mod->list, &modules);
 	return 0;
 }
@@ -1316,9 +1313,8 @@ static int __link_module(void *_mod)
  * unlink the module with the whole machine is stopped with interrupts off
  * - this defends against kallsyms not taking locks
  */
-static int __unlink_module(void *_mod)
+static int __unlink_module(struct module *mod)
 {
-	struct module *mod = _mod;
 	list_del(&mod->list);
 	return 0;
 }