[PATCH net 0/2] rhashtable rehashing fixes

[PATCH net 0/2] rhashtable rehashing fixes

[Thomas Graf]

Some rhashtable rehashing bugs found while testing with the
next rhashtable self-test queued up for the next devel cycle:

https://github.com/tgraf/net-next/commits/rht

Thomas Graf (2):
  rhashtable: Schedule async resize when sync realloc fails
  rhashtable: Do not schedule more than one rehash if we can't grow
    further

 include/linux/rhashtable.h | 2 ++
 lib/rhashtable.c           | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.3.5

[PATCH net 1/2] rhashtable: Schedule async resize when sync realloc fails

[Thomas Graf]

When rhashtable_insert_rehash() fails with ENOMEM, this indicates that
we can't allocate the necessary memory in the current context but the
limits as set by the user would still allow to grow.

Thus attempt an async resize in the background where we can allocate
using GFP_KERNEL which is more likely to succeed. The insertion itself
will still fail to indicate pressure.

This fixes a bug where the table would never continue growing once the
utilization is above 100%.

Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 include/linux/rhashtable.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index e23d242..7040b5c 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -593,6 +593,8 @@ slow_path:
 		spin_unlock_bh(lock);
 		err = rhashtable_insert_rehash(ht);
 		rcu_read_unlock();
+		if (err == -ENOMEM)
+			schedule_work(&ht->run_work);
 		if (err)
 			return err;
 
-- 
2.3.5

Re: [PATCH net 1/2] rhashtable: Schedule async resize when sync realloc fails

[Herbert Xu]

On Tue, Apr 21, 2015 at 02:55:34PM +0200, Thomas Graf wrote:
> When rhashtable_insert_rehash() fails with ENOMEM, this indicates that
> we can't allocate the necessary memory in the current context but the
> limits as set by the user would still allow to grow.
> 
> Thus attempt an async resize in the background where we can allocate
> using GFP_KERNEL which is more likely to succeed. The insertion itself
> will still fail to indicate pressure.
> 
> This fixes a bug where the table would never continue growing once the
> utilization is above 100%.
> 
> Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

Good catch.  But I think this call should happen in
rhashtable_insert_rehash since it's on the slow-path.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net 1/2] rhashtable: Schedule async resize when sync realloc fails

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 22 Apr 2015 08:36:34 +0800

> On Tue, Apr 21, 2015 at 02:55:34PM +0200, Thomas Graf wrote:
>> When rhashtable_insert_rehash() fails with ENOMEM, this indicates that
>> we can't allocate the necessary memory in the current context but the
>> limits as set by the user would still allow to grow.
>> 
>> Thus attempt an async resize in the background where we can allocate
>> using GFP_KERNEL which is more likely to succeed. The insertion itself
>> will still fail to indicate pressure.
>> 
>> This fixes a bug where the table would never continue growing once the
>> utilization is above 100%.
>> 
>> Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
>> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> Good catch.  But I think this call should happen in
> rhashtable_insert_rehash since it's on the slow-path.

Ok, then I expect a respin of this series.

Re: [PATCH net 1/2] rhashtable: Schedule async resize when sync realloc fails

[Thomas Graf]

On 04/21/15 at 10:10pm, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Wed, 22 Apr 2015 08:36:34 +0800
> 
> > On Tue, Apr 21, 2015 at 02:55:34PM +0200, Thomas Graf wrote:
> >> When rhashtable_insert_rehash() fails with ENOMEM, this indicates that
> >> we can't allocate the necessary memory in the current context but the
> >> limits as set by the user would still allow to grow.
> >> 
> >> Thus attempt an async resize in the background where we can allocate
> >> using GFP_KERNEL which is more likely to succeed. The insertion itself
> >> will still fail to indicate pressure.
> >> 
> >> This fixes a bug where the table would never continue growing once the
> >> utilization is above 100%.
> >> 
> >> Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
> >> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> > 
> > Good catch.  But I think this call should happen in
> > rhashtable_insert_rehash since it's on the slow-path.
> 
> Ok, then I expect a respin of this series.

Agreed, respinning.

[PATCH net 2/2] rhashtable: Do not schedule more than one rehash if we can't grow further

[Thomas Graf]

The current code currently only stops inserting rehashes into the
chain when no resizes are currently scheduled. As long as resizes
are scheduled and while inserting above the utilization watermark,
more and more rehashes will be scheduled.

This lead to a perfect DoS storm with thousands of rehashes
scheduled which lead to thousands of spinlocks to be taken
sequentially.

Instead, only allow either a series of resizes or a single rehash.
Drop any further rehashes and return -EBUSY.

Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 lib/rhashtable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 4898442..cb819ed 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -405,8 +405,8 @@ int rhashtable_insert_rehash(struct rhashtable *ht)
 
 	if (rht_grow_above_75(ht, tbl))
 		size *= 2;
-	/* More than two rehashes (not resizes) detected. */
-	else if (WARN_ON(old_tbl != tbl && old_tbl->size == size))
+	/* Do not schedule more than one rehash */
+	else if (old_tbl != tbl)
 		return -EBUSY;
 
 	new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC);
-- 
2.3.5

Re: [PATCH net 2/2] rhashtable: Do not schedule more than one rehash if we can't grow further

[Herbert Xu]

On Tue, Apr 21, 2015 at 02:55:35PM +0200, Thomas Graf wrote:
> The current code currently only stops inserting rehashes into the
> chain when no resizes are currently scheduled. As long as resizes
> are scheduled and while inserting above the utilization watermark,
> more and more rehashes will be scheduled.
> 
> This lead to a perfect DoS storm with thousands of rehashes
> scheduled which lead to thousands of spinlocks to be taken
> sequentially.
> 
> Instead, only allow either a series of resizes or a single rehash.
> Drop any further rehashes and return -EBUSY.
> 
> Fixes: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt