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 at dpdk.org] On Behalf Of Thomas Monjalon > Sent: Monday, May 12, 2014 7:13 AM > To: Olivier Matz > Cc: dev at 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 at 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 >
