From: Olivier MATZ <olivier.matz@6wind.com>
To: Bruce Richardson <bruce.richardson@intel.com>, dev@dpdk.org
Subject: Re: [dpdk-dev] [RFC PATCH 06/14] mbuf: reorder fields by time-of-use
Date: Tue, 12 Aug 2014 12:03:48 +0200 [thread overview]
Message-ID: <53E9E684.1040001@6wind.com> (raw)
In-Reply-To: <1407789890-17355-7-git-send-email-bruce.richardson@intel.com>
Hi Bruce,
On 08/11/2014 10:44 PM, Bruce Richardson wrote:
> * Reorder the fields in the mbuf so that we have fields that are used
> together side-by-side in the structure. This means that we have a
> contiguous block of 8-bytes in the mbuf which are used to reset an mbuf
> of descriptor rearm.
> * Where needed add in a dummy fields to overwrite values 8 or 16 bytes
> at a time, when doing RX or RX descriptor rearm. This avoids compiler
> warnings when using uint64_t values to overwrite a set of smaller
> values.
> * At the end, place fields that are only used for TX or for the slower
> RX path, and mark them as down to be moved to a second cache line.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> lib/librte_mbuf/rte_mbuf.c | 2 +-
> lib/librte_mbuf/rte_mbuf.h | 37 +++++++++++++++++++++----------------
> 2 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 64f1587..594b910 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -161,7 +161,7 @@ rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
>
> fprintf(f, "dump mbuf at 0x%p, phys=%"PRIx64", buf_len=%u\n",
> m, (uint64_t)m->buf_physaddr, (unsigned)m->buf_len);
> - fprintf(f, " pkt_len=%"PRIu32", ol_flags=%"PRIx16", nb_segs=%u, "
> + fprintf(f, " pkt_len=%"PRIu32", ol_flags=%"PRIx64", nb_segs=%u, "
> "in_port=%u\n", m->pkt_len, m->ol_flags,
> (unsigned)m->nb_segs, (unsigned)m->port);
> nb_segs = m->nb_segs;
I think this should not go in this patch. Another one "change ol_flags
to 64 bits" would be nice.
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index e0981c9..566bb7e 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -132,22 +132,20 @@ union rte_vlan_macip {
> /**< MAC+IP length. */
> #define TX_MACIP_LEN_CMP_MASK (TX_MAC_LEN_CMP_MASK | TX_IP_LEN_CMP_MASK)
>
> +
> /**
Garbage here.
> * The generic rte_mbuf, containing a packet mbuf.
> */
> struct rte_mbuf {
> - struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */
> void *buf_addr; /**< Virtual address of segment buffer. */
> phys_addr_t buf_physaddr; /**< Physical address of segment buffer. */
> - uint16_t buf_len; /**< Length of segment buffer. */
>
> - /* valid for any segment */
> - struct rte_mbuf *next; /**< Next segment of scattered packet. */
> + /* next 8 bytes are initialised on RX descriptor rearm */
> + uint64_t rearm_data[0]; /**< dummy element so we can get uin64_t ptrs
> + * to this part of the mbuf without alias error
> + */
> + uint16_t buf_len; /**< Length of segment buffer. */
> 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. */
What do you think about using an union instead? I'm not sure it's
clearer, but in case of:
union {
uint64_t u64;
struct {
uint16_t buf_len;
uint16_t data_off;
...
};
};
> -
> -#ifdef RTE_MBUF_REFCNT
> /**
> * 16-bit Reference counter.
> * It should only be accessed using the following functions:
> @@ -157,20 +155,23 @@ struct rte_mbuf {
> * config option.
> */
> union {
> +#ifdef RTE_MBUF_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 */
> #endif
> -
> - /* these fields are valid for first segment only */
> + uint16_t refcnt_reserved; /**< Do not use this field */
> + };
I think this should go in another cosmetic patch.
> uint8_t nb_segs; /**< Number of segments. */
> uint8_t port; /**< Input port. */
> - uint16_t ol_flags; /**< Offload features. */
> - uint16_t reserved; /**< Unused field. Required for padding. */
>
> - /* offload features, valid for first segment only */
> + /* remaining bytes are set on RX when pulling packet from descriptor */
> + uint64_t ol_flags; /**< Offload features. */
Should be moved to "change ol_flags to 64 bits".
> +
> + __m128i rx_descriptor_fields1[0]; /**< dummy field used as marker for
> + * writes in a vector driver */
Is it a good idea to have a specific type in the generic mbuf structure?
Moreover it seems that later in your patch series it's replaced by
something else.
Also, the 2nd line of comment mixes tabs and spaces.
>
> + /* second cache line, fields only used in slow path or on TX */
> + struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */
> + struct rte_mbuf *next; /**< Next segment of scattered packet. */
The comment is wrong, this is not a new cache line (introduced later).
Regards,
Olivier
next prev parent reply other threads:[~2014-08-12 10:00 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-11 20:44 [dpdk-dev] [RFC PATCH 00/14] Extend the mbuf structure Bruce Richardson
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 01/14] mbuf: rename RTE_MBUF_SCATTER_GATHER into RTE_MBUF_REFCNT Bruce Richardson
2014-08-11 21:45 ` Stephen Hemminger
2014-08-12 7:59 ` Olivier MATZ
2014-08-12 16:25 ` Richardson, Bruce
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 02/14] mbuf: remove rte_ctrlmbuf Bruce Richardson
2014-08-12 8:27 ` Olivier MATZ
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 03/14] mbuf: remove the rte_pktmbuf structure Bruce Richardson
2014-08-12 8:38 ` Olivier MATZ
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 04/14] mbuf: replace data pointer by an offset Bruce Richardson
2014-08-12 8:55 ` Olivier MATZ
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 05/14] mbuf: rename in_port to just port Bruce Richardson
2014-08-12 9:00 ` Olivier MATZ
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 06/14] mbuf: reorder fields by time-of-use Bruce Richardson
2014-08-12 10:03 ` Olivier MATZ [this message]
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 07/14] ixgbe: rework vector pmd following mbuf changes Bruce Richardson
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 08/14] mbuf: split mbuf across two cache lines Bruce Richardson
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 09/14] Fix performance regression due to moved pool ptr Bruce Richardson
2014-08-12 11:28 ` Olivier MATZ
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 10/14] mbuf: set next pointer to NULL on mbuf free Bruce Richardson
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 11/14] ixgbe: make mbuf_initializer queue variable global Bruce Richardson
2014-08-11 21:47 ` Stephen Hemminger
2014-08-11 22:03 ` Richardson, Bruce
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 12/14] ixgbe: Make vector stores unaligned Bruce Richardson
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 13/14] mbuf: cleanup + added in additional mbuf fields Bruce Richardson
2014-08-12 14:18 ` Olivier MATZ
2014-08-11 20:44 ` [dpdk-dev] [RFC PATCH 14/14] ixgbe: Allow vector RX of scattered packets Bruce Richardson
2014-08-12 14:43 ` [dpdk-dev] [RFC PATCH 00/14] Extend the mbuf structure Olivier MATZ
2014-08-20 12:22 ` Richardson, Bruce
2014-08-20 7:08 ` Cao, Min
2014-08-20 11:11 ` Richardson, Bruce
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53E9E684.1040001@6wind.com \
--to=olivier.matz@6wind.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).