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 -----Original Message----- From: dev [mailto:dev-boun...@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
