[dpdk-dev] [PATCH RFC 06/11] mbuf: replace data pointer by an offset
Neil Horman
nhorman at tuxdriver.com
Mon May 12 16:41:08 CEST 2014
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
>
More information about the dev
mailing list