[PATCH v2 03/13] tools/xenstore: modify interface of create_hashtable()

Juergen Gross posted 13 patches 2 years, 10 months ago
There is a newer version of this series
[PATCH v2 03/13] tools/xenstore: modify interface of create_hashtable()
Posted by Juergen Gross 2 years, 10 months ago
The minsize parameter of create_hashtable() doesn't have any real use
case for Xenstore, so drop it.

For better talloc_report_full() diagnostic output add a name parameter
to create_hashtable().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/hashtable.c        | 20 ++++++--------------
 tools/xenstore/hashtable.h        |  4 ++--
 tools/xenstore/xenstored_core.c   |  2 +-
 tools/xenstore/xenstored_domain.c |  4 ++--
 4 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
index c1b11743bb..ab1e687d0b 100644
--- a/tools/xenstore/hashtable.c
+++ b/tools/xenstore/hashtable.c
@@ -55,36 +55,28 @@ static unsigned int loadlimit(unsigned int pindex)
     return ((uint64_t)primes[pindex] * MAX_LOAD_PERCENT) / 100;
 }
 
-struct hashtable *create_hashtable(const void *ctx, unsigned int minsize,
+struct hashtable *create_hashtable(const void *ctx, const char *name,
                                    unsigned int (*hashf) (const void *),
                                    int (*eqf) (const void *, const void *),
                                    unsigned int flags)
 {
     struct hashtable *h;
-    unsigned int pindex, size = primes[0];
-
-    /* Check requested hashtable isn't too large */
-    if (minsize > (1u << 30)) return NULL;
-
-    /* Enforce size as prime */
-    for (pindex=0; pindex < PRIME_TABLE_LEN; pindex++) {
-        if (primes[pindex] > minsize) { size = primes[pindex]; break; }
-    }
 
     h = talloc_zero(ctx, struct hashtable);
     if (NULL == h)
         goto err0;
-    h->table = talloc_zero_array(h, struct entry *, size);
+    talloc_set_name_const(h, name);
+    h->table = talloc_zero_array(h, struct entry *, primes[0]);
     if (NULL == h->table)
         goto err1;
 
-    h->tablelength  = size;
+    h->tablelength  = primes[0];
     h->flags        = flags;
-    h->primeindex   = pindex;
+    h->primeindex   = 0;
     h->entrycount   = 0;
     h->hashfn       = hashf;
     h->eqfn         = eqf;
-    h->loadlimit    = loadlimit(pindex);
+    h->loadlimit    = loadlimit(0);
     return h;
 
 err1:
diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h
index 04310783b6..0e1a6f61c2 100644
--- a/tools/xenstore/hashtable.h
+++ b/tools/xenstore/hashtable.h
@@ -10,7 +10,7 @@ struct hashtable;
    
  * @name                    create_hashtable
  * @param   ctx             talloc context to use for allocations
- * @param   minsize         minimum initial size of hashtable
+ * @param   name            talloc name of the hashtable
  * @param   hashfunction    function for hashing keys
  * @param   key_eq_fn       function for determining key equality
  * @param   flags           flags HASHTABLE_*
@@ -23,7 +23,7 @@ struct hashtable;
 #define HASHTABLE_FREE_KEY   (1U << 1)
 
 struct hashtable *
-create_hashtable(const void *ctx, unsigned int minsize,
+create_hashtable(const void *ctx, const char *name,
                  unsigned int (*hashfunction) (const void *),
                  int (*key_eq_fn) (const void *, const void *),
                  unsigned int flags
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 7214b3df03..6ce7be3161 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2511,7 +2511,7 @@ void check_store(void)
 	struct check_store_data data;
 
 	/* Don't free values (they are all void *1) */
-	data.reachable = create_hashtable(NULL, 16, hash_from_key_fn,
+	data.reachable = create_hashtable(NULL, "checkstore", hash_from_key_fn,
 					  keys_equal_fn, HASHTABLE_FREE_KEY);
 	if (!data.reachable) {
 		log("check_store: ENOMEM");
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index df64c87efc..6d40aefc63 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -1017,7 +1017,7 @@ void domain_init(int evtfd)
 	int rc;
 
 	/* Start with a random rather low domain count for the hashtable. */
-	domhash = create_hashtable(NULL, 8, domhash_fn, domeq_fn, 0);
+	domhash = create_hashtable(NULL, "domains", domhash_fn, domeq_fn, 0);
 	if (!domhash)
 		barf_perror("Failed to allocate domain hashtable");
 
@@ -1804,7 +1804,7 @@ struct hashtable *domain_check_acc_init(void)
 {
 	struct hashtable *domains;
 
-	domains = create_hashtable(NULL, 8, domhash_fn, domeq_fn,
+	domains = create_hashtable(NULL, "domain_check", domhash_fn, domeq_fn,
 				   HASHTABLE_FREE_VALUE);
 	if (!domains)
 		return NULL;
-- 
2.35.3
Re: [PATCH v2 03/13] tools/xenstore: modify interface of create_hashtable()
Posted by Julien Grall 2 years, 9 months ago
Hi Juergen,

On 30/03/2023 09:50, Juergen Gross wrote:
> The minsize parameter of create_hashtable() doesn't have any real use
> case for Xenstore, so drop it.
> 
> For better talloc_report_full() diagnostic output add a name parameter
> to create_hashtable().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   tools/xenstore/hashtable.c        | 20 ++++++--------------
>   tools/xenstore/hashtable.h        |  4 ++--
>   tools/xenstore/xenstored_core.c   |  2 +-
>   tools/xenstore/xenstored_domain.c |  4 ++--
>   4 files changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
> index c1b11743bb..ab1e687d0b 100644
> --- a/tools/xenstore/hashtable.c
> +++ b/tools/xenstore/hashtable.c
> @@ -55,36 +55,28 @@ static unsigned int loadlimit(unsigned int pindex)
>       return ((uint64_t)primes[pindex] * MAX_LOAD_PERCENT) / 100;
>   }
>   
> -struct hashtable *create_hashtable(const void *ctx, unsigned int minsize,
> +struct hashtable *create_hashtable(const void *ctx, const char *name,
>                                      unsigned int (*hashf) (const void *),
>                                      int (*eqf) (const void *, const void *),
>                                      unsigned int flags)
>   {
>       struct hashtable *h;
> -    unsigned int pindex, size = primes[0];
> -
> -    /* Check requested hashtable isn't too large */
> -    if (minsize > (1u << 30)) return NULL;
> -
> -    /* Enforce size as prime */
> -    for (pindex=0; pindex < PRIME_TABLE_LEN; pindex++) {
> -        if (primes[pindex] > minsize) { size = primes[pindex]; break; }
> -    }
>   
>       h = talloc_zero(ctx, struct hashtable);
>       if (NULL == h)
>           goto err0;
> -    h->table = talloc_zero_array(h, struct entry *, size);
> +    talloc_set_name_const(h, name);
> +    h->table = talloc_zero_array(h, struct entry *, primes[0]);
>       if (NULL == h->table)
>           goto err1;
>   
> -    h->tablelength  = size;
> +    h->tablelength  = primes[0];

I find the connection between this line, ...

>       h->flags        = flags;
> -    h->primeindex   = pindex;
> +    h->primeindex   = 0;

... this one and ...

>       h->entrycount   = 0;
>       h->hashfn       = hashf;
>       h->eqfn         = eqf;
> -    h->loadlimit    = loadlimit(pindex);
> +    h->loadlimit    = loadlimit(0);

... now more difficult to find. How about setting h->primeindex first 
and then using it in place of 0?

>       return h;
>   
>   err1:
> diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h
> index 04310783b6..0e1a6f61c2 100644
> --- a/tools/xenstore/hashtable.h
> +++ b/tools/xenstore/hashtable.h
> @@ -10,7 +10,7 @@ struct hashtable;
>      
>    * @name                    create_hashtable
>    * @param   ctx             talloc context to use for allocations
> - * @param   minsize         minimum initial size of hashtable
> + * @param   name            talloc name of the hashtable
>    * @param   hashfunction    function for hashing keys
>    * @param   key_eq_fn       function for determining key equality
>    * @param   flags           flags HASHTABLE_*
> @@ -23,7 +23,7 @@ struct hashtable;
>   #define HASHTABLE_FREE_KEY   (1U << 1)
>   
>   struct hashtable *
> -create_hashtable(const void *ctx, unsigned int minsize,
> +create_hashtable(const void *ctx, const char *name,
>                    unsigned int (*hashfunction) (const void *),
>                    int (*key_eq_fn) (const void *, const void *),
>                    unsigned int flags
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 7214b3df03..6ce7be3161 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -2511,7 +2511,7 @@ void check_store(void)
>   	struct check_store_data data;
>   
>   	/* Don't free values (they are all void *1) */
> -	data.reachable = create_hashtable(NULL, 16, hash_from_key_fn,
> +	data.reachable = create_hashtable(NULL, "checkstore", hash_from_key_fn,
>   					  keys_equal_fn, HASHTABLE_FREE_KEY);
>   	if (!data.reachable) {
>   		log("check_store: ENOMEM");
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index df64c87efc..6d40aefc63 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -1017,7 +1017,7 @@ void domain_init(int evtfd)
>   	int rc;
>   
>   	/* Start with a random rather low domain count for the hashtable. */
> -	domhash = create_hashtable(NULL, 8, domhash_fn, domeq_fn, 0);
> +	domhash = create_hashtable(NULL, "domains", domhash_fn, domeq_fn, 0);
>   	if (!domhash)
>   		barf_perror("Failed to allocate domain hashtable");
>   
> @@ -1804,7 +1804,7 @@ struct hashtable *domain_check_acc_init(void)
>   {
>   	struct hashtable *domains;
>   
> -	domains = create_hashtable(NULL, 8, domhash_fn, domeq_fn,
> +	domains = create_hashtable(NULL, "domain_check", domhash_fn, domeq_fn,
>   				   HASHTABLE_FREE_VALUE);
>   	if (!domains)
>   		return NULL;

Cheers,

-- 
Julien Grall
Re: [PATCH v2 03/13] tools/xenstore: modify interface of create_hashtable()
Posted by Juergen Gross 2 years, 9 months ago
On 03.05.23 14:59, Julien Grall wrote:
> Hi Juergen,
> 
> On 30/03/2023 09:50, Juergen Gross wrote:
>> The minsize parameter of create_hashtable() doesn't have any real use
>> case for Xenstore, so drop it.
>>
>> For better talloc_report_full() diagnostic output add a name parameter
>> to create_hashtable().
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   tools/xenstore/hashtable.c        | 20 ++++++--------------
>>   tools/xenstore/hashtable.h        |  4 ++--
>>   tools/xenstore/xenstored_core.c   |  2 +-
>>   tools/xenstore/xenstored_domain.c |  4 ++--
>>   4 files changed, 11 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
>> index c1b11743bb..ab1e687d0b 100644
>> --- a/tools/xenstore/hashtable.c
>> +++ b/tools/xenstore/hashtable.c
>> @@ -55,36 +55,28 @@ static unsigned int loadlimit(unsigned int pindex)
>>       return ((uint64_t)primes[pindex] * MAX_LOAD_PERCENT) / 100;
>>   }
>> -struct hashtable *create_hashtable(const void *ctx, unsigned int minsize,
>> +struct hashtable *create_hashtable(const void *ctx, const char *name,
>>                                      unsigned int (*hashf) (const void *),
>>                                      int (*eqf) (const void *, const void *),
>>                                      unsigned int flags)
>>   {
>>       struct hashtable *h;
>> -    unsigned int pindex, size = primes[0];
>> -
>> -    /* Check requested hashtable isn't too large */
>> -    if (minsize > (1u << 30)) return NULL;
>> -
>> -    /* Enforce size as prime */
>> -    for (pindex=0; pindex < PRIME_TABLE_LEN; pindex++) {
>> -        if (primes[pindex] > minsize) { size = primes[pindex]; break; }
>> -    }
>>       h = talloc_zero(ctx, struct hashtable);
>>       if (NULL == h)
>>           goto err0;
>> -    h->table = talloc_zero_array(h, struct entry *, size);
>> +    talloc_set_name_const(h, name);
>> +    h->table = talloc_zero_array(h, struct entry *, primes[0]);
>>       if (NULL == h->table)
>>           goto err1;
>> -    h->tablelength  = size;
>> +    h->tablelength  = primes[0];
> 
> I find the connection between this line, ...
> 
>>       h->flags        = flags;
>> -    h->primeindex   = pindex;
>> +    h->primeindex   = 0;
> 
> ... this one and ...
> 
>>       h->entrycount   = 0;
>>       h->hashfn       = hashf;
>>       h->eqfn         = eqf;
>> -    h->loadlimit    = loadlimit(pindex);
>> +    h->loadlimit    = loadlimit(0);
> 
> ... now more difficult to find. How about setting h->primeindex first and then 
> using it in place of 0?

Fine with me.


Juergen