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

[Stephen Hemminger <stephen@networkplumber.org>]

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?
> +{
> +	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.

> +		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.

> +	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.


> +	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;
> +}