Skip to contentSkip to conversation

lib: introduce call_once()

PATCH In review Akinobu Mita 5 participants 19 messages Mar 10, 2008 – Mar 15, 2008

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

PATCH 1/5 lib: introduce call_once()

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

---
 include/linux/once.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 lib/Makefile         |    2 +-
 lib/once.c           |   18 ++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)
include/linux/once.h new file +42 -0
Index: 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 */
lib/once.c new file +18 -0
Index: 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);
lib/Makefile +1 -1
Index: 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
#1 lore raw

PATCH 2/5 idr: use call_once()

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.

---
 lib/idr.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
lib/idr.c +8 -5
Index: 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);
 }
#2 lore raw

PATCH 3/5 hugetlbfs: use call_once()

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

---
 fs/hugetlbfs/inode.c |   30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
fs/hugetlbfs/inode.c +15 -15
Index: 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;
#3 lore raw

PATCH 4/5 shmem: use call_once()

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

---
 mm/shmem.c |   32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)
mm/shmem.c +18 -14
Index: 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);
#4 lore raw

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

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

---
 mm/tiny-shmem.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)
mm/tiny-shmem.c +14 -7
Index: 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;
#5 lore raw

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

In reply to Akinobu Mita
Quoted 2 lines · Akinobu Mita

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

#6 lore raw

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

2008/3/11, Hugh Dickins <[email protected]>:

Quoted 9 lines · Akinobu Mita
Quoted 2 lines

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.

#7 lore raw

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

Quoted 10 lines · Akinobu Mita

2008/3/11, Hugh Dickins <[email protected]>:

Quoted 7 lines · Akinobu Mita
Quoted 2 lines

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

Quoted 3 lines

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

#8 lore raw
In reply to Akinobu Mita

include/linux/once.h line 9

Quoted 4 lines · Akinobu Mita

+++ 2.6-rc/include/linux/once.h

include/linux/once.h new file +3 -0
+struct once_control {
+	struct mutex lock;
+	int done;

bool?

include/linux/once.h line 17

Quoted 8 lines
include/linux/once.h new file +8 -0
+};
+
+#define __ONCE_INITIALIZER(name) {					\
+		.lock		= __MUTEX_INITIALIZER(name.lock),	\
+		.done		= 0,					\
+	}
+
+#define DEFINE_ONCE(name) struct once_control name = __ONCE_INITIALIZER(name)

static?

include/linux/once.h line 20

Quoted 3 lines
include/linux/once.h new file +3 -0
+
+extern int call_once_slow(struct once_control *once_control,
+			  int (*init_rouine)(void));

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

#9 lore raw

2008/3/11, Joe Perches <[email protected]>:

Quoted 9 lines · Akinobu Mita

include/linux/once.h line 9

Quoted 5 lines

+++ 2.6-rc/include/linux/once.h

include/linux/once.h new file +3 -0
+struct once_control {
+     struct mutex lock;
+     int done;

bool?

Yes, this definetly should be bool.

Quoted 11 lines

include/linux/once.h line 17

Quoted 8 lines
include/linux/once.h new file +8 -0
+};
+
+#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.

Quoted 6 lines

include/linux/once.h line 20

Quoted 3 lines
include/linux/once.h new file +3 -0
+
+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.

Quoted 1 line

spelling: s/rouine/routine/g

Oops, thanks.

maintainer
#10 lore raw
In reply to Akinobu Mita
Quoted 12 lines · On Mon, 10 Mar 2008 23:57:05 +0900 Akinobu Mita <[email protected]> wrote:

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

include/linux/once.h new file +9 -0
+/*
+ * 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

include/linux/once.h line 40

Quoted 10 lines
include/linux/once.h new file +10 -0
+ * 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();

?

#11 lore raw
Quoted 1 line · Andrew Morton

On Mon, 10 Mar 2008 23:57:05 +0900 Akinobu Mita <[email protected]>

Quoted 38 lines · wrote:
Quoted 15 lines

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.
+ */

include/linux/once.h line 40

Quoted 7 lines
include/linux/once.h new file +7 -0
+
+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);	\
	})
Quoted 6 lines

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.

maintainer
#12 lore raw
Quoted 43 lines · On Tue, 11 Mar 2008 15:10:52 +1100 Nick Piggin <[email protected]> wrote:
Quoted 1 line · Andrew Morton

On Mon, 10 Mar 2008 23:57:05 +0900 Akinobu Mita <[email protected]>

Quoted 40 lines · wrote:
Quoted 15 lines

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.
+ */

include/linux/once.h line 40

Quoted 7 lines
include/linux/once.h new file +7 -0
+
+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

Quoted 8 lines
Quoted 4 lines
	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 ;)

#13 lore raw

2008/3/11, Andrew Morton <[email protected]>:

Quoted 43 lines · On Tue, 11 Mar 2008 15:10:52 +1100 Nick Piggin <[email protected]> wrote:
Quoted 42 lines · Andrew Morton
Quoted 1 line

On Mon, 10 Mar 2008 23:57:05 +0900 Akinobu Mita <[email protected]>

Quoted 38 lines · wrote:
Quoted 15 lines

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.
+ */

include/linux/once.h line 40

Quoted 7 lines
include/linux/once.h new file +7 -0
+
+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);      \
    })
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

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.

Quoted 8 lines
Quoted 3 lines

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...

maintainer
#14 lore raw
Quoted 12 lines · On Tue, 11 Mar 2008 21:27:30 +0900 "Akinobu Mita" <[email protected]>…

2008/3/11, Andrew Morton <[email protected]>:

Quoted 11 lines · On Tue, 11 Mar 2008 15:10:52 +1100 Nick Piggin <[email protected]> wrote:
Quoted 10 lines
Quoted 10 lines
#define ONCE()                                              \
    ({                                              \
            static long flag;                       \
                                                    \
            return !test_and_set_bit(0, flag);      \
    })

and then callers can do

Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
    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:

lib/idr.c renamed from lib/idr.c~a +3 -9
@@ -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);
 }
#15 lore raw
Quoted 6 lines · Andrew Morton
#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);

maintainer
#16 lore raw
Quoted 9 lines · On Tue, 11 Mar 2008 11:56:55 -0700 Joe Perches <[email protected]> wrote:
Quoted 6 lines · Andrew Morton
#define ONCE()                                              \
    ({                                              \
            static long flag;                       \
                                                    \
            return !test_and_set_bit(0, flag);      \
    })

test_and_set_bit takes an address

duh.

Quoted 6 lines

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.

#17 lore raw
In reply to Andrew Morton
Quoted 22 lines

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

lib/idr.c renamed from lib/idr.c~a +0 -8
@@ -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

lib/idr.c static int init_id_cache(void) · line 602

Quoted 10 lines
lib/idr.c renamed from lib/idr.c~a +3 -1
@@ -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.

---
 include/linux/idr.h |    2 ++
 init/main.c         |    2 ++
 lib/idr.c           |   10 ++++------
 3 files changed, 8 insertions(+), 6 deletions(-)
lib/idr.c +4 -6
Index: 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);
 }
include/linux/idr.h +2 -0
Index: 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__ */
init/main.c +2 -0
Index: 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)
#18 lore raw
In reply to Akinobu Mita
Quoted 21 lines · Akinobu Mita
include/linux/once.h new file +8 -0
+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
===================================================================

lib/once.c new file +4 -0
@@ -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)) +{

lib/once.c new file +3 -0
+	int err = 0;
+
+	mutex_lock(&once_control->lock);

lib/once.c line 18

Quoted 10 lines
lib/once.c new file +10 -0
+	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