Skip to contentSkip to conversation

Fix panic in __d_lookup with high dentry hashtable counts

PATCH In review Dimitri Sivanich 4 participants 12 messages Jan 13, 2012 – Jan 18, 2012

Subsystems (8)
Networking Network drivers Memory management VFS +4 more subsystems
lore
Share & tools
lore

Dimitri Sivanich

author
#0 lore raw

PATCH 1/1 Fix panic in __d_lookup with high dentry hashtable counts

When the number of dentry cache hash table entries gets too high (2147483648 entries), use of a signed integer in the initialization loop prevents the dentry_hashtable from getting initialized, resulting in a panic in __d_lookup. Fixing this in dcache_init and a few other spots for consistency.

Signed-off-by Dimitri Sivanich <[email protected]>
---
 fs/dcache.c    |    8 ++++----
 fs/inode.c     |    8 ++++----
 kernel/pid.c   |    4 ++--
 net/ipv4/tcp.c |    3 ++-
 4 files changed, 12 insertions(+), 11 deletions(-)
fs/dcache.c +4 -4
Index: linux/fs/dcache.c
===================================================================
@@ -2968,7 +2968,7 @@ __setup("dhash_entries=", set_dhash_entr
 
 static void __init dcache_init_early(void)
 {
-	int loop;
+	long loop;
 
 	/* If hashes are distributed across NUMA nodes, defer
 	 * hash allocation until vmalloc space is available.

@@ -2986,13 +2986,13 @@ static void __init dcache_init_early(voi
 					&d_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << d_hash_shift); loop++)
+	for (loop = 0; loop < (1L << d_hash_shift); loop++)
 		INIT_HLIST_BL_HEAD(dentry_hashtable + loop);
 }
 
 static void __init dcache_init(void)
 {
-	int loop;
+	long loop;
 
 	/* 
 	 * A constructor could be added for stable state like the lists,

@@ -3016,7 +3016,7 @@ static void __init dcache_init(void)
 					&d_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << d_hash_shift); loop++)
+	for (loop = 0; loop < (1L << d_hash_shift); loop++)
 		INIT_HLIST_BL_HEAD(dentry_hashtable + loop);
 }
fs/inode.c +4 -4
Index: linux/fs/inode.c
===================================================================
@@ -1654,7 +1654,7 @@ __setup("ihash_entries=", set_ihash_entr
  */
 void __init inode_init_early(void)
 {
-	int loop;
+	long loop;
 
 	/* If hashes are distributed across NUMA nodes, defer
 	 * hash allocation until vmalloc space is available.

@@ -1672,13 +1672,13 @@ void __init inode_init_early(void)
 					&i_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << i_hash_shift); loop++)
+	for (loop = 0; loop < (1L << i_hash_shift); loop++)
 		INIT_HLIST_HEAD(&inode_hashtable[loop]);
 }
 
 void __init inode_init(void)
 {
-	int loop;
+	long loop;
 
 	/* inode slab cache */
 	inode_cachep = kmem_cache_create("inode_cache",

@@ -1702,7 +1702,7 @@ void __init inode_init(void)
 					&i_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << i_hash_shift); loop++)
+	for (loop = 0; loop < (1L << i_hash_shift); loop++)
 		INIT_HLIST_HEAD(&inode_hashtable[loop]);
 }
kernel/pid.c +2 -2
Index: linux/kernel/pid.c
===================================================================
@@ -543,12 +543,12 @@ struct pid *find_ge_pid(int nr, struct p
  */
 void __init pidhash_init(void)
 {
-	int i, pidhash_size;
+	long i, pidhash_size;
 
 	pid_hash = alloc_large_system_hash("PID", sizeof(*pid_hash), 0, 18,
 					   HASH_EARLY | HASH_SMALL,
 					   &pidhash_shift, NULL, 4096);
-	pidhash_size = 1 << pidhash_shift;
+	pidhash_size = 1L << pidhash_shift;
 
 	for (i = 0; i < pidhash_size; i++)
 		INIT_HLIST_HEAD(&pid_hash[i]);
net/ipv4/tcp.c +2 -1
Index: linux/net/ipv4/tcp.c
===================================================================
@@ -3220,7 +3220,8 @@ void __init tcp_init(void)
 {
 	struct sk_buff *skb = NULL;
 	unsigned long limit;
-	int i, max_share, cnt;
+	long i;
+	int max_share, cnt;
 	unsigned long jiffy = jiffies;
 
 	BUILD_BUG_ON(sizeof(struct tcp_skb_cb) > sizeof(skb->cb));
#1 lore raw
Quoted 5 lines · Dimitri Sivanich

When the number of dentry cache hash table entries gets too high (2147483648 entries), use of a signed integer in the initialization loop prevents the dentry_hashtable from getting initialized, resulting in a panic in __d_lookup. Fixing this in dcache_init and a few other spots for consistency.

Well...

nr_dentry being an int, I dont think having a so big hash table is needed/possible. Its probably a waste of memory ?

Maybe we should limit alloc_large_system_hash() to at most 2^30 slots.

[ And later, convert it to unsigned long *_hash_shift and unsigned long *_hash_mask ]

maintainer
#2 lore raw
In reply to Dimitri Sivanich

Review

Quoted 10 lines · Dimitri Sivanich

When the number of dentry cache hash table entries gets too high (2147483648 entries), use of a signed integer in the initialization loop prevents the dentry_hashtable from getting initialized, resulting in a panic in __d_lookup. Fixing this in dcache_init and a few other spots for consistency.

static void __init dcache_init(void)
{

(quoted diff) +1 -1
-	int loop;
+	long loop;

You've got to be kidding. Note that D_HASHMASK is at most 32bit. Use of long here is an overkill and so's 2^31 hash buckets (that's what, 16Gb in hash list heads alone? What kind of average chain length do you expect, BTW?)

Can alloc_large_system_hash() produce the horrors that large, anyway?

author
#3 lore raw
Quoted 7 lines · Al Viro
Quoted 5 lines · Dimitri Sivanich

When the number of dentry cache hash table entries gets too high (2147483648 entries), use of a signed integer in the initialization loop prevents the dentry_hashtable from getting initialized, resulting in a panic in __d_lookup. Fixing this in dcache_init and a few other spots for consistency.

Quoted 9 lines

Review

Quoted 4 lines

static void __init dcache_init(void)
{

(quoted diff) +1 -1
-	int loop;
+	long loop;

You've got to be kidding. Note that D_HASHMASK is at most 32bit. Use of long here is an overkill and so's 2^31 hash buckets (that's what, 16Gb in hash list heads alone? What kind of average chain length do you expect, BTW?)

Yes, long might be overkill right now, but the code is all __init time code. I don't have numbers showing average chain length at this point, I was simply fixing this one end case

Quoted 2 lines

Can alloc_large_system_hash() produce the horrors that large, anyway?

On a 16TB system, alloc_large_system_hash() produces 2^31 hash buckets, yes.

Would simply capping the value in alloc_large_system_hash() be more palatable?

Something like the following?

mm/page_alloc.c +1 -0
Index: linux/mm/page_alloc.c
===================================================================
@@ -5257,6 +5257,7 @@ void *__init alloc_large_system_hash(con
        if (max == 0) {
                max = ((unsigned long long)nr_all_pages << PAGE_SHIFT) >> 4;
                do_div(max, bucketsize);
+               max = min(max, 1ULL << 30);
        }
 
        if (numentries > max)
author
#4 lore raw
Quoted 36 lines · Dimitri Sivanich
Quoted 7 lines · Al Viro
Quoted 5 lines · Dimitri Sivanich

When the number of dentry cache hash table entries gets too high (2147483648 entries), use of a signed integer in the initialization loop prevents the dentry_hashtable from getting initialized, resulting in a panic in __d_lookup. Fixing this in dcache_init and a few other spots for consistency.

Quoted 9 lines

Review

Quoted 4 lines

static void __init dcache_init(void)
{

(quoted diff) +1 -1
-	int loop;
+	long loop;

You've got to be kidding. Note that D_HASHMASK is at most 32bit. Use of long here is an overkill and so's 2^31 hash buckets (that's what, 16Gb in hash list heads alone? What kind of average chain length do you expect, BTW?)

Yes, long might be overkill right now, but the code is all __init time code. I don't have numbers showing average chain length at this point, I was simply fixing this one end case

Quoted 2 lines

Can alloc_large_system_hash() produce the horrors that large, anyway?

On a 16TB system, alloc_large_system_hash() produces 2^31 hash buckets, yes.

Would simply capping the value in alloc_large_system_hash() be more palatable?

Something like the following?

Index: linux/mm/page_alloc.c
===================================================================

mm/page_alloc.c void *__init alloc_large_system_hash(con · line 5263

Quoted 7 lines
mm/page_alloc.c +1 -0
@@ -5257,6 +5257,7 @@ void *__init alloc_large_system_hash(con
        if (max == 0) {
                max = ((unsigned long long)nr_all_pages << PAGE_SHIFT) >> 4;
                do_div(max, bucketsize);
+               max = min(max, 1ULL << 30);
        }
 
        if (numentries > max)

Sorry, you'd probably want something more like this:

mm/page_alloc.c +1 -0
Index: linux/mm/page_alloc.c
===================================================================
@@ -5258,6 +5258,7 @@ void *__init alloc_large_system_hash(con
                max = ((unsigned long long)nr_all_pages << PAGE_SHIFT) >> 4;
                do_div(max, bucketsize);
        }
+       max = min(max, 1ULL << 30);
 
        if (numentries > max)
                numentries = max;
author
#5 lore raw

PATCH 1/1 Fix panic in __d_lookup with high dentry hashtable counts

In reply to Al Viro

When the number of dentry cache hash table entries gets too high (2147483648 entries), as happens by default on a 16TB system, use of a signed integer in the dcache_init() initialization loop prevents the dentry_hashtable from getting initialized, causing a panic in __d_lookup().

In addition, the _hash_mask returned from alloc_large_system_hash() does not support more than a 32 bit hash table size.

Changing the _hash_mask size returned from alloc_large_system_hash() to support larger hash table sizes in the future, and changing loop counter sizes appropriately.

Signed-off-by Dimitri Sivanich <[email protected]>
---
 fs/dcache.c             |   10 +++++-----
 fs/inode.c              |   10 +++++-----
 include/linux/bootmem.h |    2 +-
 mm/page_alloc.c         |    2 +-
 net/ipv4/route.c        |    8 ++++++--
 net/ipv4/tcp.c          |   13 +++++++++----
 net/ipv4/udp.c          |    5 ++++-
 7 files changed, 31 insertions(+), 19 deletions(-)
fs/dcache.c +5 -5
Index: linux/fs/dcache.c
===================================================================
@@ -99,7 +99,7 @@ static struct kmem_cache *dentry_cache _
 #define D_HASHBITS     d_hash_shift
 #define D_HASHMASK     d_hash_mask
 
-static unsigned int d_hash_mask __read_mostly;
+static unsigned long d_hash_mask __read_mostly;
 static unsigned int d_hash_shift __read_mostly;
 
 static struct hlist_bl_head *dentry_hashtable __read_mostly;

@@ -2968,7 +2968,7 @@ __setup("dhash_entries=", set_dhash_entr
 
 static void __init dcache_init_early(void)
 {
-	int loop;
+	unsigned long loop;
 
 	/* If hashes are distributed across NUMA nodes, defer
 	 * hash allocation until vmalloc space is available.

@@ -2986,13 +2986,13 @@ static void __init dcache_init_early(voi
 					&d_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << d_hash_shift); loop++)
+	for (loop = 0; loop < (1UL << d_hash_shift); loop++)
 		INIT_HLIST_BL_HEAD(dentry_hashtable + loop);
 }
 
 static void __init dcache_init(void)
 {
-	int loop;
+	unsigned long loop;
 
 	/* 
 	 * A constructor could be added for stable state like the lists,

@@ -3016,7 +3016,7 @@ static void __init dcache_init(void)
 					&d_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << d_hash_shift); loop++)
+	for (loop = 0; loop < (1UL << d_hash_shift); loop++)
 		INIT_HLIST_BL_HEAD(dentry_hashtable + loop);
 }
fs/inode.c +5 -5
Index: linux/fs/inode.c
===================================================================
@@ -60,7 +60,7 @@
  *   inode_hash_lock
  */
 
-static unsigned int i_hash_mask __read_mostly;
+static unsigned long i_hash_mask __read_mostly;
 static unsigned int i_hash_shift __read_mostly;
 static struct hlist_head *inode_hashtable __read_mostly;
 static __cacheline_aligned_in_smp DEFINE_SPINLOCK(inode_hash_lock);

@@ -1654,7 +1654,7 @@ __setup("ihash_entries=", set_ihash_entr
  */
 void __init inode_init_early(void)
 {
-	int loop;
+	unsigned long loop;
 
 	/* If hashes are distributed across NUMA nodes, defer
 	 * hash allocation until vmalloc space is available.

@@ -1672,13 +1672,13 @@ void __init inode_init_early(void)
 					&i_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << i_hash_shift); loop++)
+	for (loop = 0; loop < (1UL << i_hash_shift); loop++)
 		INIT_HLIST_HEAD(&inode_hashtable[loop]);
 }
 
 void __init inode_init(void)
 {
-	int loop;
+	unsigned long loop;
 
 	/* inode slab cache */
 	inode_cachep = kmem_cache_create("inode_cache",

@@ -1702,7 +1702,7 @@ void __init inode_init(void)
 					&i_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << i_hash_shift); loop++)
+	for (loop = 0; loop < (1UL << i_hash_shift); loop++)
 		INIT_HLIST_HEAD(&inode_hashtable[loop]);
 }
include/linux/bootmem.h +1 -1
Index: linux/include/linux/bootmem.h
===================================================================
@@ -153,7 +153,7 @@ extern void *alloc_large_system_hash(con
 				     int scale,
 				     int flags,
 				     unsigned int *_hash_shift,
-				     unsigned int *_hash_mask,
+				     unsigned long *_hash_mask,
 				     unsigned long limit);
 
 #define HASH_EARLY	0x00000001	/* Allocating during early boot? */
mm/page_alloc.c +1 -1
Index: linux/mm/page_alloc.c
===================================================================
@@ -5219,7 +5219,7 @@ void *__init alloc_large_system_hash(con
 				     int scale,
 				     int flags,
 				     unsigned int *_hash_shift,
-				     unsigned int *_hash_mask,
+				     unsigned long *_hash_mask,
 				     unsigned long limit)
 {
 	unsigned long long max = limit;
net/ipv4/route.c +6 -2
Index: linux/net/ipv4/route.c
===================================================================
@@ -3446,6 +3446,7 @@ __setup("rhash_entries=", set_rhash_entr
 
 int __init ip_rt_init(void)
 {
+	unsigned long hash_mask;
 	int rc = 0;
 
 #ifdef CONFIG_IP_ROUTE_CLASSID

@@ -3474,8 +3475,11 @@ int __init ip_rt_init(void)
 					15 : 17,
 					0,
 					&rt_hash_log,
-					&rt_hash_mask,
-					rhash_entries ? 0 : 512 * 1024);
+					&hash_mask,
+					rhash_entries ? 0x80000000 :
+								512 * 1024);
+	/* FIXME: Above limit value (0x80000000) allows the following cast. */
+	rt_hash_mask = (unsigned int) hash_mask;
 	memset(rt_hash_table, 0, (rt_hash_mask + 1) * sizeof(struct rt_hash_bucket));
 	rt_hash_lock_init();
net/ipv4/tcp.c +9 -4
Index: linux/net/ipv4/tcp.c
===================================================================
@@ -3219,8 +3219,9 @@ __setup("thash_entries=", set_thash_entr
 void __init tcp_init(void)
 {
 	struct sk_buff *skb = NULL;
-	unsigned long limit;
-	int i, max_share, cnt;
+	unsigned long limit, hash_mask;
+	unsigned i;
+	int max_share, cnt;
 	unsigned long jiffy = jiffies;
 
 	BUILD_BUG_ON(sizeof(struct tcp_skb_cb) > sizeof(skb->cb));

@@ -3245,8 +3246,12 @@ void __init tcp_init(void)
 					13 : 15,
 					0,
 					NULL,
-					&tcp_hashinfo.ehash_mask,
-					thash_entries ? 0 : 512 * 1024);
+					&hash_mask,
+					thash_entries ? 0x80000000 :
+								512 * 1024);
+	/* FIXME: Above limit value (0x80000000) allows the following cast. */
+	tcp_hashinfo.ehash_mask = (unsigned int)hash_mask;
+
 	for (i = 0; i <= tcp_hashinfo.ehash_mask; i++) {
 		INIT_HLIST_NULLS_HEAD(&tcp_hashinfo.ehash[i].chain, i);
 		INIT_HLIST_NULLS_HEAD(&tcp_hashinfo.ehash[i].twchain, i);
net/ipv4/udp.c +4 -1
Index: linux/net/ipv4/udp.c
===================================================================
@@ -2180,6 +2180,7 @@ __setup("uhash_entries=", set_uhash_entr
 
 void __init udp_table_init(struct udp_table *table, const char *name)
 {
+	unsigned long hash_mask;
 	unsigned int i;
 
 	if (!CONFIG_BASE_SMALL)

@@ -2189,8 +2190,10 @@ void __init udp_table_init(struct udp_ta
 			21, /* one slot per 2 MB */
 			0,
 			&table->log,
-			&table->mask,
+			&hash_mask,
 			64 * 1024);
+	/* FIXME: Above limit value (64 * 1024) allows the following cast. */
+	table->mask = (unsigned int)hash_mask;
 	/*
 	 * Make sure hash table has the minimum size
 	 */
maintainer
#6 lore raw

From: Dimitri Sivanich <[email protected]>
Date: Tue, 17 Jan 2012 11:13:52 -0600

Quoted 6 lines

When the number of dentry cache hash table entries gets too high (2147483648 entries), as happens by default on a 16TB system, use of a signed integer in the dcache_init() initialization loop prevents the dentry_hashtable from getting initialized, causing a panic in __d_lookup().

Quoted 8 lines

In addition, the _hash_mask returned from alloc_large_system_hash() does not support more than a 32 bit hash table size.

Changing the _hash_mask size returned from alloc_large_system_hash() to support larger hash table sizes in the future, and changing loop counter sizes appropriately.

Signed-off-by Dimitri Sivanich <[email protected]>

To be honest I think this is overkill.

Supporting anything larger than a 32-bit hash mask is not even close to being reasonable. Nobody needs a 4GB hash table, not for anything.

Instead I would just make sure everything is "unsigned int" or "u32" and calculations use things like "((u32) 1) << shift", and enforce an upper bounds of 0x80000000 or similar unconditionally in the hash allocator itself (rather than conditionally in the networking code).

All of this "long" stuff is madness, what the heck is a long? It's a non-fixed type, yet you put constants in your code (0x80000000) which depend upon that type's size.

author
#7 lore raw
Quoted 12 lines · David Miller

From: Dimitri Sivanich <[email protected]>
Date: Tue, 17 Jan 2012 11:13:52 -0600

Quoted 9 lines

When the number of dentry cache hash table entries gets too high (2147483648 entries), as happens by default on a 16TB system, use of a signed integer in the dcache_init() initialization loop prevents the dentry_hashtable from getting initialized, causing a panic in __d_lookup().

In addition, the _hash_mask returned from alloc_large_system_hash() does not support more than a 32 bit hash table size.

Quoted 7 lines
Quoted 5 lines

Changing the _hash_mask size returned from alloc_large_system_hash() to support larger hash table sizes in the future, and changing loop counter sizes appropriately.

Signed-off-by Dimitri Sivanich <[email protected]>

To be honest I think this is overkill.

I'm not going to flat-out disagree with you. These would be huge hash tables. The thought was to make this __init code as flexible as possible.

Quoted 3 lines

Supporting anything larger than a 32-bit hash mask is not even close to being reasonable. Nobody needs a 4GB hash table, not for anything.

Yes, at this point that is likely true.

Quoted 5 lines

Instead I would just make sure everything is "unsigned int" or "u32" and calculations use things like "((u32) 1) << shift", and enforce an upper bounds of 0x80000000 or similar unconditionally in the hash allocator itself (rather than conditionally in the networking code).

OK. I had mentioned capping the value in alloc_large_system_hash() to 32 bits, but got no response to that proposal. I'll create a proper patch.

Quoted 4 lines

All of this "long" stuff is madness, what the heck is a long? It's a non-fixed type, yet you put constants in your code (0x80000000) which depend upon that type's size.

author
#8 lore raw
In reply to David Miller
Quoted 5 lines · David Miller

To be honest I think this is overkill.

Supporting anything larger than a 32-bit hash mask is not even close to being reasonable. Nobody needs a 4GB hash table, not for anything.

Here is a patch that keeps the 32-bit hash mask.

When the number of dentry cache hash table entries gets too high (2147483648 entries), as happens by default on a 16TB system, use of a signed integer in the dcache_init() initialization loop prevents the dentry_hashtable from getting initialized, causing a panic in __d_lookup(). Fix this in dcache_init() and similar areas.

Signed-off-by Dimitri Sivanich <[email protected]>
---
 fs/dcache.c     |    8 ++++----
 fs/inode.c      |    8 ++++----
 kernel/pid.c    |    4 ++--
 mm/page_alloc.c |    1 +
 net/ipv4/tcp.c  |    5 +++--
 5 files changed, 14 insertions(+), 12 deletions(-)
fs/dcache.c +4 -4
Index: linux/fs/dcache.c
===================================================================
@@ -2968,7 +2968,7 @@ __setup("dhash_entries=", set_dhash_entr
 
 static void __init dcache_init_early(void)
 {
-	int loop;
+	unsigned int loop;
 
 	/* If hashes are distributed across NUMA nodes, defer
 	 * hash allocation until vmalloc space is available.

@@ -2986,13 +2986,13 @@ static void __init dcache_init_early(voi
 					&d_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << d_hash_shift); loop++)
+	for (loop = 0; loop < (1U << d_hash_shift); loop++)
 		INIT_HLIST_BL_HEAD(dentry_hashtable + loop);
 }
 
 static void __init dcache_init(void)
 {
-	int loop;
+	unsigned int loop;
 
 	/* 
 	 * A constructor could be added for stable state like the lists,

@@ -3016,7 +3016,7 @@ static void __init dcache_init(void)
 					&d_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << d_hash_shift); loop++)
+	for (loop = 0; loop < (1U << d_hash_shift); loop++)
 		INIT_HLIST_BL_HEAD(dentry_hashtable + loop);
 }
fs/inode.c +4 -4
Index: linux/fs/inode.c
===================================================================
@@ -1654,7 +1654,7 @@ __setup("ihash_entries=", set_ihash_entr
  */
 void __init inode_init_early(void)
 {
-	int loop;
+	unsigned int loop;
 
 	/* If hashes are distributed across NUMA nodes, defer
 	 * hash allocation until vmalloc space is available.

@@ -1672,13 +1672,13 @@ void __init inode_init_early(void)
 					&i_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << i_hash_shift); loop++)
+	for (loop = 0; loop < (1U << i_hash_shift); loop++)
 		INIT_HLIST_HEAD(&inode_hashtable[loop]);
 }
 
 void __init inode_init(void)
 {
-	int loop;
+	unsigned int loop;
 
 	/* inode slab cache */
 	inode_cachep = kmem_cache_create("inode_cache",

@@ -1702,7 +1702,7 @@ void __init inode_init(void)
 					&i_hash_mask,
 					0);
 
-	for (loop = 0; loop < (1 << i_hash_shift); loop++)
+	for (loop = 0; loop < (1U << i_hash_shift); loop++)
 		INIT_HLIST_HEAD(&inode_hashtable[loop]);
 }
mm/page_alloc.c +1 -0
Index: linux/mm/page_alloc.c
===================================================================
@@ -5258,6 +5258,7 @@ void *__init alloc_large_system_hash(con
 		max = ((unsigned long long)nr_all_pages << PAGE_SHIFT) >> 4;
 		do_div(max, bucketsize);
 	}
+	max = min(max, 0x80000000ULL);
 
 	if (numentries > max)
 		numentries = max;
kernel/pid.c +2 -2
Index: linux/kernel/pid.c
===================================================================
@@ -543,12 +543,12 @@ struct pid *find_ge_pid(int nr, struct p
  */
 void __init pidhash_init(void)
 {
-	int i, pidhash_size;
+	unsigned int i, pidhash_size;
 
 	pid_hash = alloc_large_system_hash("PID", sizeof(*pid_hash), 0, 18,
 					   HASH_EARLY | HASH_SMALL,
 					   &pidhash_shift, NULL, 4096);
-	pidhash_size = 1 << pidhash_shift;
+	pidhash_size = 1U << pidhash_shift;
 
 	for (i = 0; i < pidhash_size; i++)
 		INIT_HLIST_HEAD(&pid_hash[i]);
net/ipv4/tcp.c +3 -2
Index: linux/net/ipv4/tcp.c
===================================================================
@@ -3220,7 +3220,8 @@ void __init tcp_init(void)
 {
 	struct sk_buff *skb = NULL;
 	unsigned long limit;
-	int i, max_share, cnt;
+	int max_share, cnt;
+	unsigned int i;
 	unsigned long jiffy = jiffies;
 
 	BUILD_BUG_ON(sizeof(struct tcp_skb_cb) > sizeof(skb->cb));

@@ -3263,7 +3264,7 @@ void __init tcp_init(void)
 					&tcp_hashinfo.bhash_size,
 					NULL,
 					64 * 1024);
-	tcp_hashinfo.bhash_size = 1 << tcp_hashinfo.bhash_size;
+	tcp_hashinfo.bhash_size = 1U << tcp_hashinfo.bhash_size;
 	for (i = 0; i < tcp_hashinfo.bhash_size; i++) {
 		spin_lock_init(&tcp_hashinfo.bhash[i].lock);
 		INIT_HLIST_HEAD(&tcp_hashinfo.bhash[i].chain);
maintainer Acked-by
#9 lore raw

From: Dimitri Sivanich <[email protected]>
Date: Tue, 17 Jan 2012 15:05:41 -0600

Quoted 10 lines · David Miller
Quoted 5 lines

To be honest I think this is overkill.

Supporting anything larger than a 32-bit hash mask is not even close to being reasonable. Nobody needs a 4GB hash table, not for anything.

Here is a patch that keeps the 32-bit hash mask.

Quoted 7 lines

When the number of dentry cache hash table entries gets too high (2147483648 entries), as happens by default on a 16TB system, use of a signed integer in the dcache_init() initialization loop prevents the dentry_hashtable from getting initialized, causing a panic in __d_lookup(). Fix this in dcache_init() and similar areas.

Signed-off-by Dimitri Sivanich <[email protected]>

This looks good to me, thanks Dimitri:

Acked-by David S. Miller <[email protected]>
maintainer
#10 lore raw
In reply to Dimitri Sivanich
Quoted 6 lines · Dimitri Sivanich

When the number of dentry cache hash table entries gets too high (2147483648 entries), as happens by default on a 16TB system, use of a signed integer in the dcache_init() initialization loop prevents the dentry_hashtable from getting initialized, causing a panic in __d_lookup().

Quoted 6 lines

In addition, the _hash_mask returned from alloc_large_system_hash() does not support more than a 32 bit hash table size.

Changing the _hash_mask size returned from alloc_large_system_hash() to support larger hash table sizes in the future, and changing loop counter sizes appropriately.

... and I still would like to see somebody familiar with uses of other hashes to comment on the desirability of such monsters. For dcache and icache it's absolutely certain to be worse than useless. We are talking about 4Gbuckets here...

maintainer
#11 lore raw

From: Al Viro <[email protected]>
Date: Tue, 17 Jan 2012 17:25:27 +0000

Quoted 10 lines · Dimitri Sivanich
Quoted 9 lines

When the number of dentry cache hash table entries gets too high (2147483648 entries), as happens by default on a 16TB system, use of a signed integer in the dcache_init() initialization loop prevents the dentry_hashtable from getting initialized, causing a panic in __d_lookup().

In addition, the _hash_mask returned from alloc_large_system_hash() does not support more than a 32 bit hash table size.

Quoted 8 lines
Quoted 3 lines

Changing the _hash_mask size returned from alloc_large_system_hash() to support larger hash table sizes in the future, and changing loop counter sizes appropriately.

... and I still would like to see somebody familiar with uses of other hashes to comment on the desirability of such monsters. For dcache and icache it's absolutely certain to be worse than useless. We are talking about 4Gbuckets here...

It's wrong in networking too.