On Tue, 5 Nov 2019 17:28:00 +0200
Bing Zhao <bi...@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;
> +}
