Re: [dpdk-dev] [PATCH] net/mlx5: introduce mlx5 hash list

[Bing Zhao <bingz@mellanox.com>]

Hi Stephen,
Many thanks for your reviewing and comments. PSB.

BR. Bing

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, November 6, 2019 1:25 AM
> To: Bing Zhao <bingz@mellanox.com>
> Cc: Ori Kam <orika@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: introduce mlx5 hash list
> 
> On Tue,  5 Nov 2019 17:28:00 +0200
> Bing Zhao <bingz@mellanox.com> wrote:
> 
> > +struct mlx5_hlist *
> > +mlx5_hlist_create(char *name, uint64_t size,
> > +mlx5_hlist_destroy_callback_fn cb)
> 
> Name is unused in rte_malloc, so why bother with it here?

[Bing]: The name is also saved in the hash list, though it is never used
either in the following stage. But saving such string will give some help when
facing some memory corruption issue. And in practice, such const string in
rte_*alloc_* functions will help to record more allocation information if
there is a wrap for debugging.

> > +{
> > +	struct mlx5_hlist *h;
> > +	uint64_t act_size;
> > +	uint64_t alloc_size;
> > +
> > +	if (!size)
> > +		return NULL;
> > +	/* If the low 32B is power of 2, then the 64B integer is too. */
> > +	if (!rte_is_power_of_2((uint32_t)size)) {
> 
> If you are doing cast, therefore you are truncating, therefore size can't
> really be a uint64_t and should be size_t or uint32_t.


[Bing]: Nice catch, thanks. Yes, if is truncated, the validation might be
incorrect, 0x11 1000 0000 is not a power of 2 even if 0x1000 0000 is.
And indeed, there is no such big array of head is needed in practice. U32
should be enough. I will correct it.

> 
> > +		act_size = rte_align64pow2(size);
> > +		DRV_LOG(WARNING, "Size 0x%" PRIX64 " is not power
> of 2, will "
> > +			"be aligned to 0x%" PRIX64 ".\n", size, act_size);
> > +	} else {
> > +		act_size = size;
> > +	}
> 
> Power of 2 hash list size wastes significant amount of memory.

[Bing]: In Koh's implementation, '%' is used. In general, '&' and a little
faster than "%". If '%' is used, then there is no such restriction of the head
array size. And to my understanding, if the hash signature is calculated
perfectly, then the power of 2 hash list size will make all the entries
distributed fairly among the lists.  Correct me if anything wrong.

> 
> > +	alloc_size = sizeof(struct mlx5_hlist) +
> > +		     sizeof(struct mlx5_hlist_head) * act_size;
> > +	/* Using zmalloc, then no need to initialize the heads. */
> > +	h = rte_zmalloc(name, alloc_size, RTE_CACHE_LINE_SIZE);
> Why not use rte_calloc?
> 
> Why use rte_malloc at all? Does this need to be accessed from
> hardware or shared between primary/secondary.  Also rte_malloc has
> less tooling support (coverity, leak checking, etc) than standard malloc.
> 

[Bing]: Yes, there is no need, and for each rte memory allocation, the
overhead is 64B but libc malloc sometimes only occupies 16B. Other stuffs
working together with the lists will be allocated by rte APIs. To my
understanding, it is better to keep them "near" each other inside the memory.
@Slava, what's your opinion?

> 
> > +	if (!h) {
> > +		DRV_LOG(ERR, "No memory for hash list %s
> creation\n",
> > +			name ? name : "None");
> > +		return NULL;
> > +	}
> > +	if (name)
> > +		snprintf(h->name, MLX5_HLIST_NAMESIZE, "%s",
> name);
> > +	if (!cb)
> > +		DRV_LOG(WARNING, "No callback function is specified,
> will use "
> > +			"rte_free when destroying hlist %p %s\n",
> > +			(void *)h, h->name);
> > +	else
> > +		h->cb = cb;
> > +	h->table_sz = act_size;
> > +	h->mask = act_size - 1;
> > +	DRV_LOG(DEBUG, "Hash list with %s size 0x%" PRIX64 ",
> callback "
> > +		"0x%" PRIX64 "is created.\n",
> > +		h->name, act_size, (uint64_t)(uintptr_t)cb);
> > +	return h;
> > +}