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 <redacted>
---
fs/dcache.c | 8 ++++----
fs/inode.c | 8 ++++----
kernel/pid.c | 4 ++--
net/ipv4/tcp.c | 3 ++-
4 files changed, 12 insertions(+), 11 deletions(-)
Index: linux/fs/dcache.c
===================================================================
--- linux.orig/fs/dcache.c+++ linux/fs/dcache.c
@@ -2968,7 +2968,7 @@ __setup("dhash_entries=", set_dhash_entrstaticvoid__initdcache_init_early(void){-intloop;+longloop;/* If hashes are distributed across NUMA nodes, defer*hashallocationuntilvmallocspaceisavailable.
@@ -1654,7 +1654,7 @@ __setup("ihash_entries=", set_ihash_entr*/void__initinode_init_early(void){-intloop;+longloop;/* If hashes are distributed across NUMA nodes, defer*hashallocationuntilvmallocspaceisavailable.
From: Eric Dumazet <hidden> Date: 2012-01-13 16:15:46
Le vendredi 13 janvier 2012 à 09:52 -0600, Dimitri Sivanich a écrit :
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 ]
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2012-01-13 16:22:49
On Fri, Jan 13, 2012 at 09:52:37AM -0600, Dimitri Sivanich wrote:
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)
{
- 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?
On Fri, Jan 13, 2012 at 04:22:36PM +0000, Al Viro wrote:
On Fri, Jan 13, 2012 at 09:52:37AM -0600, Dimitri Sivanich wrote:
quoted
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
static void __init dcache_init(void)
{
- 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
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
===================================================================
On Fri, Jan 13, 2012 at 10:36:42AM -0600, Dimitri Sivanich wrote:
quoted hunk
On Fri, Jan 13, 2012 at 04:22:36PM +0000, Al Viro wrote:
quoted
On Fri, Jan 13, 2012 at 09:52:37AM -0600, Dimitri Sivanich wrote:
quoted
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
static void __init dcache_init(void)
{
- 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
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
===================================================================
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 <redacted>
---
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(-)
Index: linux/fs/dcache.c
===================================================================
@@ -2968,7 +2968,7 @@ __setup("dhash_entries=", set_dhash_entrstaticvoid__initdcache_init_early(void){-intloop;+unsignedlongloop;/* If hashes are distributed across NUMA nodes, defer*hashallocationuntilvmallocspaceisavailable.
@@ -1654,7 +1654,7 @@ __setup("ihash_entries=", set_ihash_entr*/void__initinode_init_early(void){-intloop;+unsignedlongloop;/* If hashes are distributed across NUMA nodes, defer*hashallocationuntilvmallocspaceisavailable.
@@ -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=(unsignedint)hash_mask;memset(rt_hash_table,0,(rt_hash_mask+1)*sizeof(structrt_hash_bucket));rt_hash_lock_init();
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 <redacted>
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.
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2012-01-17 17:25:27
On Tue, Jan 17, 2012 at 11:13:52AM -0600, Dimitri Sivanich wrote:
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.
... 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...
From: David Miller <davem@davemloft.net> Date: 2012-01-17 17:28:00
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Tue, 17 Jan 2012 17:25:27 +0000
On Tue, Jan 17, 2012 at 11:13:52AM -0600, Dimitri Sivanich wrote:
quoted
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.
... 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...
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 <redacted>
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.
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.
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.
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.
On Tue, Jan 17, 2012 at 12:22:29PM -0500, David Miller wrote:
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 <redacted>
---
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(-)
Index: linux/fs/dcache.c
===================================================================
--- linux.orig/fs/dcache.c+++ linux/fs/dcache.c
@@ -2968,7 +2968,7 @@ __setup("dhash_entries=", set_dhash_entrstaticvoid__initdcache_init_early(void){-intloop;+unsignedintloop;/* If hashes are distributed across NUMA nodes, defer*hashallocationuntilvmallocspaceisavailable.
@@ -1654,7 +1654,7 @@ __setup("ihash_entries=", set_ihash_entr*/void__initinode_init_early(void){-intloop;+unsignedintloop;/* If hashes are distributed across NUMA nodes, defer*hashallocationuntilvmallocspaceisavailable.
On Tue, Jan 17, 2012 at 12:22:29PM -0500, David Miller wrote:
quoted
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 <redacted>
This looks good to me, thanks Dimitri:
Acked-by: David S. Miller <davem@davemloft.net>