Re: [dpdk-dev] [PATCH RFC 06/11] mbuf: replace data pointer by an offset

[Neil Horman <nhorman@tuxdriver.com>]

On Mon, May 12, 2014 at 02:36:12PM +0000, Venkatesan, Venky wrote:
> Olivier, 
> 
> This is a hugely problematic change, and has a pretty large  performance impact (because the dependency to compute and access). We debated this for a long time during the early days of DPDK and decided against it. This is also a repeated sequence - the driver will do it twice (Rx + Tx) and the next level stack will do it twice (Rx + Tx) ... 
> 
> My vote is to reject this change particular change to the mbuf. 
> 
> Regards, 
> -Venky
> 
Do you have perforamance numbers to compare throughput with and without this
change?  I always feel suspcious when I see the spectre of performane used to
support or deny a change without supporting reasoning or metrics.

Neil

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, May 12, 2014 7:13 AM
> To: Olivier Matz
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH RFC 06/11] mbuf: replace data pointer by an offset
> 
> Hi Olivier,
> 
> 2014-05-09 16:50, Olivier Matz:
> > The mbuf structure already contains a pointer to the beginning of the 
> > buffer (m->buf_addr). It is not needed to use 8 bytes again to store 
> > another pointer to the beginning of the data.
> > 
> > Using a 16 bits unsigned integer is enough as we know that a mbuf is 
> > never longer than 64KB. We gain 6 bytes in the structure thanks to 
> > this modification.
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> [...]
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -132,6 +132,13 @@ struct rte_mbuf {
> >  	void *buf_addr;           /**< Virtual address of segment buffer. */
> >  	uint64_t buf_physaddr:48; /**< Physical address of segment buffer. */
> >  	uint64_t buf_len:16;      /**< Length of segment buffer. */
> > +
> > +	/* valid for any segment */
> > +	struct rte_mbuf *next;    /**< Next segment of scattered packet. */
> > +	uint16_t data_off;
> > +	uint16_t data_len;        /**< Amount of data in segment buffer. */
> > +	uint32_t pkt_len;         /**< Total pkt len: sum of all segments. */
> > +
> >  #ifdef RTE_MBUF_REFCNT
> >  	/**
> >  	 * 16-bit Reference counter.
> > @@ -142,36 +149,30 @@ struct rte_mbuf {
> >  	 * config option.
> >  	 */
> >  	union {
> > -		rte_atomic16_t refcnt_atomic;   /**< Atomically accessed refcnt */
> > -		uint16_t refcnt;                /**< Non-atomically accessed refcnt 
> */
> > +		rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */
> > +		uint16_t refcnt;  /**< Non-atomically accessed refcnt */
> >  	};
> >  #else
> > -	uint16_t refcnt_reserved;     /**< Do not use this field */
> > +	uint16_t refcnt_reserved; /**< Do not use this field */
> >  #endif
> > 
> > -	uint16_t ol_flags;            /**< Offload features. */
> > -	uint32_t reserved;             /**< Unused field. Required for padding. 
> */
> > -
> > -	/* valid for any segment */
> > -	struct rte_mbuf *next;  /**< Next segment of scattered packet. */
> > -	void* data;             /**< Start address of data in segment buffer. */
> > -	uint16_t data_len;      /**< Amount of data in segment buffer. */
> > -
> >  	/* these fields are valid for first segment only */
> > -	uint8_t nb_segs;        /**< Number of segments. */
> > -	uint8_t in_port;        /**< Input port. */
> > -	uint32_t pkt_len;       /**< Total pkt len: sum of all segment data_len.
> > */ +	uint8_t nb_segs;          /**< Number of segments. */
> > +	uint8_t in_port;          /**< Input port. */
> > +	uint16_t ol_flags;        /**< Offload features. */
> > +	uint16_t reserved;        /**< Unused field. Required for padding. */
> > 
> >  	/* offload features, valid for first segment only */
> >  	union rte_vlan_macip vlan_macip;
> >  	union {
> > -		uint32_t rss;       /**< RSS hash result if RSS enabled */
> > +		uint32_t rss;     /**< RSS hash result if RSS enabled */
> >  		struct {
> >  			uint16_t hash;
> >  			uint16_t id;
> > -		} fdir;             /**< Filter identifier if FDIR enabled */
> > -		uint32_t sched;     /**< Hierarchical scheduler */
> > -	} hash;                 /**< hash information */
> > +		} fdir;           /**< Filter identifier if FDIR enabled */
> > +		uint32_t sched;   /**< Hierarchical scheduler */
> > +	} hash;                   /**< hash information */
> > +	uint64_t reserved2;       /**< Unused field. Required for padding. */
> >  } __rte_cache_aligned;
> 
> There are some cosmetic changes mixed with real changes.
> It make hard to read them.
> Please split this patch.
> 
> --
> Thomas
>