All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] lib/rhashtable: reorder some inititalization sequences
@ 2018-05-14 15:13 Davidlohr Bueso
  2018-05-15  2:52 ` David Miller
  0 siblings, 1 reply; 3+ messages in thread
From: Davidlohr Bueso @ 2018-05-14 15:13 UTC (permalink / raw)
  To: akpm, tgraf, herbert; +Cc: netdev, linux-kernel, dave, Davidlohr Bueso

rhashtable_init() allocates memory at the very end of the
call, once everything is setup; with the exception of the
nelems parameter. However, unless the user is doing something
bogus with params for which -EINVAL is returned, memory
allocation is the only operation that can trigger the call
to fail.

Thus move bucket_table_alloc() up such that we fail back to
the caller asap, instead of doing useless checks. This is
safe as the the table allocation isn't using the halfly
setup 'ht' structure and bucket_table_alloc() call chain only
ends up using the ht->nulls_base member in INIT_RHT_NULLS_HEAD.

Also move the locking initialization down to the end.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 lib/rhashtable.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 9427b5766134..68aadd6bff60 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -1022,8 +1022,6 @@ int rhashtable_init(struct rhashtable *ht,
 	struct bucket_table *tbl;
 	size_t size;
 
-	size = HASH_DEFAULT_SIZE;
-
 	if ((!params->key_len && !params->obj_hashfn) ||
 	    (params->obj_hashfn && !params->obj_cmpfn))
 		return -EINVAL;
@@ -1032,10 +1030,17 @@ int rhashtable_init(struct rhashtable *ht,
 		return -EINVAL;
 
 	memset(ht, 0, sizeof(*ht));
-	mutex_init(&ht->mutex);
-	spin_lock_init(&ht->lock);
 	memcpy(&ht->p, params, sizeof(*params));
 
+	if (!params->nelem_hint)
+		size = HASH_DEFAULT_SIZE;
+	else
+		size = rounded_hashtable_size(&ht->p);
+
+	tbl = bucket_table_alloc(ht, size, GFP_KERNEL);
+	if (tbl == NULL)
+		return -ENOMEM;
+
 	if (params->min_size)
 		ht->p.min_size = roundup_pow_of_two(params->min_size);
 
@@ -1050,9 +1055,6 @@ int rhashtable_init(struct rhashtable *ht,
 
 	ht->p.min_size = max_t(u16, ht->p.min_size, HASH_MIN_SIZE);
 
-	if (params->nelem_hint)
-		size = rounded_hashtable_size(&ht->p);
-
 	if (params->locks_mul)
 		ht->p.locks_mul = roundup_pow_of_two(params->locks_mul);
 	else
@@ -1068,10 +1070,8 @@ int rhashtable_init(struct rhashtable *ht,
 		}
 	}
 
-	tbl = bucket_table_alloc(ht, size, GFP_KERNEL);
-	if (tbl == NULL)
-		return -ENOMEM;
-
+	mutex_init(&ht->mutex);
+	spin_lock_init(&ht->lock);
 	atomic_set(&ht->nelems, 0);
 
 	RCU_INIT_POINTER(ht->tbl, tbl);
-- 
2.13.6

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

* Re: [PATCH] lib/rhashtable: reorder some inititalization sequences
  2018-05-14 15:13 [PATCH] lib/rhashtable: reorder some inititalization sequences Davidlohr Bueso
@ 2018-05-15  2:52 ` David Miller
  2018-05-15  3:37   ` Herbert Xu
  0 siblings, 1 reply; 3+ messages in thread
From: David Miller @ 2018-05-15  2:52 UTC (permalink / raw)
  To: dave; +Cc: akpm, tgraf, herbert, netdev, linux-kernel, dbueso

From: Davidlohr Bueso <dave@stgolabs.net>
Date: Mon, 14 May 2018 08:13:32 -0700

> rhashtable_init() allocates memory at the very end of the
> call, once everything is setup; with the exception of the
> nelems parameter. However, unless the user is doing something
> bogus with params for which -EINVAL is returned, memory
> allocation is the only operation that can trigger the call
> to fail.
> 
> Thus move bucket_table_alloc() up such that we fail back to
> the caller asap, instead of doing useless checks. This is
> safe as the the table allocation isn't using the halfly
> setup 'ht' structure and bucket_table_alloc() call chain only
> ends up using the ht->nulls_base member in INIT_RHT_NULLS_HEAD.
> 
> Also move the locking initialization down to the end.
> 
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>

The user potentially "doing something bogus" is why the most
expensive part of the initialization (the memory allocation)
is done after everything else is validated.

I think it's best to keep things as-is.

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

* Re: [PATCH] lib/rhashtable: reorder some inititalization sequences
  2018-05-15  2:52 ` David Miller
@ 2018-05-15  3:37   ` Herbert Xu
  0 siblings, 0 replies; 3+ messages in thread
From: Herbert Xu @ 2018-05-15  3:37 UTC (permalink / raw)
  To: David Miller; +Cc: dave, akpm, tgraf, netdev, linux-kernel, dbueso

On Mon, May 14, 2018 at 10:52:13PM -0400, David Miller wrote:
> From: Davidlohr Bueso <dave@stgolabs.net>
> Date: Mon, 14 May 2018 08:13:32 -0700
> 
> > rhashtable_init() allocates memory at the very end of the
> > call, once everything is setup; with the exception of the
> > nelems parameter. However, unless the user is doing something
> > bogus with params for which -EINVAL is returned, memory
> > allocation is the only operation that can trigger the call
> > to fail.
> > 
> > Thus move bucket_table_alloc() up such that we fail back to
> > the caller asap, instead of doing useless checks. This is
> > safe as the the table allocation isn't using the halfly
> > setup 'ht' structure and bucket_table_alloc() call chain only
> > ends up using the ht->nulls_base member in INIT_RHT_NULLS_HEAD.
> > 
> > Also move the locking initialization down to the end.
> > 
> > Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> 
> The user potentially "doing something bogus" is why the most
> expensive part of the initialization (the memory allocation)
> is done after everything else is validated.
> 
> I think it's best to keep things as-is.

I agree.

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] 3+ messages in thread

end of thread, other threads:[~2018-05-15  3:37 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-14 15:13 [PATCH] lib/rhashtable: reorder some inititalization sequences Davidlohr Bueso
2018-05-15  2:52 ` David Miller
2018-05-15  3:37   ` Herbert Xu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.