[PATCH v1 2/4] mbuf: add second dynamic field member for VA only build

[Bruce Richardson]

On Mon, Aug 29, 2022 at 08:32:20PM +0200, Morten Brørup wrote:
> 
> > From: Shijith Thotton [mailto:sthotton at marvell.com]
> > Sent: Monday, 29 August 2022 17.16
> > 
> > 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 at 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 81cb07c2e4..98ce62fd6a 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 defined).
> > -	 * 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
> > defined).
> > +		 * 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;
> > +	};
> > 
> >  	/* next 8 bytes are initialised on RX descriptor rearm */
> >  	RTE_MARKER64 rearm_data;
> 
> I know that the intention here is to keep the rte_mbuf structure intact, which will certainly improve the probability of getting this patch series into DPDK.
> 
> So, I will add a comment for the benefit of the other participants in the discussion:
> 
> With this patch, and in RTE_IOVA_AS_VA mode, it becomes possible to move m->next into the first cache line, so rte_pktmbuf_prefree_seg() does not have to touch the second cache line, thus potentially improving performance by eliminating one cache miss per freed packet segment. (I also recall someone mentioning that some PMDs set m->next on RX... If that is the case, a cache miss per packet might also be avoidable in those PMDs.)
> 
> Obviously, moving m->next to the first cache line is not related to this patch series, but would belong in a completely different patch.
>

+1 to that, with the exception that if it is decided to move the next
pointer rather than use this as dynamic space, I think it *should* be in
this patch series, rather than mucking about with mbuf twice. :-)