Re: [PATCH v3 2/5] mbuf: add second dynamic field member for VA only build

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

Hi,

On Wed, Sep 21, 2022 at 07:26:18PM +0530, Shijith Thotton wrote:
> mbuf physical address field is not used in builds which only uses VA. It
> is used to expand the dynamic field area.
> 
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---
>  lib/mbuf/rte_mbuf_core.h | 26 +++++++++++++++++---------
>  lib/mbuf/rte_mbuf_dyn.c  |  2 ++
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> index c6292e7252..94907f301d 100644
> --- a/lib/mbuf/rte_mbuf_core.h
> +++ b/lib/mbuf/rte_mbuf_core.h
> @@ -579,15 +579,23 @@ struct rte_mbuf {
>  	RTE_MARKER cacheline0;
>  
>  	void *buf_addr;           /**< Virtual address of segment buffer. */
> -	/**
> -	 * Physical address of segment buffer.
> -	 * This field is invalid if the build is configured to use only
> -	 * virtual address as IOVA (i.e. RTE_IOVA_AS_VA is 1).
> -	 * Force alignment to 8-bytes, so as to ensure we have the exact
> -	 * same mbuf cacheline0 layout for 32-bit and 64-bit. This makes
> -	 * working on vector drivers easier.
> -	 */
> -	rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
> +	RTE_STD_C11
> +	union {
> +		/**
> +		 * Physical address of segment buffer.
> +		 * This field is invalid if the build is configured to use only
> +		 * virtual address as IOVA (i.e. RTE_IOVA_AS_VA is 1).
> +		 * Force alignment to 8-bytes, so as to ensure we have the exact
> +		 * same mbuf cacheline0 layout for 32-bit and 64-bit. This makes
> +		 * working on vector drivers easier.
> +		 */
> +		rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
> +		/**
> +		 * Reserved for dynamic field in builds where physical address
> +		 * field is invalid.
> +		 */
> +		uint64_t dynfield2;
> +	};

Same comment than on previous patch: using a #if instead of the union here looks
better to me to ensure that we never use buf_iova when RTE_IOVA_AS_VA=1.

>  
>  	/* next 8 bytes are initialised on RX descriptor rearm */
>  	RTE_MARKER64 rearm_data;
> diff --git a/lib/mbuf/rte_mbuf_dyn.c b/lib/mbuf/rte_mbuf_dyn.c
> index 4ae79383b5..6a4cf96897 100644
> --- a/lib/mbuf/rte_mbuf_dyn.c
> +++ b/lib/mbuf/rte_mbuf_dyn.c
> @@ -128,6 +128,8 @@ init_shared_mem(void)
>  		 */
>  		memset(shm, 0, sizeof(*shm));
>  		mark_free(dynfield1);
> +		if (RTE_IOVA_AS_VA)
> +			mark_free(dynfield2);

In this case, it will have to be a #if here too.

>  
>  		/* init free_flags */
>  		for (mask = RTE_MBUF_F_FIRST_FREE; mask <= RTE_MBUF_F_LAST_FREE; mask <<= 1)


Also, I think we can add in the RTE_IOVA_AS_VA documentation that it
replaces the buf_iova by 8 bytes of dynamic field.