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

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

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Sunday, 29 September 2024 17.35
> 
> 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.

This patch defines and uses __rte_alloc_align(). OK.

> 
> Recent 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().

This patch defines __rte_alloc_func(), but uses it in the next patch in the series.
Suggest either doing both here, or move the definition of __rte_alloc_func() to the next patch.


> +/**
> + * Tells the compiler this is a function like malloc and that the
> pointer
> + * returned cannot alias any other pointer (ie new memory).

There's a good example of its use here:
https://developers.redhat.com/blog/2021/04/30/detecting-memory-management-bugs-with-gcc-11-part-1-understanding-dynamic-allocation#detecting_mismatched_deallocations

It not only refers to memory, but also handle pointers.
You might want to replace "ie new memory" by "ie new object" or similar.


Please add the optional arguments to pass to __rte_alloc_func to the macro description, e.g.:
@param [free_func]
  The name of the deallocation function to free the allocated object
@param [free_func_ptr_index]
  The deallocation function's argument index of the object pointer.

PS: The brackets indicate that the parameter is optional. I didn't know, so it is what I found on the internet.

> + *
> + * Also, with recent GCC versions also able to track that proper
> + * dealloctor function is used for this pointer.
> + */
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 110000)
> +#define __rte_alloc_func(...) \
> +	__attribute__((malloc, malloc(__VA_ARGS__)))
> +
> +#elif defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> +#define __rte_alloc_func(...) \
> +	__attribute__((malloc))
> +#else
> +#define __rte_alloc_func(...)
> +#endif

The _func postfix seems superfluous. Macros hinting about Hot and cold functions are simply __rte_hot and __rte_cold, without _func postfix.
It's probably a matter of taste, so I'll leave it up to you.

Minor detail:
When looking at the code using the macro, it seems somewhat confusing that the macro name is "__rte_alloc" when its arguments describe the associated free function.
But I have no ideas for a better name...
Even if the two arguments were required, the primary purpose of the macro is to inform the compiler that the function is an allocation function; so that must be dominant in the name of the macro, which it is with the current name.

With the macro description updated,
Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>