[dpdk-dev] [PATCH v2] mbuf:rearrange mbuf to be more mbuf chain friendly
2016-06-27 13:06, Wiles, Keith: > > On 6/27/16, 4:05 AM, "Thomas Monjalon" wrote: > > >2016-06-27 10:27, Olivier Matz: > >> On 06/27/2016 10:21 AM, Ananyev, Konstantin wrote: > >> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Keith Wiles > >> >> Move the next pointer to the first cacheline of the rte_mbuf structure > >> >> and move the offload values to the second cacheline to give better > >> >> performance to applications using chained mbufs. > >> >> > >> >> Enabled by a configuration option CONFIG_RTE_MBUF_CHAIN_FRIENDLY default > >> >> is set to No. > >> > > >> > First, it would make ixgbe and i40e vector RX functions to work > >> > incorrectly. > >> > Second, I don't think we can afford to allow people swap mbuf fields in > >> > the way they like. > >> > Otherwise we'll end-up with totally unmaintainable code pretty soon. > >> > So NACK. > >> > >> +1 > > > >To be more precise, the arrangement of fields in rte_mbuf is open > >to debate and changes. > >There is a recent discussion here: > > http://dpdk.org/ml/archives/dev/2016-May/039483.html > > > >I think we must try to improve few things in mbuf during the 16.11 cycle. > >But it must not be allowed to have a build option to adapt this structure > >or any other API. There is only one DPDK API for a given version. > > I just received a private email thread on this one and it appears it is not a > big of a problem as was stated before. ? So yes we can reject this one. > > Someone rejected these in patchwork already, which I expected I would be the > one to reject the patches. Is this not the case? I understand if the patch > just hangs round, but I would have expected after the list rejected the patch > I would be the one to reject the patches. I try to keep up with my patches > and rejecting a patch before I have a chance to do so seems a bit harsh to me. Yes it's me, sorry I've been quick when I've seen the first 2 comments.
[dpdk-dev] [PATCH v2] mbuf:rearrange mbuf to be more mbuf chain friendly
On 6/27/16, 4:05 AM, "Thomas Monjalon" wrote: >2016-06-27 10:27, Olivier Matz: >> On 06/27/2016 10:21 AM, Ananyev, Konstantin wrote: >> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Keith Wiles >> >> Move the next pointer to the first cacheline of the rte_mbuf structure >> >> and move the offload values to the second cacheline to give better >> >> performance to applications using chained mbufs. >> >> >> >> Enabled by a configuration option CONFIG_RTE_MBUF_CHAIN_FRIENDLY default >> >> is set to No. >> > >> > First, it would make ixgbe and i40e vector RX functions to work >> > incorrectly. >> > Second, I don't think we can afford to allow people swap mbuf fields in >> > the way they like. >> > Otherwise we'll end-up with totally unmaintainable code pretty soon. >> > So NACK. >> >> +1 > >To be more precise, the arrangement of fields in rte_mbuf is open >to debate and changes. >There is a recent discussion here: > http://dpdk.org/ml/archives/dev/2016-May/039483.html > >I think we must try to improve few things in mbuf during the 16.11 cycle. >But it must not be allowed to have a build option to adapt this structure >or any other API. There is only one DPDK API for a given version. I just received a private email thread on this one and it appears it is not a big of a problem as was stated before. ? So yes we can reject this one. Someone rejected these in patchwork already, which I expected I would be the one to reject the patches. Is this not the case? I understand if the patch just hangs round, but I would have expected after the list rejected the patch I would be the one to reject the patches. I try to keep up with my patches and rejecting a patch before I have a chance to do so seems a bit harsh to me. >
[dpdk-dev] [PATCH v2] mbuf:rearrange mbuf to be more mbuf chain friendly
2016-06-27 10:27, Olivier Matz: > On 06/27/2016 10:21 AM, Ananyev, Konstantin wrote: > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Keith Wiles > >> Move the next pointer to the first cacheline of the rte_mbuf structure > >> and move the offload values to the second cacheline to give better > >> performance to applications using chained mbufs. > >> > >> Enabled by a configuration option CONFIG_RTE_MBUF_CHAIN_FRIENDLY default > >> is set to No. > > > > First, it would make ixgbe and i40e vector RX functions to work incorrectly. > > Second, I don't think we can afford to allow people swap mbuf fields in the > > way they like. > > Otherwise we'll end-up with totally unmaintainable code pretty soon. > > So NACK. > > +1 To be more precise, the arrangement of fields in rte_mbuf is open to debate and changes. There is a recent discussion here: http://dpdk.org/ml/archives/dev/2016-May/039483.html I think we must try to improve few things in mbuf during the 16.11 cycle. But it must not be allowed to have a build option to adapt this structure or any other API. There is only one DPDK API for a given version.
[dpdk-dev] [PATCH v2] mbuf:rearrange mbuf to be more mbuf chain friendly
On 06/27/2016 10:21 AM, Ananyev, Konstantin wrote: > > >> -Original Message- >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Keith Wiles >> Sent: Saturday, June 25, 2016 4:56 PM >> To: dev at dpdk.org >> Subject: [dpdk-dev] [PATCH v2] mbuf:rearrange mbuf to be more mbuf chain >> friendly >> >> Move the next pointer to the first cacheline of the rte_mbuf structure >> and move the offload values to the second cacheline to give better >> performance to applications using chained mbufs. >> >> Enabled by a configuration option CONFIG_RTE_MBUF_CHAIN_FRIENDLY default >> is set to No. > > First, it would make ixgbe and i40e vector RX functions to work incorrectly. > Second, I don't think we can afford to allow people swap mbuf fields in the > way they like. > Otherwise we'll end-up with totally unmaintainable code pretty soon. > So NACK. +1
[dpdk-dev] [PATCH v2] mbuf:rearrange mbuf to be more mbuf chain friendly
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Keith Wiles
> Sent: Saturday, June 25, 2016 4:56 PM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH v2] mbuf:rearrange mbuf to be more mbuf chain
> friendly
>
> Move the next pointer to the first cacheline of the rte_mbuf structure
> and move the offload values to the second cacheline to give better
> performance to applications using chained mbufs.
>
> Enabled by a configuration option CONFIG_RTE_MBUF_CHAIN_FRIENDLY default
> is set to No.
First, it would make ixgbe and i40e vector RX functions to work incorrectly.
Second, I don't think we can afford to allow people swap mbuf fields in the way
they like.
Otherwise we'll end-up with totally unmaintainable code pretty soon.
So NACK.
Konstantin
>
> Signed-off-by: Keith Wiles
> ---
> config/common_base | 2 +
> .../linuxapp/eal/include/exec-env/rte_kni_common.h | 8 +++
> lib/librte_mbuf/rte_mbuf.h | 67
> +++---
> 3 files changed, 56 insertions(+), 21 deletions(-)
>
> diff --git a/config/common_base b/config/common_base
> index 379a791..f7c624e 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -405,6 +405,8 @@ CONFIG_RTE_LIBRTE_MBUF_DEBUG=n
> CONFIG_RTE_MBUF_DEFAULT_MEMPOOL_OPS="ring_mp_mc"
> CONFIG_RTE_MBUF_REFCNT_ATOMIC=y
> CONFIG_RTE_PKTMBUF_HEADROOM=128
> +# Set to y if needing to be mbuf chain friendly.
> +CONFIG_RTE_MBUF_CHAIN_FRIENDLY=n
>
> #
> # Compile librte_timer
> diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> b/lib/librte_eal/linuxapp/eal/include/exec-
> env/rte_kni_common.h
> index 2acdfd9..44d65cd 100644
> --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> @@ -120,11 +120,19 @@ struct rte_kni_mbuf {
> char pad2[4];
> uint32_t pkt_len; /**< Total pkt len: sum of all segment
> data_len. */
> uint16_t data_len; /**< Amount of data in segment buffer. */
> +#ifdef RTE_MBUF_CHAIN_FRIENDLY
> + char pad3[8];
> + void *next;
>
> /* fields on second cache line */
> + char pad4[16] __attribute__((__aligned__(RTE_CACHE_LINE_MIN_SIZE)));
> + void *pool;
> +#else
> + /* fields on second cache line */
> char pad3[8] __attribute__((__aligned__(RTE_CACHE_LINE_MIN_SIZE)));
> void *pool;
> void *next;
> +#endif
> };
>
> /*
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 15e3a10..6e6ba0e 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -765,6 +765,28 @@ typedef uint8_t MARKER8[0]; /**< generic marker with
> 1B alignment */
> typedef uint64_t MARKER64[0]; /**< marker that allows us to overwrite 8 bytes
> * with a single assignment */
>
> +typedef union {
> + uint32_t rss; /**< RSS hash result if RSS enabled */
> + struct {
> + union {
> + struct {
> + uint16_t hash;
> + uint16_t id;
> + };
> + uint32_t lo;
> + /**< Second 4 flexible bytes */
> + };
> + uint32_t hi;
> + /**< First 4 flexible bytes or FD ID, dependent on
> + PKT_RX_FDIR_* flag in ol_flags. */
> + } fdir; /**< Filter identifier if FDIR enabled */
> + struct {
> + uint32_t lo;
> + uint32_t hi;
> + } sched; /**< Hierarchical scheduler */
> + uint32_t usr; /**< User defined tags. See rte_distributor_process()
> */
> +} rss_hash_t;
> +
> /**
> * The generic rte_mbuf, containing a packet mbuf.
> */
> @@ -824,28 +846,31 @@ struct rte_mbuf {
> uint16_t data_len;/**< Amount of data in segment buffer. */
> /** VLAN TCI (CPU order), valid if PKT_RX_VLAN_STRIPPED is set. */
> uint16_t vlan_tci;
> +#ifdef RTE_MBUF_CHAIN_FRIENDLY
> + /*
> + * Move offload into the second cache line and next in the first.
> + * Better performance for applications using chained mbufs to have
> + * the next pointer in the first cache line.
> + * If you change this structure, you must change the user-mode
> + * version in rte_kni_common.h to match the new layout.
> + */
> + uint32_t seqn; /**< Sequence number. See also rte_reorder_insert() */
> + uint16_t vlan_tci_outer; /**< Outer VLAN
[dpdk-dev] [PATCH v2] mbuf:rearrange mbuf to be more mbuf chain friendly
Move the next pointer to the first cacheline of the rte_mbuf structure
and move the offload values to the second cacheline to give better
performance to applications using chained mbufs.
Enabled by a configuration option CONFIG_RTE_MBUF_CHAIN_FRIENDLY default
is set to No.
Signed-off-by: Keith Wiles
---
config/common_base | 2 +
.../linuxapp/eal/include/exec-env/rte_kni_common.h | 8 +++
lib/librte_mbuf/rte_mbuf.h | 67 +++---
3 files changed, 56 insertions(+), 21 deletions(-)
diff --git a/config/common_base b/config/common_base
index 379a791..f7c624e 100644
--- a/config/common_base
+++ b/config/common_base
@@ -405,6 +405,8 @@ CONFIG_RTE_LIBRTE_MBUF_DEBUG=n
CONFIG_RTE_MBUF_DEFAULT_MEMPOOL_OPS="ring_mp_mc"
CONFIG_RTE_MBUF_REFCNT_ATOMIC=y
CONFIG_RTE_PKTMBUF_HEADROOM=128
+# Set to y if needing to be mbuf chain friendly.
+CONFIG_RTE_MBUF_CHAIN_FRIENDLY=n
#
# Compile librte_timer
diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
index 2acdfd9..44d65cd 100644
--- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
+++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
@@ -120,11 +120,19 @@ struct rte_kni_mbuf {
char pad2[4];
uint32_t pkt_len; /**< Total pkt len: sum of all segment
data_len. */
uint16_t data_len; /**< Amount of data in segment buffer. */
+#ifdef RTE_MBUF_CHAIN_FRIENDLY
+ char pad3[8];
+ void *next;
/* fields on second cache line */
+ char pad4[16] __attribute__((__aligned__(RTE_CACHE_LINE_MIN_SIZE)));
+ void *pool;
+#else
+ /* fields on second cache line */
char pad3[8] __attribute__((__aligned__(RTE_CACHE_LINE_MIN_SIZE)));
void *pool;
void *next;
+#endif
};
/*
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 15e3a10..6e6ba0e 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -765,6 +765,28 @@ typedef uint8_t MARKER8[0]; /**< generic marker with 1B
alignment */
typedef uint64_t MARKER64[0]; /**< marker that allows us to overwrite 8 bytes
* with a single assignment */
+typedef union {
+ uint32_t rss; /**< RSS hash result if RSS enabled */
+ struct {
+ union {
+ struct {
+ uint16_t hash;
+ uint16_t id;
+ };
+ uint32_t lo;
+ /**< Second 4 flexible bytes */
+ };
+ uint32_t hi;
+ /**< First 4 flexible bytes or FD ID, dependent on
+ PKT_RX_FDIR_* flag in ol_flags. */
+ } fdir; /**< Filter identifier if FDIR enabled */
+ struct {
+ uint32_t lo;
+ uint32_t hi;
+ } sched; /**< Hierarchical scheduler */
+ uint32_t usr; /**< User defined tags. See rte_distributor_process()
*/
+} rss_hash_t;
+
/**
* The generic rte_mbuf, containing a packet mbuf.
*/
@@ -824,28 +846,31 @@ struct rte_mbuf {
uint16_t data_len;/**< Amount of data in segment buffer. */
/** VLAN TCI (CPU order), valid if PKT_RX_VLAN_STRIPPED is set. */
uint16_t vlan_tci;
+#ifdef RTE_MBUF_CHAIN_FRIENDLY
+ /*
+* Move offload into the second cache line and next in the first.
+* Better performance for applications using chained mbufs to have
+* the next pointer in the first cache line.
+* If you change this structure, you must change the user-mode
+* version in rte_kni_common.h to match the new layout.
+*/
+ uint32_t seqn; /**< Sequence number. See also rte_reorder_insert() */
+ uint16_t vlan_tci_outer; /**< Outer VLAN Tag Control Identifier (CPU
order) */
+ struct rte_mbuf *next;/**< Next segment of scattered packet. */
+
+ /* second cache line - fields only used in slow path or on TX */
+ MARKER cacheline1 __rte_cache_min_aligned;
+
+ rss_hash_t hash; /**< hash information */
union {
- uint32_t rss; /**< RSS hash result if RSS enabled */
- struct {
- union {
- struct {
- uint16_t hash;
- uint16_t id;
- };
- uint32_t lo;
- /**< Second 4 flexible bytes */
- };
- uint32_t hi;
- /**< First 4 flexible bytes or FD ID, dependent on
-PKT_RX_FDIR_* flag in ol_flags. */
- } fdir; /**< Filter identifier if FDIR enabled */
- struct {
-
