netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] rhashtable-test: add cond_resched() to thread test
@ 2015-08-28 10:28 Phil Sutter
  2015-08-28 10:28 ` [PATCH 2/3] rhashtable-test: retry insert operations in threads Phil Sutter
                   ` (2 more replies)
  0 siblings, 3 replies; 27+ messages in thread
From: Phil Sutter @ 2015-08-28 10:28 UTC (permalink / raw)
  To: davem; +Cc: netdev, linux-kernel, tgraf, fengguang.wu, wfg, lkp

This should fix for soft lockup bugs triggered on slow systems.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/test_rhashtable.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
index 8c1ad1c..63654e3 100644
--- a/lib/test_rhashtable.c
+++ b/lib/test_rhashtable.c
@@ -236,6 +236,8 @@ static int thread_lookup_test(struct thread_data *tdata)
 			       obj->value, key);
 			err++;
 		}
+
+		cond_resched();
 	}
 	return err;
 }
@@ -251,6 +253,7 @@ static int threadfunc(void *data)
 
 	for (i = 0; i < entries; i++) {
 		tdata->objs[i].value = (tdata->id << 16) | i;
+		cond_resched();
 		err = rhashtable_insert_fast(&ht, &tdata->objs[i].node,
 		                             test_rht_params);
 		if (err == -ENOMEM || err == -EBUSY) {
@@ -285,6 +288,8 @@ static int threadfunc(void *data)
 				goto out;
 			}
 			tdata->objs[i].value = TEST_INSERT_FAIL;
+
+			cond_resched();
 		}
 		err = thread_lookup_test(tdata);
 		if (err) {
-- 
2.1.2

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-08-28 10:28 [PATCH 1/3] rhashtable-test: add cond_resched() to thread test Phil Sutter
@ 2015-08-28 10:28 ` Phil Sutter
  2015-08-28 11:09   ` Thomas Graf
  2015-08-28 10:28 ` [PATCH 3/3] rhashtable-test: calculate max_entries value by default Phil Sutter
  2015-08-28 11:03 ` [PATCH 1/3] rhashtable-test: add cond_resched() to thread test Thomas Graf
  2 siblings, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2015-08-28 10:28 UTC (permalink / raw)
  To: davem; +Cc: netdev, linux-kernel, tgraf, fengguang.wu, wfg, lkp

After adding cond_resched() calls to threadfunc(), a surprisingly high
rate of insert failures occurred probably due to table resizes getting a
better chance to run in background. To not soften up the remaining
tests, retry inserts until they either succeed or fail permanently.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/test_rhashtable.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
index 63654e3..093cf84 100644
--- a/lib/test_rhashtable.c
+++ b/lib/test_rhashtable.c
@@ -244,7 +244,7 @@ static int thread_lookup_test(struct thread_data *tdata)
 
 static int threadfunc(void *data)
 {
-	int i, step, err = 0, insert_fails = 0;
+	int i, step, err = 0, retries = 0;
 	struct thread_data *tdata = data;
 
 	up(&prestart_sem);
@@ -253,21 +253,22 @@ static int threadfunc(void *data)
 
 	for (i = 0; i < entries; i++) {
 		tdata->objs[i].value = (tdata->id << 16) | i;
+insert_retry:
 		cond_resched();
 		err = rhashtable_insert_fast(&ht, &tdata->objs[i].node,
 		                             test_rht_params);
 		if (err == -ENOMEM || err == -EBUSY) {
-			tdata->objs[i].value = TEST_INSERT_FAIL;
-			insert_fails++;
+			retries++;
+			goto insert_retry;
 		} else if (err) {
 			pr_err("  thread[%d]: rhashtable_insert_fast failed\n",
 			       tdata->id);
 			goto out;
 		}
 	}
-	if (insert_fails)
-		pr_info("  thread[%d]: %d insert failures\n",
-		        tdata->id, insert_fails);
+	if (retries)
+		pr_info("  thread[%d]: retried insert operation %d times\n",
+		        tdata->id, retries);
 
 	err = thread_lookup_test(tdata);
 	if (err) {
-- 
2.1.2

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 3/3] rhashtable-test: calculate max_entries value by default
  2015-08-28 10:28 [PATCH 1/3] rhashtable-test: add cond_resched() to thread test Phil Sutter
  2015-08-28 10:28 ` [PATCH 2/3] rhashtable-test: retry insert operations in threads Phil Sutter
@ 2015-08-28 10:28 ` Phil Sutter
  2015-08-28 11:11   ` Thomas Graf
  2015-08-28 11:03 ` [PATCH 1/3] rhashtable-test: add cond_resched() to thread test Thomas Graf
  2 siblings, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2015-08-28 10:28 UTC (permalink / raw)
  To: davem; +Cc: netdev, linux-kernel, tgraf, fengguang.wu, wfg, lkp

A maximum table size of 64k entries is insufficient for the multiple
threads test even in default configuration (10 threads * 50000 objects =
500000 objects in total). Since we know how many objects will be
inserted, calculate the max size unless overridden by parameter.

Note that specifying the exact number of objects upon table init won't
suffice as that value is being rounded down to the next power of two -
anticipate this by rounding up to the next power of two in beforehand.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/test_rhashtable.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
index 093cf84..73fcb8e 100644
--- a/lib/test_rhashtable.c
+++ b/lib/test_rhashtable.c
@@ -36,9 +36,9 @@ static int runs = 4;
 module_param(runs, int, 0);
 MODULE_PARM_DESC(runs, "Number of test runs per variant (default: 4)");
 
-static int max_size = 65536;
+static int max_size = 0;
 module_param(max_size, int, 0);
-MODULE_PARM_DESC(runs, "Maximum table size (default: 65536)");
+MODULE_PARM_DESC(runs, "Maximum table size (default: calculated)");
 
 static bool shrinking = false;
 module_param(shrinking, bool, 0);
@@ -317,7 +317,7 @@ static int __init test_rht_init(void)
 	entries = min(entries, MAX_ENTRIES);
 
 	test_rht_params.automatic_shrinking = shrinking;
-	test_rht_params.max_size = max_size;
+	test_rht_params.max_size = max_size ? : roundup_pow_of_two(entries);
 	test_rht_params.nelem_hint = size;
 
 	pr_info("Running rhashtable test nelem=%d, max_size=%d, shrinking=%d\n",
@@ -363,6 +363,8 @@ static int __init test_rht_init(void)
 		return -ENOMEM;
 	}
 
+	test_rht_params.max_size = max_size ? :
+	                           roundup_pow_of_two(tcount * entries);
 	err = rhashtable_init(&ht, &test_rht_params);
 	if (err < 0) {
 		pr_warn("Test failed: Unable to initialize hashtable: %d\n",
-- 
2.1.2

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/3] rhashtable-test: add cond_resched() to thread test
  2015-08-28 10:28 [PATCH 1/3] rhashtable-test: add cond_resched() to thread test Phil Sutter
  2015-08-28 10:28 ` [PATCH 2/3] rhashtable-test: retry insert operations in threads Phil Sutter
  2015-08-28 10:28 ` [PATCH 3/3] rhashtable-test: calculate max_entries value by default Phil Sutter
@ 2015-08-28 11:03 ` Thomas Graf
  2 siblings, 0 replies; 27+ messages in thread
From: Thomas Graf @ 2015-08-28 11:03 UTC (permalink / raw)
  To: Phil Sutter; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On 08/28/15 at 12:28pm, Phil Sutter wrote:
> This should fix for soft lockup bugs triggered on slow systems.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Thomas Graf <tgraf@suug.ch>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-08-28 10:28 ` [PATCH 2/3] rhashtable-test: retry insert operations in threads Phil Sutter
@ 2015-08-28 11:09   ` Thomas Graf
  2015-08-28 11:13     ` Phil Sutter
  0 siblings, 1 reply; 27+ messages in thread
From: Thomas Graf @ 2015-08-28 11:09 UTC (permalink / raw)
  To: Phil Sutter; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On 08/28/15 at 12:28pm, Phil Sutter wrote:
> After adding cond_resched() calls to threadfunc(), a surprisingly high
> rate of insert failures occurred probably due to table resizes getting a
> better chance to run in background. To not soften up the remaining
> tests, retry inserts until they either succeed or fail permanently.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  lib/test_rhashtable.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
> index 63654e3..093cf84 100644
> --- a/lib/test_rhashtable.c
> +++ b/lib/test_rhashtable.c
> @@ -244,7 +244,7 @@ static int thread_lookup_test(struct thread_data *tdata)
>  
>  static int threadfunc(void *data)
>  {
> -	int i, step, err = 0, insert_fails = 0;
> +	int i, step, err = 0, retries = 0;
>  	struct thread_data *tdata = data;
>  
>  	up(&prestart_sem);
> @@ -253,21 +253,22 @@ static int threadfunc(void *data)
>  
>  	for (i = 0; i < entries; i++) {
>  		tdata->objs[i].value = (tdata->id << 16) | i;
> +insert_retry:
>  		cond_resched();
>  		err = rhashtable_insert_fast(&ht, &tdata->objs[i].node,
>  		                             test_rht_params);
>  		if (err == -ENOMEM || err == -EBUSY) {
> -			tdata->objs[i].value = TEST_INSERT_FAIL;
> -			insert_fails++;
> +			retries++;
> +			goto insert_retry;

Is it safe to retry indefinitely on ENOMEM? Retrying on EBUSY is
definitely an improvement and we should do the same in the non
threaded test as well.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/3] rhashtable-test: calculate max_entries value by default
  2015-08-28 10:28 ` [PATCH 3/3] rhashtable-test: calculate max_entries value by default Phil Sutter
@ 2015-08-28 11:11   ` Thomas Graf
  0 siblings, 0 replies; 27+ messages in thread
From: Thomas Graf @ 2015-08-28 11:11 UTC (permalink / raw)
  To: Phil Sutter; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On 08/28/15 at 12:28pm, Phil Sutter wrote:
> A maximum table size of 64k entries is insufficient for the multiple
> threads test even in default configuration (10 threads * 50000 objects =
> 500000 objects in total). Since we know how many objects will be
> inserted, calculate the max size unless overridden by parameter.
> 
> Note that specifying the exact number of objects upon table init won't
> suffice as that value is being rounded down to the next power of two -
> anticipate this by rounding up to the next power of two in beforehand.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Thomas Graf <tgraf@suug.ch>

Thanks for doing this work.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-08-28 11:09   ` Thomas Graf
@ 2015-08-28 11:13     ` Phil Sutter
  2015-08-28 13:34       ` Phil Sutter
  0 siblings, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2015-08-28 11:13 UTC (permalink / raw)
  To: Thomas Graf; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Fri, Aug 28, 2015 at 01:09:29PM +0200, Thomas Graf wrote:
> On 08/28/15 at 12:28pm, Phil Sutter wrote:
> > After adding cond_resched() calls to threadfunc(), a surprisingly high
> > rate of insert failures occurred probably due to table resizes getting a
> > better chance to run in background. To not soften up the remaining
> > tests, retry inserts until they either succeed or fail permanently.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  lib/test_rhashtable.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
> > index 63654e3..093cf84 100644
> > --- a/lib/test_rhashtable.c
> > +++ b/lib/test_rhashtable.c
> > @@ -244,7 +244,7 @@ static int thread_lookup_test(struct thread_data *tdata)
> >  
> >  static int threadfunc(void *data)
> >  {
> > -	int i, step, err = 0, insert_fails = 0;
> > +	int i, step, err = 0, retries = 0;
> >  	struct thread_data *tdata = data;
> >  
> >  	up(&prestart_sem);
> > @@ -253,21 +253,22 @@ static int threadfunc(void *data)
> >  
> >  	for (i = 0; i < entries; i++) {
> >  		tdata->objs[i].value = (tdata->id << 16) | i;
> > +insert_retry:
> >  		cond_resched();
> >  		err = rhashtable_insert_fast(&ht, &tdata->objs[i].node,
> >  		                             test_rht_params);
> >  		if (err == -ENOMEM || err == -EBUSY) {
> > -			tdata->objs[i].value = TEST_INSERT_FAIL;
> > -			insert_fails++;
> > +			retries++;
> > +			goto insert_retry;
> 
> Is it safe to retry indefinitely on ENOMEM? Retrying on EBUSY is
> definitely an improvement and we should do the same in the non
> threaded test as well.

Oh yes, that is definitely a bug. I will respin and add the same for the
normal test, too.

Thanks, Phil

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-08-28 11:13     ` Phil Sutter
@ 2015-08-28 13:34       ` Phil Sutter
  2015-08-28 22:43         ` Thomas Graf
  0 siblings, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2015-08-28 13:34 UTC (permalink / raw)
  To: Thomas Graf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Fri, Aug 28, 2015 at 01:13:20PM +0200, Phil Sutter wrote:
> On Fri, Aug 28, 2015 at 01:09:29PM +0200, Thomas Graf wrote:
> > On 08/28/15 at 12:28pm, Phil Sutter wrote:
> > > After adding cond_resched() calls to threadfunc(), a surprisingly high
> > > rate of insert failures occurred probably due to table resizes getting a
> > > better chance to run in background. To not soften up the remaining
> > > tests, retry inserts until they either succeed or fail permanently.
> > > 
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  lib/test_rhashtable.c | 13 +++++++------
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
> > > index 63654e3..093cf84 100644
> > > --- a/lib/test_rhashtable.c
> > > +++ b/lib/test_rhashtable.c
> > > @@ -244,7 +244,7 @@ static int thread_lookup_test(struct thread_data *tdata)
> > >  
> > >  static int threadfunc(void *data)
> > >  {
> > > -	int i, step, err = 0, insert_fails = 0;
> > > +	int i, step, err = 0, retries = 0;
> > >  	struct thread_data *tdata = data;
> > >  
> > >  	up(&prestart_sem);
> > > @@ -253,21 +253,22 @@ static int threadfunc(void *data)
> > >  
> > >  	for (i = 0; i < entries; i++) {
> > >  		tdata->objs[i].value = (tdata->id << 16) | i;
> > > +insert_retry:
> > >  		cond_resched();
> > >  		err = rhashtable_insert_fast(&ht, &tdata->objs[i].node,
> > >  		                             test_rht_params);
> > >  		if (err == -ENOMEM || err == -EBUSY) {
> > > -			tdata->objs[i].value = TEST_INSERT_FAIL;
> > > -			insert_fails++;
> > > +			retries++;
> > > +			goto insert_retry;
> > 
> > Is it safe to retry indefinitely on ENOMEM? Retrying on EBUSY is
> > definitely an improvement and we should do the same in the non
> > threaded test as well.
> 
> Oh yes, that is definitely a bug. I will respin and add the same for the
> normal test, too.

Quite ugly, IMHO: rhashtable_insert_fast() may return -ENOMEM as
non-permanent error, if allocation in GFP_ATOMIC failed. In this case,
allocation in GFP_KERNEL is retried by rht_deferred_worker(). Sadly,
there is no way to determine if that has already been tried and failed.

The thread test triggers GFP_ATOMIC allocation failure quite easily, so
I can't really just ignore this issue. :)

Cheers, Phil

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-08-28 13:34       ` Phil Sutter
@ 2015-08-28 22:43         ` Thomas Graf
  2015-08-29  9:07           ` Phil Sutter
  0 siblings, 1 reply; 27+ messages in thread
From: Thomas Graf @ 2015-08-28 22:43 UTC (permalink / raw)
  To: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On 08/28/15 at 03:34pm, Phil Sutter wrote:
> Quite ugly, IMHO: rhashtable_insert_fast() may return -ENOMEM as
> non-permanent error, if allocation in GFP_ATOMIC failed. In this case,
> allocation in GFP_KERNEL is retried by rht_deferred_worker(). Sadly,
> there is no way to determine if that has already been tried and failed.
> 
> The thread test triggers GFP_ATOMIC allocation failure quite easily, so
> I can't really just ignore this issue. :)

Return EBUSY or ENOBUFS in the non-permanent case? It is definitely
helpful if the API allows to differ between permanent and
non-permanent errors.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-08-28 22:43         ` Thomas Graf
@ 2015-08-29  9:07           ` Phil Sutter
  2015-08-30  7:47             ` Herbert Xu
  0 siblings, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2015-08-29  9:07 UTC (permalink / raw)
  To: Thomas Graf; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Sat, Aug 29, 2015 at 12:43:03AM +0200, Thomas Graf wrote:
> On 08/28/15 at 03:34pm, Phil Sutter wrote:
> > Quite ugly, IMHO: rhashtable_insert_fast() may return -ENOMEM as
> > non-permanent error, if allocation in GFP_ATOMIC failed. In this case,
> > allocation in GFP_KERNEL is retried by rht_deferred_worker(). Sadly,
> > there is no way to determine if that has already been tried and failed.
> > 
> > The thread test triggers GFP_ATOMIC allocation failure quite easily, so
> > I can't really just ignore this issue. :)
> 
> Return EBUSY or ENOBUFS in the non-permanent case? It is definitely
> helpful if the API allows to differ between permanent and
> non-permanent errors.

Yes, indeed. Therefore rht_deferred_worker() needs to check the return
value of rhashtable_expand(). The question is how to propagate the error
condition, as the worker's return value is not being kept track of
(function returns void even).

Should we introduce a new field to struct rhashtable to track the
internal state? This might allow to clean up some rather obscure tests,
e.g. whether a table resize is in progress or not.

Cheers, Phil

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-08-29  9:07           ` Phil Sutter
@ 2015-08-30  7:47             ` Herbert Xu
  2015-08-31 11:00               ` Phil Sutter
  0 siblings, 1 reply; 27+ messages in thread
From: Herbert Xu @ 2015-08-30  7:47 UTC (permalink / raw)
  To: Phil Sutter; +Cc: tgraf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

Phil Sutter <phil@nwl.cc> wrote:
>
> Should we introduce a new field to struct rhashtable to track the
> internal state? This might allow to clean up some rather obscure tests,
> e.g. whether a table resize is in progress or not.

Why would we want to do that? The deferred expansion is done
on a best effort basis so its failure has nothing to do with
the failure of a subsequent insertion.

The insertion must have tried its own last-ditch synchronous
expansion and only fail if that fails.

Cheers,
-- 
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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-08-30  7:47             ` Herbert Xu
@ 2015-08-31 11:00               ` Phil Sutter
  2015-09-01 11:43                 ` Herbert Xu
  0 siblings, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2015-08-31 11:00 UTC (permalink / raw)
  To: Herbert Xu; +Cc: tgraf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Sun, Aug 30, 2015 at 03:47:17PM +0800, Herbert Xu wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> >
> > Should we introduce a new field to struct rhashtable to track the
> > internal state? This might allow to clean up some rather obscure tests,
> > e.g. whether a table resize is in progress or not.
> 
> Why would we want to do that? The deferred expansion is done
> on a best effort basis so its failure has nothing to do with
> the failure of a subsequent insertion.

The variable would be used to track if the worker has failed to allocate
memory in background.

Since the failing insertion will be retried, subsequent inserts are not
necessary unrelated.

> The insertion must have tried its own last-ditch synchronous
> expansion and only fail if that fails.

Who do you mean with "the insertion"? The user or the API?

Cheers, Phil

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-08-31 11:00               ` Phil Sutter
@ 2015-09-01 11:43                 ` Herbert Xu
  2015-09-01 12:46                   ` Phil Sutter
  0 siblings, 1 reply; 27+ messages in thread
From: Herbert Xu @ 2015-09-01 11:43 UTC (permalink / raw)
  To: tgraf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Mon, Aug 31, 2015 at 01:00:12PM +0200, Phil Sutter wrote:
> 
> The variable would be used to track if the worker has failed to allocate
> memory in background.
> 
> Since the failing insertion will be retried, subsequent inserts are not
> necessary unrelated.

If an insertion fails it is never retried.  The only thing that is
retried is the expansion of the table.  So I have no idea what
you are talking about.

Cheers,
-- 
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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 11:43                 ` Herbert Xu
@ 2015-09-01 12:46                   ` Phil Sutter
  2015-09-01 13:00                     ` Herbert Xu
  0 siblings, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2015-09-01 12:46 UTC (permalink / raw)
  To: Herbert Xu; +Cc: tgraf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Tue, Sep 01, 2015 at 07:43:00PM +0800, Herbert Xu wrote:
> On Mon, Aug 31, 2015 at 01:00:12PM +0200, Phil Sutter wrote:
> > 
> > The variable would be used to track if the worker has failed to allocate
> > memory in background.
> > 
> > Since the failing insertion will be retried, subsequent inserts are not
> > necessary unrelated.
> 
> If an insertion fails it is never retried.  The only thing that is
> retried is the expansion of the table.  So I have no idea what
> you are talking about.

This is not an inherent behaviour of the implementation but general
agreement. The insertion may fail non-permanently (returning -EBUSY),
users are expected to handle this by retrying the operation.

Cheers, Phil

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 12:46                   ` Phil Sutter
@ 2015-09-01 13:00                     ` Herbert Xu
  2015-09-01 13:40                       ` Eric Dumazet
  2015-09-01 13:43                       ` Phil Sutter
  0 siblings, 2 replies; 27+ messages in thread
From: Herbert Xu @ 2015-09-01 13:00 UTC (permalink / raw)
  To: tgraf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Tue, Sep 01, 2015 at 02:46:48PM +0200, Phil Sutter wrote:
>
> This is not an inherent behaviour of the implementation but general
> agreement. The insertion may fail non-permanently (returning -EBUSY),
> users are expected to handle this by retrying the operation.

Absolutely not.  The only reason for an insertion to fail is if we
can't allocate enough memory.  Unless the user is also looping its
kmalloc calls it definitely shouldn't be retrying the insert.

If an expansion fails it means either that the system is suffering
a catastrophic memory shortage, or the user of rhashtable is doing
something wrong.

Cheers,
-- 
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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 13:00                     ` Herbert Xu
@ 2015-09-01 13:40                       ` Eric Dumazet
  2015-09-01 13:43                       ` Phil Sutter
  1 sibling, 0 replies; 27+ messages in thread
From: Eric Dumazet @ 2015-09-01 13:40 UTC (permalink / raw)
  To: Herbert Xu; +Cc: tgraf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Tue, 2015-09-01 at 21:00 +0800, Herbert Xu wrote:
> On Tue, Sep 01, 2015 at 02:46:48PM +0200, Phil Sutter wrote:
> >
> > This is not an inherent behaviour of the implementation but general
> > agreement. The insertion may fail non-permanently (returning -EBUSY),
> > users are expected to handle this by retrying the operation.
> 
> Absolutely not.  The only reason for an insertion to fail is if we
> can't allocate enough memory.  Unless the user is also looping its
> kmalloc calls it definitely shouldn't be retrying the insert.
> 
> If an expansion fails it means either that the system is suffering
> a catastrophic memory shortage, or the user of rhashtable is doing
> something wrong.


-EBUSY does not sound as a memory allocation error.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 13:00                     ` Herbert Xu
  2015-09-01 13:40                       ` Eric Dumazet
@ 2015-09-01 13:43                       ` Phil Sutter
  2015-09-01 13:50                         ` Herbert Xu
  1 sibling, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2015-09-01 13:43 UTC (permalink / raw)
  To: Herbert Xu; +Cc: tgraf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Tue, Sep 01, 2015 at 09:00:57PM +0800, Herbert Xu wrote:
> On Tue, Sep 01, 2015 at 02:46:48PM +0200, Phil Sutter wrote:
> >
> > This is not an inherent behaviour of the implementation but general
> > agreement. The insertion may fail non-permanently (returning -EBUSY),
> > users are expected to handle this by retrying the operation.
> 
> Absolutely not.  The only reason for an insertion to fail is if we
> can't allocate enough memory.  Unless the user is also looping its
> kmalloc calls it definitely shouldn't be retrying the insert.

rhashtable_insert_fast() returns -EBUSY if the table is full
(rht_grow_above_100() returns true) and an asynchronous rehash operation
is active. AFAICT, this is not necessarily caused by memory pressure.

> If an expansion fails it means either that the system is suffering
> a catastrophic memory shortage, or the user of rhashtable is doing
> something wrong.

Hmm. Since memory allocation is first tried with GFP_ATOMIC set and upon
failure retried in background, this seems like a situation which might
happen during normal use. If that already indicates a severe problem,
why retry in background at all?

Cheers, Phil

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 13:43                       ` Phil Sutter
@ 2015-09-01 13:50                         ` Herbert Xu
  2015-09-01 13:56                           ` Phil Sutter
  0 siblings, 1 reply; 27+ messages in thread
From: Herbert Xu @ 2015-09-01 13:50 UTC (permalink / raw)
  To: tgraf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Tue, Sep 01, 2015 at 03:43:11PM +0200, Phil Sutter wrote:
>
> Hmm. Since memory allocation is first tried with GFP_ATOMIC set and upon
> failure retried in background, this seems like a situation which might
> happen during normal use. If that already indicates a severe problem,
> why retry in background at all?

It should be tried in the background first at 70% and only when
that fails would we hit the 100% case and then we will try it
with GFP_ATOMIC.  If that fails then the insertion will fail.

Cheers,
-- 
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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 13:50                         ` Herbert Xu
@ 2015-09-01 13:56                           ` Phil Sutter
  2015-09-01 14:03                             ` Herbert Xu
  0 siblings, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2015-09-01 13:56 UTC (permalink / raw)
  To: Herbert Xu; +Cc: tgraf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Tue, Sep 01, 2015 at 09:50:19PM +0800, Herbert Xu wrote:
> On Tue, Sep 01, 2015 at 03:43:11PM +0200, Phil Sutter wrote:
> >
> > Hmm. Since memory allocation is first tried with GFP_ATOMIC set and upon
> > failure retried in background, this seems like a situation which might
> > happen during normal use. If that already indicates a severe problem,
> > why retry in background at all?
> 
> It should be tried in the background first at 70% and only when
> that fails would we hit the 100% case and then we will try it
> with GFP_ATOMIC.  If that fails then the insertion will fail.

Ah, good to know. Thanks for clarifying this!

Looking at rhashtable_test.c, I see the initial table size is 8 entries.
70% of that is 5.6 entries, so background expansion is started after the
6th entry has been added, right? Given there are 10 threads running
which try to insert 50k entries at the same time, I don't think it's
unlikely that three more entries are inserted before the background
expansion completes.

Cheers, Phil

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 13:56                           ` Phil Sutter
@ 2015-09-01 14:03                             ` Herbert Xu
  2015-09-01 14:13                               ` Thomas Graf
  0 siblings, 1 reply; 27+ messages in thread
From: Herbert Xu @ 2015-09-01 14:03 UTC (permalink / raw)
  To: tgraf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Tue, Sep 01, 2015 at 03:56:18PM +0200, Phil Sutter wrote:
> 
> Looking at rhashtable_test.c, I see the initial table size is 8 entries.
> 70% of that is 5.6 entries, so background expansion is started after the
> 6th entry has been added, right? Given there are 10 threads running
> which try to insert 50k entries at the same time, I don't think it's
> unlikely that three more entries are inserted before the background
> expansion completes.

Yes but in that case the GFP_ATOMIC allocation should work because
the table is so small anyway.

Cheers,
-- 
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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 14:03                             ` Herbert Xu
@ 2015-09-01 14:13                               ` Thomas Graf
  2015-09-01 14:16                                 ` Herbert Xu
  0 siblings, 1 reply; 27+ messages in thread
From: Thomas Graf @ 2015-09-01 14:13 UTC (permalink / raw)
  To: Herbert Xu; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On 09/01/15 at 10:03pm, Herbert Xu wrote:
> On Tue, Sep 01, 2015 at 03:56:18PM +0200, Phil Sutter wrote:
> > 
> > Looking at rhashtable_test.c, I see the initial table size is 8 entries.
> > 70% of that is 5.6 entries, so background expansion is started after the
> > 6th entry has been added, right? Given there are 10 threads running
> > which try to insert 50k entries at the same time, I don't think it's
> > unlikely that three more entries are inserted before the background
> > expansion completes.
> 
> Yes but in that case the GFP_ATOMIC allocation should work because
> the table is so small anyway.

You can easily trigger this outside of the testsuite as well. Open
10K Netlink sockets in a loop and the creation of the sockets will
fail way before memory pressure kicks in.

I agree with you that the user should never retry on memory failure.
That's why I suggested to differentiate between a "permanent" failure
(memory pressure) and non-permanent failure (temporary overload on
background expansion). Hence the proposed difference of return codes
ENOMEM and EBUSY to report this back to the API user.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 14:13                               ` Thomas Graf
@ 2015-09-01 14:16                                 ` Herbert Xu
  2015-09-01 14:51                                   ` Thomas Graf
  0 siblings, 1 reply; 27+ messages in thread
From: Herbert Xu @ 2015-09-01 14:16 UTC (permalink / raw)
  To: Thomas Graf; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Tue, Sep 01, 2015 at 04:13:05PM +0200, Thomas Graf wrote:
>
> You can easily trigger this outside of the testsuite as well. Open
> 10K Netlink sockets in a loop and the creation of the sockets will
> fail way before memory pressure kicks in.

That means our implementation is buggy.  Do you have a test program?

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 14:16                                 ` Herbert Xu
@ 2015-09-01 14:51                                   ` Thomas Graf
  2015-09-02  2:00                                     ` Herbert Xu
  2015-09-10  8:03                                     ` Herbert Xu
  0 siblings, 2 replies; 27+ messages in thread
From: Thomas Graf @ 2015-09-01 14:51 UTC (permalink / raw)
  To: Herbert Xu; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On 09/01/15 at 10:16pm, Herbert Xu wrote:
> On Tue, Sep 01, 2015 at 04:13:05PM +0200, Thomas Graf wrote:
> >
> > You can easily trigger this outside of the testsuite as well. Open
> > 10K Netlink sockets in a loop and the creation of the sockets will
> > fail way before memory pressure kicks in.
> 
> That means our implementation is buggy.  Do you have a test program?

1. The current in-kernel self-test
2. bind_netlink.c: https://github.com/tgraf/rhashtable

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 14:51                                   ` Thomas Graf
@ 2015-09-02  2:00                                     ` Herbert Xu
  2015-09-02  7:07                                       ` Thomas Graf
  2015-09-10  8:03                                     ` Herbert Xu
  1 sibling, 1 reply; 27+ messages in thread
From: Herbert Xu @ 2015-09-02  2:00 UTC (permalink / raw)
  To: Thomas Graf; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Tue, Sep 01, 2015 at 04:51:24PM +0200, Thomas Graf wrote:
>
> 1. The current in-kernel self-test
> 2. bind_netlink.c: https://github.com/tgraf/rhashtable

Thanks, I will try to reproduce this.
-- 
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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-02  2:00                                     ` Herbert Xu
@ 2015-09-02  7:07                                       ` Thomas Graf
  0 siblings, 0 replies; 27+ messages in thread
From: Thomas Graf @ 2015-09-02  7:07 UTC (permalink / raw)
  To: Herbert Xu; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On 09/02/15 at 10:00am, Herbert Xu wrote:
> On Tue, Sep 01, 2015 at 04:51:24PM +0200, Thomas Graf wrote:
> >
> > 1. The current in-kernel self-test
> > 2. bind_netlink.c: https://github.com/tgraf/rhashtable
> 
> Thanks, I will try to reproduce this.

The path in question is:

int rhashtable_insert_rehash(struct rhashtable *ht)
{
	[...]

        old_tbl = rht_dereference_rcu(ht->tbl, ht);
        tbl = rhashtable_last_table(ht, old_tbl);

        size = tbl->size;

        if (rht_grow_above_75(ht, tbl))
                size *= 2;
        /* Do not schedule more than one rehash */
        else if (old_tbl != tbl)
                return -EBUSY;

The behaviour in question is the immediate rehash during
insertion which we want to fail.

Commits:
        ccd57b1bd32460d27bbb9c599e795628a3c66983
        a87b9ebf1709687ff213091d0fdb4254b1564803

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-01 14:51                                   ` Thomas Graf
  2015-09-02  2:00                                     ` Herbert Xu
@ 2015-09-10  8:03                                     ` Herbert Xu
  2015-09-10 10:05                                       ` Phil Sutter
  1 sibling, 1 reply; 27+ messages in thread
From: Herbert Xu @ 2015-09-10  8:03 UTC (permalink / raw)
  To: Thomas Graf; +Cc: davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Tue, Sep 01, 2015 at 04:51:24PM +0200, Thomas Graf wrote:
> 
> 1. The current in-kernel self-test
> 2. bind_netlink.c: https://github.com/tgraf/rhashtable

I can't reproduce it:

$ while :; do ./bind_netlink 10000 12345; done
Ports successfully created, terminating
Create 10000 ports
Created 1000 ports
Created 2000 ports
Created 3000 ports
Created 4000 ports
Created 5000 ports
Created 6000 ports
Created 7000 ports
Created 8000 ports
Created 9000 ports
Created 10000 ports
Ports successfully created, terminating
...

So what am I missing?

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/3] rhashtable-test: retry insert operations in threads
  2015-09-10  8:03                                     ` Herbert Xu
@ 2015-09-10 10:05                                       ` Phil Sutter
  0 siblings, 0 replies; 27+ messages in thread
From: Phil Sutter @ 2015-09-10 10:05 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Thomas Graf, davem, netdev, linux-kernel, fengguang.wu, wfg, lkp

On Thu, Sep 10, 2015 at 04:03:44PM +0800, Herbert Xu wrote:
> On Tue, Sep 01, 2015 at 04:51:24PM +0200, Thomas Graf wrote:
> > 
> > 1. The current in-kernel self-test
> > 2. bind_netlink.c: https://github.com/tgraf/rhashtable
> 
> I can't reproduce it:

I can't speak for netlink, but if you apply patch 1/3 from this thread,
test_rhashtable.c starts generating many insert failures during the
multiple thread test.

Cheers, Phil

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2015-09-10 10:05 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-28 10:28 [PATCH 1/3] rhashtable-test: add cond_resched() to thread test Phil Sutter
2015-08-28 10:28 ` [PATCH 2/3] rhashtable-test: retry insert operations in threads Phil Sutter
2015-08-28 11:09   ` Thomas Graf
2015-08-28 11:13     ` Phil Sutter
2015-08-28 13:34       ` Phil Sutter
2015-08-28 22:43         ` Thomas Graf
2015-08-29  9:07           ` Phil Sutter
2015-08-30  7:47             ` Herbert Xu
2015-08-31 11:00               ` Phil Sutter
2015-09-01 11:43                 ` Herbert Xu
2015-09-01 12:46                   ` Phil Sutter
2015-09-01 13:00                     ` Herbert Xu
2015-09-01 13:40                       ` Eric Dumazet
2015-09-01 13:43                       ` Phil Sutter
2015-09-01 13:50                         ` Herbert Xu
2015-09-01 13:56                           ` Phil Sutter
2015-09-01 14:03                             ` Herbert Xu
2015-09-01 14:13                               ` Thomas Graf
2015-09-01 14:16                                 ` Herbert Xu
2015-09-01 14:51                                   ` Thomas Graf
2015-09-02  2:00                                     ` Herbert Xu
2015-09-02  7:07                                       ` Thomas Graf
2015-09-10  8:03                                     ` Herbert Xu
2015-09-10 10:05                                       ` Phil Sutter
2015-08-28 10:28 ` [PATCH 3/3] rhashtable-test: calculate max_entries value by default Phil Sutter
2015-08-28 11:11   ` Thomas Graf
2015-08-28 11:03 ` [PATCH 1/3] rhashtable-test: add cond_resched() to thread test Thomas Graf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).