Re: [PATCH v3 3/5] lib: move mbuf next pointer to first cache line

[Olivier Matz <olivier.matz@6wind.com>]

On Wed, Sep 21, 2022 at 07:26:19PM +0530, Shijith Thotton wrote:
> Swapped position of mbuf next pointer and second dynamic field (dynfield2)
> if the build is configured to use IOVA as VA. This is to move the mbuf
> next pointer to first cache line. kni library is disabled for this
> change as it depends on the offset value of next pointer.
> 
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---
>  lib/mbuf/rte_mbuf_core.h | 29 +++++++++++++++++++++--------
>  lib/meson.build          |  3 +++
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> index 94907f301d..915dcd8653 100644
> --- a/lib/mbuf/rte_mbuf_core.h
> +++ b/lib/mbuf/rte_mbuf_core.h
> @@ -590,11 +590,14 @@ struct rte_mbuf {
>  		 * working on vector drivers easier.
>  		 */
>  		rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
> +#if RTE_IOVA_AS_VA
>  		/**
> -		 * Reserved for dynamic field in builds where physical address
> -		 * field is invalid.
> +		 * Next segment of scattered packet.
> +		 * This field is valid when physical address field is invalid.
> +		 * Otherwise next pointer in the second cache line will be used.
>  		 */
> -		uint64_t dynfield2;
> +		struct rte_mbuf *next;
> +#endif
>  	};
>  
>  	/* next 8 bytes are initialised on RX descriptor rearm */
> @@ -711,11 +714,21 @@ struct rte_mbuf {
>  	/* second cache line - fields only used in slow path or on TX */
>  	RTE_MARKER cacheline1 __rte_cache_min_aligned;
>  
> -	/**
> -	 * Next segment of scattered packet. Must be NULL in the last segment or
> -	 * in case of non-segmented packet.
> -	 */
> -	struct rte_mbuf *next;
> +	RTE_STD_C11
> +	union {
> +#if !RTE_IOVA_AS_VA
> +		/**
> +		 * Next segment of scattered packet. Must be NULL in the last
> +		 * segment or in case of non-segmented packet.
> +		 */
> +		struct rte_mbuf *next;
> +#endif
> +		/**
> +		 * Reserved for dynamic field when the next pointer is in first
> +		 * cache line (i.e. RTE_IOVA_AS_VA is 1).
> +		 */
> +		uint64_t dynfield2;
> +	};

Same comment than other patches about union vs #if.

>  
>  	/* fields to support TX offloads */
>  	RTE_STD_C11
> diff --git a/lib/meson.build b/lib/meson.build
> index c648f7d800..73d93bc803 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -88,6 +88,9 @@ optional_libs = [
>  disabled_libs = []
>  opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
>          check: true).stdout().split()
> +if dpdk_conf.get('RTE_IOVA_AS_VA') == 1
> +    opt_disabled_libs += ['kni']
> +endif

I guess this should be in the previous patch instead, since kni uses
m->buf_iova

>  foreach l:opt_disabled_libs
>      if not optional_libs.contains(l)
>          warning('Cannot disable mandatory library "@0@"'.format(l))


After this change, the documentation of RTE_IOVA_AS_VA can be enhanced to
explain that it also moves the next pointer to the first cache line, possibly
increasing the performance.