[PATCH] Use ilog2() in fs/namespace.c

[PATCH] Use ilog2() in fs/namespace.c

[Eric Dumazet]

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

We can use ilog2() in fs/namespace.c to compute hash_bits and hash_mask at
compile time, not runtime.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>


[-- Attachment #2: namespace.patch --]
[-- Type: text/plain, Size: 1416 bytes --]

diff --git a/fs/namespace.c b/fs/namespace.c
index 0608388..835f14a 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -25,6 +25,7 @@
 #include <linux/security.h>
 #include <linux/mount.h>
 #include <linux/ramfs.h>
+#include <linux/log2.h>
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
 #include "pnode.h"
@@ -36,7 +37,8 @@ __cacheline_aligned_in_smp DEFINE_SPINLOCK(vfsmount_lock);
 static int event;
 
 static struct list_head *mount_hashtable __read_mostly;
-static int hash_mask __read_mostly, hash_bits __read_mostly;
+#define hash_bits ilog2(PAGE_SIZE / sizeof(struct list_head))
+#define hash_mask ((1UL << hash_bits) - 1)
 static struct kmem_cache *mnt_cache __read_mostly;
 static struct rw_semaphore namespace_sem;
 
@@ -1828,24 +1830,7 @@ void __init mnt_init(void)
 	if (!mount_hashtable)
 		panic("Failed to allocate mount hash table\n");
 
-	/*
-	 * Find the power-of-two list-heads that can fit into the allocation..
-	 * We don't guarantee that "sizeof(struct list_head)" is necessarily
-	 * a power-of-two.
-	 */
-	nr_hash = PAGE_SIZE / sizeof(struct list_head);
-	hash_bits = 0;
-	do {
-		hash_bits++;
-	} while ((nr_hash >> hash_bits) != 0);
-	hash_bits--;
-
-	/*
-	 * Re-calculate the actual number of entries and the mask
-	 * from the number of bits we can fit.
-	 */
 	nr_hash = 1UL << hash_bits;
-	hash_mask = nr_hash - 1;
 
 	printk("Mount-cache hash table entries: %d\n", nr_hash);
 

Re: [PATCH] Use ilog2() in fs/namespace.c

[Andrew Morton]

On Fri, 14 Dec 2007 18:51:06 +0100 Eric Dumazet <dada1@cosmosbay.com> wrote:

> We can use ilog2() in fs/namespace.c to compute hash_bits and hash_mask at
> compile time, not runtime.

Well noted.

> [namespace.patch  text/plain (1.4KB)]

argh. (save-as, read, copy-paste, s/^/> /g)

> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -25,6 +25,7 @@
>  #include <linux/security.h>
>  #include <linux/mount.h>
>  #include <linux/ramfs.h>
> +#include <linux/log2.h>
>  #include <asm/uaccess.h>
>  #include <asm/unistd.h>
>  #include "pnode.h"
> @@ -36,7 +37,8 @@ __cacheline_aligned_in_smp DEFINE_SPINLOCK(vfsmount_lock);
>  static int event;
>  
>  static struct list_head *mount_hashtable __read_mostly;
> -static int hash_mask __read_mostly, hash_bits __read_mostly;
> +#define hash_bits ilog2(PAGE_SIZE / sizeof(struct list_head))
> +#define hash_mask ((1UL << hash_bits) - 1)
>  static struct kmem_cache *mnt_cache __read_mostly;
>  static struct rw_semaphore namespace_sem;
>  
> @@ -1828,24 +1830,7 @@ void __init mnt_init(void)
>  	if (!mount_hashtable)
>  		panic("Failed to allocate mount hash table\n");
>  
> -	/*
> -	 * Find the power-of-two list-heads that can fit into the allocation..
> -	 * We don't guarantee that "sizeof(struct list_head)" is necessarily
> -	 * a power-of-two.
> -	 */
> -	nr_hash = PAGE_SIZE / sizeof(struct list_head);
> -	hash_bits = 0;
> -	do {
> -		hash_bits++;
> -	} while ((nr_hash >> hash_bits) != 0);
> -	hash_bits--;
> -
> -	/*
> -	 * Re-calculate the actual number of entries and the mask
> -	 * from the number of bits we can fit.
> -	 */
>  	nr_hash = 1UL << hash_bits;
> -	hash_mask = nr_hash - 1;
>  
>  	printk("Mount-cache hash table entries: %d\n", nr_hash);

Those #defines you now have there are foul.  Please, when there's a choice
between doing it minimally and doing it right, let's do it right?

Look what we can now do:


--- a/fs/namespace.c~use-ilog2-in-fs-namespacec-fix
+++ a/fs/namespace.c
@@ -31,14 +31,15 @@
 #include "pnode.h"
 #include "internal.h"
 
+#define HASH_SHIFT ilog2(PAGE_SIZE / sizeof(struct list_head))
+#define HASH_SIZE (1UL << HASH_SHIFT)
+
 /* spinlock for vfsmount related operations, inplace of dcache_lock */
 __cacheline_aligned_in_smp DEFINE_SPINLOCK(vfsmount_lock);
 
 static int event;
 
 static struct list_head *mount_hashtable __read_mostly;
-#define hash_bits ilog2(PAGE_SIZE / sizeof(struct list_head))
-#define hash_mask ((1UL << hash_bits) - 1)
 static struct kmem_cache *mnt_cache __read_mostly;
 static struct rw_semaphore namespace_sem;
 
@@ -50,8 +51,8 @@ static inline unsigned long hash(struct 
 {
 	unsigned long tmp = ((unsigned long)mnt / L1_CACHE_BYTES);
 	tmp += ((unsigned long)dentry / L1_CACHE_BYTES);
-	tmp = tmp + (tmp >> hash_bits);
-	return tmp & hash_mask;
+	tmp = tmp + (tmp >> HASH_SHIFT);
+	return tmp & (HASH_SIZE - 1);
 }
 
 struct vfsmount *alloc_vfsmnt(const char *name)
@@ -1815,9 +1816,7 @@ static void __init init_mount_tree(void)
 
 void __init mnt_init(void)
 {
-	struct list_head *d;
-	unsigned int nr_hash;
-	int i;
+	unsigned u;
 	int err;
 
 	init_rwsem(&namespace_sem);
@@ -1830,18 +1829,11 @@ void __init mnt_init(void)
 	if (!mount_hashtable)
 		panic("Failed to allocate mount hash table\n");
 
-	nr_hash = 1UL << hash_bits;
+	printk("Mount-cache hash table entries: %lu\n", HASH_SIZE);
 
-	printk("Mount-cache hash table entries: %d\n", nr_hash);
+	for (u = 0; u < HASH_SIZE; u++)
+		INIT_LIST_HEAD(&mount_hashtable[u]);
 
-	/* And initialize the newly allocated array */
-	d = mount_hashtable;
-	i = nr_hash;
-	do {
-		INIT_LIST_HEAD(d);
-		d++;
-		i--;
-	} while (i);
 	err = sysfs_init();
 	if (err)
 		printk(KERN_WARNING "%s: sysfs_init error: %d\n",
_

Re: [PATCH] Use ilog2() in fs/namespace.c

[Eric Dumazet]

On Fri, 21 Dec 2007 02:29:12 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 14 Dec 2007 18:51:06 +0100 Eric Dumazet <dada1@cosmosbay.com> wrote:
> 
> > We can use ilog2() in fs/namespace.c to compute hash_bits and hash_mask at
> > compile time, not runtime.
> 
> Well noted.
> 
> > [namespace.patch  text/plain (1.4KB)]
> 
> argh. (save-as, read, copy-paste, s/^/> /g)
> 
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/security.h>
> >  #include <linux/mount.h>
> >  #include <linux/ramfs.h>
> > +#include <linux/log2.h>
> >  #include <asm/uaccess.h>
> >  #include <asm/unistd.h>
> >  #include "pnode.h"
> > @@ -36,7 +37,8 @@ __cacheline_aligned_in_smp DEFINE_SPINLOCK(vfsmount_lock);
> >  static int event;
> >  
> >  static struct list_head *mount_hashtable __read_mostly;
> > -static int hash_mask __read_mostly, hash_bits __read_mostly;
> > +#define hash_bits ilog2(PAGE_SIZE / sizeof(struct list_head))
> > +#define hash_mask ((1UL << hash_bits) - 1)
> >  static struct kmem_cache *mnt_cache __read_mostly;
> >  static struct rw_semaphore namespace_sem;
> >  
> > @@ -1828,24 +1830,7 @@ void __init mnt_init(void)
> >  	if (!mount_hashtable)
> >  		panic("Failed to allocate mount hash table\n");
> >  
> > -	/*
> > -	 * Find the power-of-two list-heads that can fit into the allocation..
> > -	 * We don't guarantee that "sizeof(struct list_head)" is necessarily
> > -	 * a power-of-two.
> > -	 */
> > -	nr_hash = PAGE_SIZE / sizeof(struct list_head);
> > -	hash_bits = 0;
> > -	do {
> > -		hash_bits++;
> > -	} while ((nr_hash >> hash_bits) != 0);
> > -	hash_bits--;
> > -
> > -	/*
> > -	 * Re-calculate the actual number of entries and the mask
> > -	 * from the number of bits we can fit.
> > -	 */
> >  	nr_hash = 1UL << hash_bits;
> > -	hash_mask = nr_hash - 1;
> >  
> >  	printk("Mount-cache hash table entries: %d\n", nr_hash);
> 
> Those #defines you now have there are foul.  Please, when there's a choice
> between doing it minimally and doing it right, let's do it right?
>

Indeed !

Thanks Andrew for taking the time and fixing my lazyness :(
Your version is much cleaner of course.

 
> Look what we can now do:
> 
> 
> --- a/fs/namespace.c~use-ilog2-in-fs-namespacec-fix
> +++ a/fs/namespace.c
> @@ -31,14 +31,15 @@
>  #include "pnode.h"
>  #include "internal.h"
>  
> +#define HASH_SHIFT ilog2(PAGE_SIZE / sizeof(struct list_head))
> +#define HASH_SIZE (1UL << HASH_SHIFT)
> +
>  /* spinlock for vfsmount related operations, inplace of dcache_lock */
>  __cacheline_aligned_in_smp DEFINE_SPINLOCK(vfsmount_lock);
>  
>  static int event;
>  
>  static struct list_head *mount_hashtable __read_mostly;
> -#define hash_bits ilog2(PAGE_SIZE / sizeof(struct list_head))
> -#define hash_mask ((1UL << hash_bits) - 1)
>  static struct kmem_cache *mnt_cache __read_mostly;
>  static struct rw_semaphore namespace_sem;
>  
> @@ -50,8 +51,8 @@ static inline unsigned long hash(struct 
>  {
>  	unsigned long tmp = ((unsigned long)mnt / L1_CACHE_BYTES);
>  	tmp += ((unsigned long)dentry / L1_CACHE_BYTES);
> -	tmp = tmp + (tmp >> hash_bits);
> -	return tmp & hash_mask;
> +	tmp = tmp + (tmp >> HASH_SHIFT);
> +	return tmp & (HASH_SIZE - 1);
>  }
>  
>  struct vfsmount *alloc_vfsmnt(const char *name)
> @@ -1815,9 +1816,7 @@ static void __init init_mount_tree(void)
>  
>  void __init mnt_init(void)
>  {
> -	struct list_head *d;
> -	unsigned int nr_hash;
> -	int i;
> +	unsigned u;
>  	int err;
>  
>  	init_rwsem(&namespace_sem);
> @@ -1830,18 +1829,11 @@ void __init mnt_init(void)
>  	if (!mount_hashtable)
>  		panic("Failed to allocate mount hash table\n");
>  
> -	nr_hash = 1UL << hash_bits;
> +	printk("Mount-cache hash table entries: %lu\n", HASH_SIZE);
>  
> -	printk("Mount-cache hash table entries: %d\n", nr_hash);
> +	for (u = 0; u < HASH_SIZE; u++)
> +		INIT_LIST_HEAD(&mount_hashtable[u]);
>  
> -	/* And initialize the newly allocated array */
> -	d = mount_hashtable;
> -	i = nr_hash;
> -	do {
> -		INIT_LIST_HEAD(d);
> -		d++;
> -		i--;
> -	} while (i);
>  	err = sysfs_init();
>  	if (err)
>  		printk(KERN_WARNING "%s: sysfs_init error: %d\n",
> _
>