[PATCH 1/5] lib: introduce call_once()

[PATCH 1/5] lib: introduce call_once()

[Akinobu Mita]

call_once() is an utility function which has similar functionality of
pthread_once().

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 include/linux/once.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 lib/Makefile         |    2 +-
 lib/once.c           |   18 ++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)

Index: 2.6-rc/include/linux/once.h
===================================================================
--- /dev/null
+++ 2.6-rc/include/linux/once.h
@@ -0,0 +1,42 @@
+#ifndef __LINUX_ONCE_H
+#define __LINUX_ONCE_H
+
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+
+struct once_control {
+	struct mutex lock;
+	int done;
+};
+
+#define __ONCE_INITIALIZER(name) {					\
+		.lock		= __MUTEX_INITIALIZER(name.lock),	\
+		.done		= 0,					\
+	}
+
+#define DEFINE_ONCE(name) struct once_control name = __ONCE_INITIALIZER(name)
+
+extern int call_once_slow(struct once_control *once_control,
+			  int (*init_rouine)(void));
+
+/*
+ * call_once - call the initialization function only once
+ *
+ * @once_control: guarantee that the init_routine will be called only once
+ * @init_routine: initialization function
+ *
+ * The first call to call_once(), with a given once_control, shall call the
+ * init_routine with no arguments and return the value init_routine returned.
+ * If the init_routine returns zero which indicates the initialization
+ * succeeded, subsequent calls of call_once() with the same once_control shall
+ * not call the init_routine and return zero.
+ */
+
+static inline int call_once(struct once_control *once_control,
+			    int (*init_rouine)(void))
+{
+	return likely(once_control->done) ? 0
+			: call_once_slow(once_control, init_rouine);
+}
+
+#endif /* __LINUX_ONCE_H */
Index: 2.6-rc/lib/once.c
===================================================================
--- /dev/null
+++ 2.6-rc/lib/once.c
@@ -0,0 +1,18 @@
+#include <linux/module.h>
+#include <linux/once.h>
+
+int call_once_slow(struct once_control *once_control, int (*init_rouine)(void))
+{
+	int err = 0;
+
+	mutex_lock(&once_control->lock);
+	if (!once_control->done) {
+		err = init_rouine();
+		if (!err)
+			once_control->done = 1;
+	}
+	mutex_unlock(&once_control->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(call_once_slow);
Index: 2.6-rc/lib/Makefile
===================================================================
--- 2.6-rc.orig/lib/Makefile
+++ 2.6-rc/lib/Makefile
@@ -14,7 +14,7 @@ lib-$(CONFIG_SMP) += cpumask.o
 lib-y	+= kobject.o kref.o klist.o
 
 obj-y += div64.o sort.o parser.o halfmd4.o debug_locks.o random32.o \
-	 bust_spinlocks.o hexdump.o kasprintf.o bitmap.o scatterlist.o
+	 bust_spinlocks.o hexdump.o kasprintf.o bitmap.o scatterlist.o once.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG

[PATCH 2/5] idr: use call_once()

[Akinobu Mita]

idr_layer_cache is created when idr_init() is called first time by someone.

This patch makes the idr_layer_cache initialization to use call_once().

This conversion prevents an unlikely race condition when two idr_init()
callers attempt to create idr_layer_cache simultaneously.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 lib/idr.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Index: 2.6-rc/lib/idr.c
===================================================================
--- 2.6-rc.orig/lib/idr.c
+++ 2.6-rc/lib/idr.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/once.h>
 #endif
 #include <linux/err.h>
 #include <linux/string.h>
@@ -585,12 +586,12 @@ static void idr_cache_ctor(struct kmem_c
 	memset(idr_layer, 0, sizeof(struct idr_layer));
 }
 
-static  int init_id_cache(void)
+static int init_id_cache(void)
 {
-	if (!idr_layer_cache)
-		idr_layer_cache = kmem_cache_create("idr_layer_cache",
+	idr_layer_cache = kmem_cache_create("idr_layer_cache",
 			sizeof(struct idr_layer), 0, 0, idr_cache_ctor);
-	return 0;
+
+	return !idr_layer_cache ? -ENOMEM : 0;
 }
 
 /**
@@ -602,7 +603,9 @@ static  int init_id_cache(void)
  */
 void idr_init(struct idr *idp)
 {
-	init_id_cache();
+	static DEFINE_ONCE(once);
+
+	call_once(&once, init_id_cache);
 	memset(idp, 0, sizeof(struct idr));
 	spin_lock_init(&idp->lock);
 }

[PATCH 3/5] hugetlbfs: use call_once()

[Akinobu Mita]

This patch defers mounting hugetlbfs till hugetlb_file_setup() is
called first time by using call_once().

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: William Irwin <wli@holomorphy.com>
---
 fs/hugetlbfs/inode.c |   30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Index: 2.6-rc/fs/hugetlbfs/inode.c
===================================================================
--- 2.6-rc.orig/fs/hugetlbfs/inode.c
+++ 2.6-rc/fs/hugetlbfs/inode.c
@@ -31,6 +31,7 @@
 #include <linux/dnotify.h>
 #include <linux/statfs.h>
 #include <linux/security.h>
+#include <linux/once.h>
 
 #include <asm/uaccess.h>
 
@@ -903,6 +904,13 @@ static struct file_system_type hugetlbfs
 
 static struct vfsmount *hugetlbfs_vfsmount;
 
+static int init_hugetlbfs_vfsmount(void)
+{
+	hugetlbfs_vfsmount = kern_mount(&hugetlbfs_fs_type);
+
+	return IS_ERR(hugetlbfs_vfsmount) ? PTR_ERR(hugetlbfs_vfsmount) : 0;
+}
+
 static int can_do_hugetlb_shm(void)
 {
 	return likely(capable(CAP_IPC_LOCK) ||
@@ -912,14 +920,16 @@ static int can_do_hugetlb_shm(void)
 
 struct file *hugetlb_file_setup(const char *name, size_t size)
 {
-	int error = -ENOMEM;
+	int error;
 	struct file *file;
 	struct inode *inode;
 	struct dentry *dentry, *root;
 	struct qstr quick_string;
+	static DEFINE_ONCE(once);
 
-	if (!hugetlbfs_vfsmount)
-		return ERR_PTR(-ENOENT);
+	error = call_once(&once, init_hugetlbfs_vfsmount);
+	if (error)
+		return ERR_PTR(error);
 
 	if (!can_do_hugetlb_shm())
 		return ERR_PTR(-EPERM);
@@ -970,7 +980,6 @@ out_shm_unlock:
 static int __init init_hugetlbfs_fs(void)
 {
 	int error;
-	struct vfsmount *vfsmount;
 
 	error = bdi_init(&hugetlbfs_backing_dev_info);
 	if (error)
@@ -986,18 +995,9 @@ static int __init init_hugetlbfs_fs(void
 	if (error)
 		goto out;
 
-	vfsmount = kern_mount(&hugetlbfs_fs_type);
-
-	if (!IS_ERR(vfsmount)) {
-		hugetlbfs_vfsmount = vfsmount;
-		return 0;
-	}
-
-	error = PTR_ERR(vfsmount);
-
+	return 0;
  out:
-	if (error)
-		kmem_cache_destroy(hugetlbfs_inode_cachep);
+	kmem_cache_destroy(hugetlbfs_inode_cachep);
  out2:
 	bdi_destroy(&hugetlbfs_backing_dev_info);
 	return error;

[PATCH 4/5] shmem: use call_once()

[Akinobu Mita]

This patch defers mounting tmpfs till shmem_file_setup() is
called first time by using call_once().

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 mm/shmem.c |   32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

Index: 2.6-rc/mm/shmem.c
===================================================================
--- 2.6-rc.orig/mm/shmem.c
+++ 2.6-rc/mm/shmem.c
@@ -50,6 +50,7 @@
 #include <linux/migrate.h>
 #include <linux/highmem.h>
 #include <linux/seq_file.h>
+#include <linux/once.h>
 
 #include <asm/uaccess.h>
 #include <asm/div64.h>
@@ -2520,7 +2521,6 @@ static struct file_system_type tmpfs_fs_
 	.get_sb		= shmem_get_sb,
 	.kill_sb	= kill_litter_super,
 };
-static struct vfsmount *shm_mnt;
 
 static int __init init_tmpfs(void)
 {
@@ -2540,27 +2540,29 @@ static int __init init_tmpfs(void)
 		goto out2;
 	}
 
-	shm_mnt = vfs_kern_mount(&tmpfs_fs_type, MS_NOUSER,
-				tmpfs_fs_type.name, NULL);
-	if (IS_ERR(shm_mnt)) {
-		error = PTR_ERR(shm_mnt);
-		printk(KERN_ERR "Could not kern_mount tmpfs\n");
-		goto out1;
-	}
 	return 0;
-
-out1:
-	unregister_filesystem(&tmpfs_fs_type);
 out2:
 	destroy_inodecache();
 out3:
 	bdi_destroy(&shmem_backing_dev_info);
 out4:
-	shm_mnt = ERR_PTR(error);
 	return error;
 }
 module_init(init_tmpfs)
 
+static struct vfsmount *shm_mnt;
+
+static int init_shm_mnt(void)
+{
+	shm_mnt = vfs_kern_mount(&tmpfs_fs_type, MS_NOUSER,
+				tmpfs_fs_type.name, NULL);
+	if (IS_ERR(shm_mnt)) {
+		printk(KERN_ERR "Could not kern_mount tmpfs\n");
+		return PTR_ERR(shm_mnt);
+	}
+	return 0;
+}
+
 /*
  * shmem_file_setup - get an unlinked file living in tmpfs
  *
@@ -2575,9 +2577,11 @@ struct file *shmem_file_setup(char *name
 	struct inode *inode;
 	struct dentry *dentry, *root;
 	struct qstr this;
+	static DEFINE_ONCE(once);
 
-	if (IS_ERR(shm_mnt))
-		return (void *)shm_mnt;
+	error = call_once(&once, init_shm_mnt);
+	if (error)
+		return ERR_PTR(error);
 
 	if (size < 0 || size > SHMEM_MAX_BYTES)
 		return ERR_PTR(-EINVAL);

[PATCH 5/5] tiny-shmem: use call_once()

[Akinobu Mita]

This patch defers mounting tmpfs till shmem_file_setup() is
called first time by using call_once().

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 mm/tiny-shmem.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Index: 2.6-rc/mm/tiny-shmem.c
===================================================================
--- 2.6-rc.orig/mm/tiny-shmem.c
+++ 2.6-rc/mm/tiny-shmem.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/swap.h>
 #include <linux/ramfs.h>
+#include <linux/once.h>
 
 static struct file_system_type tmpfs_fs_type = {
 	.name		= "tmpfs",
@@ -26,19 +27,23 @@ static struct file_system_type tmpfs_fs_
 	.kill_sb	= kill_litter_super,
 };
 
-static struct vfsmount *shm_mnt;
-
 static int __init init_tmpfs(void)
 {
 	BUG_ON(register_filesystem(&tmpfs_fs_type) != 0);
 
-	shm_mnt = kern_mount(&tmpfs_fs_type);
-	BUG_ON(IS_ERR(shm_mnt));
-
 	return 0;
 }
 module_init(init_tmpfs)
 
+static struct vfsmount *shm_mnt;
+
+static int init_shm_mnt(void)
+{
+	shm_mnt = kern_mount(&tmpfs_fs_type);
+
+	return IS_ERR(shm_mnt) ? PTR_ERR(shm_mnt) : 0;
+}
+
 /*
  * shmem_file_setup - get an unlinked file living in tmpfs
  *
@@ -53,9 +58,11 @@ struct file *shmem_file_setup(char *name
 	struct inode *inode;
 	struct dentry *dentry, *root;
 	struct qstr this;
+	static DEFINE_ONCE(once);
 
-	if (IS_ERR(shm_mnt))
-		return (void *)shm_mnt;
+	error = call_once(&once, init_shm_mnt);
+	if (error)
+		return ERR_PTR(error);
 
 	error = -ENOMEM;
 	this.name = name;

Re: [PATCH 4/5] shmem: use call_once()

[Hugh Dickins]

On Tue, 11 Mar 2008, Akinobu Mita wrote:
> This patch defers mounting tmpfs till shmem_file_setup() is
> called first time by using call_once().

Please explain why we might need this patch: is something changing
elsewhere?  Or are you misled by that "module_init(init_tmpfs)"
into thinking that mm/shmem.c is sometimes built modular?

Hugh

Re: [PATCH 4/5] shmem: use call_once()

[Akinobu Mita]

2008/3/11, Hugh Dickins <hugh@veritas.com>:
> On Tue, 11 Mar 2008, Akinobu Mita wrote:
>  > This patch defers mounting tmpfs till shmem_file_setup() is
>  > called first time by using call_once().
>
>
> Please explain why we might need this patch: is something changing
>  elsewhere?  Or are you misled by that "module_init(init_tmpfs)"
>  into thinking that mm/shmem.c is sometimes built modular?
>

If no processes call shmem_file_setup() (via shm_get(2)), it is unnecessary
to do vfs_kern_mount(&tmpfs_fs_type, ...) unconditionary in boot-time.
So I thought it is suitable example to demonstrate how to use "call_once()"
in this patch set.

Re: [PATCH 4/5] shmem: use call_once()

[Hugh Dickins]

On Tue, 11 Mar 2008, Akinobu Mita wrote:
> 2008/3/11, Hugh Dickins <hugh@veritas.com>:
> > On Tue, 11 Mar 2008, Akinobu Mita wrote:
> >  > This patch defers mounting tmpfs till shmem_file_setup() is
> >  > called first time by using call_once().
> >
> > Please explain why we might need this patch: is something changing
> >  elsewhere?  Or are you misled by that "module_init(init_tmpfs)"
> >  into thinking that mm/shmem.c is sometimes built modular?
> 
> If no processes call shmem_file_setup() (via shm_get(2)), it is unnecessary
                                       or shmem_zero_setup, not very common
> to do vfs_kern_mount(&tmpfs_fs_type, ...) unconditionary in boot-time.
> So I thought it is suitable example to demonstrate how to use "call_once()"
> in this patch set.

Oh, I see, thanks.  Well, I don't feel all that strongly about it; but
on the whole I'd prefer we leave it as part of the __init, than change
it around to provide this example (and risk introducing some weird issue
e.g. related to its "dev"?).  I guess the same should go for the huge
and the tiny, whereas you have better justification in the idr case.
Call me over-cautious.

Hugh

Re: [PATCH 1/5] lib: introduce call_once()

[Joe Perches]

On Mon, 2008-03-10 at 23:57 +0900, Akinobu Mita wrote:
> +++ 2.6-rc/include/linux/once.h
> +struct once_control {
> +	struct mutex lock;
> +	int done;

bool?

> +};
> +
> +#define __ONCE_INITIALIZER(name) {					\
> +		.lock		= __MUTEX_INITIALIZER(name.lock),	\
> +		.done		= 0,					\
> +	}
> +
> +#define DEFINE_ONCE(name) struct once_control name = __ONCE_INITIALIZER(name)

static?

> +
> +extern int call_once_slow(struct once_control *once_control,
> +			  int (*init_rouine)(void));

return bool?
spelling: s/rouine/routine/g

Re: [PATCH 1/5] lib: introduce call_once()

[Akinobu Mita]

2008/3/11, Joe Perches <joe@perches.com>:
> On Mon, 2008-03-10 at 23:57 +0900, Akinobu Mita wrote:
>  > +++ 2.6-rc/include/linux/once.h
>
> > +struct once_control {
>  > +     struct mutex lock;
>  > +     int done;
>
>
> bool?

Yes, this definetly should be bool.

>  > +};
>  > +
>  > +#define __ONCE_INITIALIZER(name) {                                   \
>  > +             .lock           = __MUTEX_INITIALIZER(name.lock),       \
>  > +             .done           = 0,                                    \
>  > +     }
>  > +
>  > +#define DEFINE_ONCE(name) struct once_control name = __ONCE_INITIALIZER(name)
>
>
> static?

no, static keyword should not be implicitly added in this macro.
DEFINE_ONCE is intended to be anologous to DEFINE_LOCK, DEFINE_WAIT,
and all other similar interfaces in kernel.

>  > +
>  > +extern int call_once_slow(struct once_control *once_control,
>  > +                       int (*init_rouine)(void));
>
>
> return bool?

call_once() returns error-code when init_routine fails.

>  spelling: s/rouine/routine/g

Oops, thanks.

Re: [PATCH 1/5] lib: introduce call_once()

[Andrew Morton]

On Mon, 10 Mar 2008 23:57:05 +0900 Akinobu Mita <akinobu.mita@gmail.com> wrote:

> call_once() is an utility function which has similar functionality of
> pthread_once().
> 
> +/*
> + * call_once - call the initialization function only once
> + *
> + * @once_control: guarantee that the init_routine will be called only once
> + * @init_routine: initialization function
> + *
> + * The first call to call_once(), with a given once_control, shall call the
> + * init_routine with no arguments and return the value init_routine returned.
> + * If the init_routine returns zero which indicates the initialization
> + * succeeded, subsequent calls of call_once() with the same once_control shall
> + * not call the init_routine and return zero.
> + */
> +
> +static inline int call_once(struct once_control *once_control,
> +			    int (*init_rouine)(void))
> +{
> +	return likely(once_control->done) ? 0
> +			: call_once_slow(once_control, init_rouine);
> +}

I don't believe that this shold be described in terms of an "init_routine". 
This mechanism can be used for things other than initialisation routines.

It is spelled "routine", not "rouine".


Would it not be simpler and more general to do:

#define ONCE()						\
	({						\
		static long flag;			\
							\
		return !test_and_set_bit(0, flag);	\
	})

and then callers can do

	if (ONCE())
		do_something();

?

Re: [PATCH 1/5] lib: introduce call_once()

[Nick Piggin]

On Tuesday 11 March 2008 14:48, Andrew Morton wrote:
> On Mon, 10 Mar 2008 23:57:05 +0900 Akinobu Mita <akinobu.mita@gmail.com> 
wrote:
> > call_once() is an utility function which has similar functionality of
> > pthread_once().
> >
> > +/*
> > + * call_once - call the initialization function only once
> > + *
> > + * @once_control: guarantee that the init_routine will be called only
> > once + * @init_routine: initialization function
> > + *
> > + * The first call to call_once(), with a given once_control, shall call
> > the + * init_routine with no arguments and return the value init_routine
> > returned. + * If the init_routine returns zero which indicates the
> > initialization + * succeeded, subsequent calls of call_once() with the
> > same once_control shall + * not call the init_routine and return zero.
> > + */
> > +
> > +static inline int call_once(struct once_control *once_control,
> > +			    int (*init_rouine)(void))
> > +{
> > +	return likely(once_control->done) ? 0
> > +			: call_once_slow(once_control, init_rouine);
> > +}
>
> I don't believe that this shold be described in terms of an "init_routine".
> This mechanism can be used for things other than initialisation routines.
>
> It is spelled "routine", not "rouine".
>
>
> Would it not be simpler and more general to do:
>
> #define ONCE()						\
> 	({						\
> 		static long flag;			\
> 							\
> 		return !test_and_set_bit(0, flag);	\
> 	})
>
> and then callers can do
>
> 	if (ONCE())
> 		do_something();
>
> ?

Isn't this usually going to be buggy if you have concurrent access
here? I'd prefer to keep synchronisation details in the caller and
not have this call_once at all.

Re: [PATCH 1/5] lib: introduce call_once()

[Andrew Morton]

On Tue, 11 Mar 2008 15:10:52 +1100 Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> On Tuesday 11 March 2008 14:48, Andrew Morton wrote:
> > On Mon, 10 Mar 2008 23:57:05 +0900 Akinobu Mita <akinobu.mita@gmail.com> 
> wrote:
> > > call_once() is an utility function which has similar functionality of
> > > pthread_once().
> > >
> > > +/*
> > > + * call_once - call the initialization function only once
> > > + *
> > > + * @once_control: guarantee that the init_routine will be called only
> > > once + * @init_routine: initialization function
> > > + *
> > > + * The first call to call_once(), with a given once_control, shall call
> > > the + * init_routine with no arguments and return the value init_routine
> > > returned. + * If the init_routine returns zero which indicates the
> > > initialization + * succeeded, subsequent calls of call_once() with the
> > > same once_control shall + * not call the init_routine and return zero.
> > > + */
> > > +
> > > +static inline int call_once(struct once_control *once_control,
> > > +			    int (*init_rouine)(void))
> > > +{
> > > +	return likely(once_control->done) ? 0
> > > +			: call_once_slow(once_control, init_rouine);
> > > +}
> >
> > I don't believe that this shold be described in terms of an "init_routine".
> > This mechanism can be used for things other than initialisation routines.
> >
> > It is spelled "routine", not "rouine".
> >
> >
> > Would it not be simpler and more general to do:
> >
> > #define ONCE()						\
> > 	({						\
> > 		static long flag;			\
> > 							\
> > 		return !test_and_set_bit(0, flag);	\
> > 	})
> >
> > and then callers can do
> >
> > 	if (ONCE())
> > 		do_something();
> >
> > ?
> 
> Isn't this usually going to be buggy if you have concurrent access
> here? I'd prefer to keep synchronisation details in the caller and
> not have this call_once at all.

Well, I'm a bit dubious about the calue of all of this (althoug I didn't
review the callers).

But the above code is guaranteed race-free ;)

Re: [PATCH 1/5] lib: introduce call_once()

[Akinobu Mita]

2008/3/11, Andrew Morton <akpm@linux-foundation.org>:
> On Tue, 11 Mar 2008 15:10:52 +1100 Nick Piggin <nickpiggin@yahoo.com.au> wrote:
>
>  > On Tuesday 11 March 2008 14:48, Andrew Morton wrote:
>  > > On Mon, 10 Mar 2008 23:57:05 +0900 Akinobu Mita <akinobu.mita@gmail.com>
>  > wrote:
>  > > > call_once() is an utility function which has similar functionality of
>  > > > pthread_once().
>  > > >
>  > > > +/*
>  > > > + * call_once - call the initialization function only once
>  > > > + *
>  > > > + * @once_control: guarantee that the init_routine will be called only
>  > > > once + * @init_routine: initialization function
>  > > > + *
>  > > > + * The first call to call_once(), with a given once_control, shall call
>  > > > the + * init_routine with no arguments and return the value init_routine
>  > > > returned. + * If the init_routine returns zero which indicates the
>  > > > initialization + * succeeded, subsequent calls of call_once() with the
>  > > > same once_control shall + * not call the init_routine and return zero.
>  > > > + */
>  > > > +
>  > > > +static inline int call_once(struct once_control *once_control,
>  > > > +                     int (*init_rouine)(void))
>  > > > +{
>  > > > + return likely(once_control->done) ? 0
>  > > > +                 : call_once_slow(once_control, init_rouine);
>  > > > +}
>  > >
>  > > I don't believe that this shold be described in terms of an "init_routine".
>  > > This mechanism can be used for things other than initialisation routines.
>  > >
>  > > It is spelled "routine", not "rouine".
>  > >
>  > >
>  > > Would it not be simpler and more general to do:
>  > >
>  > > #define ONCE()                                              \
>  > >     ({                                              \
>  > >             static long flag;                       \
>  > >                                                     \
>  > >             return !test_and_set_bit(0, flag);      \
>  > >     })
>  > >
>  > > and then callers can do
>  > >
>  > >     if (ONCE())
>  > >             do_something();
>  > >
>  > > ?

This cannot be used directly in the conversions of patch 2 - 5.
Because the callers of call_once() in patch 2 - 5 need to wait for the
complition of "init_routine". Some blocking mechanism is needed.

>  > Isn't this usually going to be buggy if you have concurrent access
>  > here? I'd prefer to keep synchronisation details in the caller and
>  > not have this call_once at all.
>
>
> Well, I'm a bit dubious about the calue of all of this (althoug I didn't
>  review the callers).
>

I'll try to find another conversions to back up the necessity...

Re: [PATCH 1/5] lib: introduce call_once()

[Andrew Morton]

On Tue, 11 Mar 2008 21:27:30 +0900
"Akinobu Mita" <akinobu.mita@gmail.com> wrote:

> 2008/3/11, Andrew Morton <akpm@linux-foundation.org>:
> > On Tue, 11 Mar 2008 15:10:52 +1100 Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> >
> >  > > #define ONCE()                                              \
> >  > >     ({                                              \
> >  > >             static long flag;                       \
> >  > >                                                     \
> >  > >             return !test_and_set_bit(0, flag);      \
> >  > >     })
> >  > >
> >  > > and then callers can do
> >  > >
> >  > >     if (ONCE())
> >  > >             do_something();
> >  > >
> >  > > ?
> 
> This cannot be used directly in the conversions of patch 2 - 5.
> Because the callers of call_once() in patch 2 - 5 need to wait for the
> complition of "init_routine". Some blocking mechanism is needed.

Of course it can be used in those places.  Here's the idr.c conversion:

--- a/lib/idr.c~a
+++ a/lib/idr.c
@@ -585,14 +585,6 @@ static void idr_cache_ctor(struct kmem_c
 	memset(idr_layer, 0, sizeof(struct idr_layer));
 }
 
-static  int init_id_cache(void)
-{
-	if (!idr_layer_cache)
-		idr_layer_cache = kmem_cache_create("idr_layer_cache",
-			sizeof(struct idr_layer), 0, 0, idr_cache_ctor);
-	return 0;
-}
-
 /**
  * idr_init - initialize idr handle
  * @idp:	idr handle
@@ -602,7 +594,9 @@ static  int init_id_cache(void)
  */
 void idr_init(struct idr *idp)
 {
-	init_id_cache();
+	if (ONCE())
+		idr_layer_cache = kmem_cache_create("idr_layer_cache",
+			sizeof(struct idr_layer), 0, 0, idr_cache_ctor);
 	memset(idp, 0, sizeof(struct idr));
 	spin_lock_init(&idp->lock);
 }

Re: [PATCH 1/5] lib: introduce call_once()

[Joe Perches]

On Tue, 2008-03-11 at 10:35 -0700, Andrew Morton wrote:
> #define ONCE()                                              \
>     ({                                              \
>             static long flag;                       \
>                                                     \
>             return !test_and_set_bit(0, flag);      \
>     })

test_and_set_bit takes an address

Perhaps:

#define DO_ONCE(x) \
	({ static long flag; if (test_and_set_bit(0, &flag)) x; 1; })

DO_ONCE(foo);

Re: [PATCH 1/5] lib: introduce call_once()

[Andrew Morton]

On Tue, 11 Mar 2008 11:56:55 -0700
Joe Perches <joe@perches.com> wrote:

> On Tue, 2008-03-11 at 10:35 -0700, Andrew Morton wrote:
> > #define ONCE()                                              \
> >     ({                                              \
> >             static long flag;                       \
> >                                                     \
> >             return !test_and_set_bit(0, flag);      \
> >     })
> 
> test_and_set_bit takes an address

duh.

> Perhaps:
> 
> #define DO_ONCE(x) \
> 	({ static long flag; if (test_and_set_bit(0, &flag)) x; 1; })
> 
> DO_ONCE(foo);

No, that's completely unnecessary and would produce nasty-looking code. 
Take a look at some of the wait_event monstrosities we have.


I'm not sure that we need any of this once() stuff really.

Re: [PATCH 1/5] lib: introduce call_once()

[Akinobu Mita]

> Of course it can be used in those places.  Here's the idr.c conversion:
> 
> --- a/lib/idr.c~a
> +++ a/lib/idr.c
> @@ -585,14 +585,6 @@ static void idr_cache_ctor(struct kmem_c
>  	memset(idr_layer, 0, sizeof(struct idr_layer));
>  }
>  
> -static  int init_id_cache(void)
> -{
> -	if (!idr_layer_cache)
> -		idr_layer_cache = kmem_cache_create("idr_layer_cache",
> -			sizeof(struct idr_layer), 0, 0, idr_cache_ctor);
> -	return 0;
> -}
> -
>  /**
>   * idr_init - initialize idr handle
>   * @idp:	idr handle
> @@ -602,7 +594,9 @@ static  int init_id_cache(void)
>   */
>  void idr_init(struct idr *idp)
>  {
> -	init_id_cache();
> +	if (ONCE())
> +		idr_layer_cache = kmem_cache_create("idr_layer_cache",
> +			sizeof(struct idr_layer), 0, 0, idr_cache_ctor);
>  	memset(idp, 0, sizeof(struct idr));
>  	spin_lock_init(&idp->lock);
>  }
> 
> 

Maybe this doesn't handle kmem_cache_create() failure.

Anyhow this is the alternative patch to fix what the patch 1/5 was
trying to fix.

Subject: idr: create idr_layer_cache at boot time

This patch checks kmem_cache_create() failure by SLAB_PANIC
by creating idr_layer_cache unconditionary at boot time rather than
creates when idr_init() is called first time by someone.

This change also enables to eliminate unnecessary check every time
idr_init() is called.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 include/linux/idr.h |    2 ++
 init/main.c         |    2 ++
 lib/idr.c           |   10 ++++------
 3 files changed, 8 insertions(+), 6 deletions(-)

Index: 2.6-rc/lib/idr.c
===================================================================
--- 2.6-rc.orig/lib/idr.c
+++ 2.6-rc/lib/idr.c
@@ -585,12 +585,11 @@ static void idr_cache_ctor(struct kmem_c
 	memset(idr_layer, 0, sizeof(struct idr_layer));
 }
 
-static  int init_id_cache(void)
+void __init init_id_cache(void)
 {
-	if (!idr_layer_cache)
-		idr_layer_cache = kmem_cache_create("idr_layer_cache",
-			sizeof(struct idr_layer), 0, 0, idr_cache_ctor);
-	return 0;
+	idr_layer_cache = kmem_cache_create("idr_layer_cache",
+				sizeof(struct idr_layer), 0, SLAB_PANIC,
+				idr_cache_ctor);
 }
 
 /**
@@ -602,7 +601,6 @@ static  int init_id_cache(void)
  */
 void idr_init(struct idr *idp)
 {
-	init_id_cache();
 	memset(idp, 0, sizeof(struct idr));
 	spin_lock_init(&idp->lock);
 }
Index: 2.6-rc/include/linux/idr.h
===================================================================
--- 2.6-rc.orig/include/linux/idr.h
+++ 2.6-rc/include/linux/idr.h
@@ -115,4 +115,6 @@ void ida_remove(struct ida *ida, int id)
 void ida_destroy(struct ida *ida);
 void ida_init(struct ida *ida);
 
+void __init init_id_cache(void);
+
 #endif /* __IDR_H__ */
Index: 2.6-rc/init/main.c
===================================================================
--- 2.6-rc.orig/init/main.c
+++ 2.6-rc/init/main.c
@@ -58,6 +58,7 @@
 #include <linux/kthread.h>
 #include <linux/sched.h>
 #include <linux/signal.h>
+#include <linux/idr.h>
 
 #include <asm/io.h>
 #include <asm/bugs.h>
@@ -616,6 +617,7 @@ asmlinkage void __init start_kernel(void
 	enable_debug_pagealloc();
 	cpu_hotplug_init();
 	kmem_cache_init();
+	init_id_cache();
 	setup_per_cpu_pageset();
 	numa_policy_init();
 	if (late_time_init)

Re: [PATCH 1/5] lib: introduce call_once()

[Nick Piggin]

On Tuesday 11 March 2008 01:57, Akinobu Mita wrote:

> +static inline int call_once(struct once_control *once_control,
> +			    int (*init_rouine)(void))
> +{
> +	return likely(once_control->done) ? 0
> +			: call_once_slow(once_control, init_rouine);
> +}
> +
> +#endif /* __LINUX_ONCE_H */
> Index: 2.6-rc/lib/once.c
> ===================================================================
> --- /dev/null
> +++ 2.6-rc/lib/once.c
> @@ -0,0 +1,18 @@
> +#include <linux/module.h>
> +#include <linux/once.h>
> +
> +int call_once_slow(struct once_control *once_control, int
> (*init_rouine)(void)) +{
> +	int err = 0;
> +
> +	mutex_lock(&once_control->lock);
> +	if (!once_control->done) {
> +		err = init_rouine();
> +		if (!err)
> +			once_control->done = 1;
> +	}
> +	mutex_unlock(&once_control->lock);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(call_once_slow);

The store "once_control->done = 1" can become visible before
init_routine has finished. The code after calling call_once may
also speculatively load some memory before the load of
once_control->done completes, so you can likewise have a data
race that way too.

To fix this, you need smp_wmb after init_rouine(), and probably
smp_mb() in the fastpath after the check but before returning.

Basically any time you have this situation where you're touching
a shared variable without using locks, then you're vastly
increasing the complexity of the code, and so you must have a
good reason for it.

So acquiring the mutex unconditionally would be the best way to
go, unless you're calling this a lot in fastpaths (in which case
I would say you should probably rework your code)

Thanks,
Nick