RE: [PATCH v6 17/17] eal: add function attributes for allocation functions

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 2 October 2024 17.43
> 
> The allocation functions take a alignment argument that
> can be useful to hint the compiler optimizer. This is supported
> by GCC and Clang but only useful with GCC because Clang gives
> warning if alignment is 0.
> 
> Newer versions of GCC have a malloc attribute that can be used to find
> mismatches between allocation and free; the typical problem caught is a
> pointer allocated with rte_malloc() that is then incorrectly freed
> using
> free(). The name of the DPDK wrapper macros for these attributes
> are chosen to be similar to what GLIBC is using in cdefs.h.
> Note: The rte_free function prototype was moved ahead of the allocation
> functions since the dealloc attribute now refers to it.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

I see many of my comments to v3 have already been addressed. Great minds think alike. :-)

> +/**
> + * With recent GCC versions also able to track that proper
> + * deallocator function is used for this pointer.
> + */
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 110000)
> +#define __rte_dealloc(dealloc, argno) \
> +	__attribute__((malloc(dealloc, argno)))
> +#else
> +#define __rte_dealloc(dealloc, argno)
> +#endif

A matter of taste...
The name "__rte_malloc" is closely associated with the function name "malloc()"; for consistency suggest naming this "__rte_free" or "__rte_malloc_free".

<brainstorming>
If named __rte_malloc_free, it could include the __rte_malloc (as in previous versions of the patch).
However, that might be confusing, so probably not a good idea.
I prefer keeping the attributes separate, as in this version.
</brainstorming>


> +++ b/lib/eal/include/rte_malloc.h
> @@ -31,6 +31,26 @@ struct rte_malloc_socket_stats {
>  	size_t heap_allocsz_bytes; /**< Total allocated bytes on heap */
>  };
> 
> +/**
> + * Function attribut for prototypes that expect to release memory with
> rte_free()

Typo: attribut -> attribute

> + */
> +#define __rte_dealloc_free __rte_dealloc(rte_free, 1)

Minor detail:
I'm worried someone might misunderstand the purpose of this shortcut, and use it with an allocator function where a different deallocator is associated.
Moving it from rte_common.h to rte_malloc.h is a huge improvement; but please consider if the benefit outweighs the risk.
Again, I'll leave it up to you.