Re: [PATCH v4 01/18] mbuf: deprecate GCC marker in rte mbuf struct

[Thomas Monjalon <thomas@monjalon.net>]

15/02/2024 07:21, Tyler Retzlaff:
> Provide a macro that allows conditional expansion of RTE_MARKER fields
> to empty to allow rte_mbuf to be used with MSVC. It is proposed that
> we announce the fields to be __rte_deprecated (currently disabled).
> 
> Introduce C11 anonymous unions to permit aliasing of well-known
> offsets by name into the rte_mbuf structure by a *new* name and to
> provide padding for cache alignment.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  doc/guides/rel_notes/deprecation.rst |  20 ++
>  lib/eal/include/rte_common.h         |   6 +
>  lib/mbuf/rte_mbuf_core.h             | 375 +++++++++++++++++++----------------
>  3 files changed, 233 insertions(+), 168 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 10630ba..8594255 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -17,6 +17,26 @@ Other API and ABI deprecation notices are to be posted below.
>  Deprecation Notices
>  -------------------
>  
> +* mbuf: Named zero sized fields of type ``RTE_MARKER`` and ``RTE_MARKER64``
> +  will be removed from ``struct rte_mbuf`` and replaced with new fields
> +  in anonymous unions.
> +
> +  The names of the fields impacted are:
> +
> +    old name                  new name
> +
> +  ``cacheline0``            ``mbuf_cachelin0``
> +  ``rearm_data``            ``mbuf_rearm_data``
> +  ``rx_descriptor_fields1`` ``mbuf_rx_descriptor_fields1``
> +  ``cacheline1``            ``mbuf_cachelin1``
> +
> +  Contributions to DPDK should immediately start using the new names,
> +  applications should adapt to new names as soon as possible as the
> +  old names will be removed in a future DPDK release.
> +
> +  Note: types of the new names are not API compatible with the old and
> +  some code conversion is required to maintain correct behavior.
> +
>  * build: The ``enable_kmods`` option is deprecated and will be removed in a future release.
>    Setting/clearing the option has no impact on the build.
>    Instead, kernel modules will be always built for OS's where out-of-tree kernel modules
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index d7d6390..af73f67 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -582,6 +582,12 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>  /** Marker for 8B alignment in a structure. */
>  __extension__ typedef uint64_t RTE_MARKER64[0];
>  
> +#define __rte_marker(type, name) type name /* __rte_deprecated */
> +
> +#else
> +
> +#define __rte_marker(type, name)
> +
>  #endif
>  
>  /*********** Macros for calculating min and max **********/
> diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> index 5688683..9e9590b 100644
> --- a/lib/mbuf/rte_mbuf_core.h
> +++ b/lib/mbuf/rte_mbuf_core.h
> @@ -16,7 +16,10 @@
>   * New fields and flags should fit in the "dynamic space".
>   */
>  
> +#include <assert.h>
> +#include <stdalign.h>
>  #include <stdint.h>
> +#include <stddef.h>
>  
>  #include <rte_byteorder.h>
>  #include <rte_stdatomic.h>
> @@ -464,204 +467,240 @@ enum {
>   * The generic rte_mbuf, containing a packet mbuf.
>   */
>  struct rte_mbuf {
> -	RTE_MARKER cacheline0;
> -
> -	void *buf_addr;           /**< Virtual address of segment buffer. */
> +	__rte_marker(RTE_MARKER, cacheline0);

You don't need to keep the first argument.
This would be simpler:
	__rte_marker(cacheline0);
You just need to create 2 functions: __rte_marker and __rte_marker64.

You should replace all occurrences of RTE_MARKER in DPDK in one patch,
and mark RTE_MARKER as deprecated (use #pragma GCC poison)

> +	union {
> +		char mbuf_cacheline0[RTE_CACHE_LINE_MIN_SIZE];
> +		__extension__
> +		struct {
> +			void *buf_addr;           /**< Virtual address of segment buffer. 

I think it is ugly.

Changing mbuf API is a serious issue.