Hi Bruce, > -----Original Message----- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Richardson, Bruce > Sent: Friday, July 04, 2014 12:39 AM > To: dev at dpdk.org > Subject: [dpdk-dev] Making space in mbuf data-structure > > Hi all, > > At this stage it's been well recognised that we need to make more space in > the mbuf data structure for new fields. We in Intel have > had some discussion on this and this email is to outline what our current > thinking and approach on this issue is, and look for additional > suggestions and feedback. > > Firstly, we believe that there is no possible way that we can ever fit all > the fields we need to fit into a 64-byte mbuf, and so we need to > start looking at a 128-byte mbuf instead. Examples of new fields that need to > fit in, include - 32-64 bits for additional offload flags, 32- > bits more for filter information for support for the new filters in the i40e > driver, an additional 2-4 bytes for storing info on a second > vlan tag, 4-bytes for storing a sequence number to enable packet reordering > in future, as well as potentially a number of other fields > or splitting out fields that are superimposed over each other right now, e.g. > for the qos scheduler. We also want to allow space for use > by other non-Intel NIC drivers that may be open-sourced to dpdk.org in the > future too, where they support fields and offloads that > our hardware doesn't. > > If we accept the fact of a 2-cache-line mbuf, then the issue becomes how to > rework things so that we spread our fields over the two > cache lines while causing the lowest slow-down possible. The general approach > that we are looking to take is to focus the first cache > line on fields that are updated on RX , so that receive only deals with one > cache line. The second cache line can be used for application > data and information that will only be used on the TX leg. This would allow > us to work on the first cache line in RX as now, and have the > second cache line being prefetched in the background so that it is available > when necessary. Hardware prefetches should help us out > here. We also may move rarely used, or slow-path RX fields e.g. such as those > for chained mbufs with jumbo frames, to the second > cache line, depending upon the performance impact and bytes savings achieved. > > With this approach, we still need to make space in the first cache line for > information for the new or expanded receive offloads. First > off the blocks is to look at moving the mempool pointer into the second cache > line. This will free-up 8 bytes in cache line 0, with a field > that is only used when cleaning up after TX. A prototype patch for this > change is given below, for your review and comment. Initial > quick tests with testpmd (testpmd -c 600 -n 4 -- --mbcache=250 > --txqflags=0xf01 --burst=32 --txrst=32 --txfreet=32 --rxfreet=32 -- > total-num-mbufs=65536), and l3fwd (l3fwd -c 400 -n 4 -- -p F -P > --config="(0,0,10),(1,0,10),(2,0,10),(3,0,10)") showed only a slight > performance decrease with testpmd and equal or slightly better performance > with l3fwd. These would both have been using the > vector PMD - I haven't looked to change the fallback TX implementation yet > for this change, since it's not as performance optimized > and hence cycle-count sensitive.
Regarding code changes itself: If I understand your code changes correctly: ixgbe_tx_free_bufs() will use rte_mempool_put_bulk() *only* if all mbufs in the bulk belong to the same mempool. If we have let say 2 groups of mbufs from 2 different mempools - it wouldn't be able to use put_bulk. While, as I remember, current implementation is able to use put_bulk() in such case. I think, it is possible to change you code to do a better grouping. > > Beyond this change, I'm also investigating potentially moving the "next" > pointer to the second cache line, but it's looking harder to > move without serious impact, so we'll have to see there what can be done. > Other fields I'll examine in due course too, and send on > any findings I may have. Could you explain it a bit? I always had an impression that moving next pointer to the 2-nd cache line would have less impact then moving mempool pointer. My thinking was: next is used only in scatter versions of RX/TX which are slower than optimised RX/TX anyway. So few extra cycles wouldn't be that noticeable. Though I admit I never did any measurements for that case. About space savings for the first cache line: - I still think that Oliver's suggestion of replacing data pointer (8 bytes) with data offset (2 bytes) makes a lot of sense. > > Once we have freed up space, then we can start looking at what fields get to > use that space and what way we shuffle the existing > fields about, but that's a discussion for another day! > > Please see patch below for moving pool pointer to second cache line of mbuf. > All feedback welcome, naturally. > > Regards, > /Bruce > > diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c > index 2b87521..e0cf30c 100644 > --- a/app/test/test_mbuf.c > +++ b/app/test/test_mbuf.c > @@ -832,7 +832,7 @@ test_failing_mbuf_sanity_check(void) > int > test_mbuf(void) > { > - RTE_BUILD_BUG_ON(sizeof(struct rte_mbuf) != 64); > + RTE_BUILD_BUG_ON(sizeof(struct rte_mbuf) != CACHE_LINE_SIZE*2); > > /* create pktmbuf pool if it does not exist */ > if (pktmbuf_pool == NULL) { > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > index 2735f37..aa3ec32 100644 > --- a/lib/librte_mbuf/rte_mbuf.h > +++ b/lib/librte_mbuf/rte_mbuf.h > @@ -181,7 +181,8 @@ enum rte_mbuf_type { > * The generic rte_mbuf, containing a packet mbuf or a control mbuf. > */ > struct rte_mbuf { > - struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */ > + uint64_t this_space_for_rent; // 8-bytes free for reuse. > + > 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. */ > @@ -210,12 +211,16 @@ struct rte_mbuf { > struct rte_pktmbuf pkt; > }; > > +// second cache line starts here > + struct rte_mempool *pool __rte_cache_aligned; > + /**< Pool from which mbuf was allocated. */ > + > union { > uint8_t metadata[0]; > uint16_t metadata16[0]; > uint32_t metadata32[0]; > uint64_t metadata64[0]; > - }; > + } __rte_cache_aligned; > } __rte_cache_aligned; > > #define RTE_MBUF_METADATA_UINT8(mbuf, offset) \ > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.h > b/lib/librte_pmd_ixgbe/ixgbe_rxtx.h > index 64c0695..7374e0d 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.h > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.h > @@ -171,10 +171,6 @@ struct igb_tx_queue { > volatile union ixgbe_adv_tx_desc *tx_ring; > uint64_t tx_ring_phys_addr; /**< TX ring DMA address. */ > struct igb_tx_entry *sw_ring; /**< virtual address of SW ring. */ > -#ifdef RTE_IXGBE_INC_VECTOR > - /** continuous tx entry sequence within the same mempool */ > - struct igb_tx_entry_seq *sw_ring_seq; > -#endif > volatile uint32_t *tdt_reg_addr; /**< Address of TDT register. */ > uint16_t nb_tx_desc; /**< number of TX descriptors. */ > uint16_t tx_tail; /**< current value of TDT reg. */ > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > index 09e19a3..e9fd739 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > @@ -401,9 +401,8 @@ static inline int __attribute__((always_inline)) > ixgbe_tx_free_bufs(struct igb_tx_queue *txq) > { > struct igb_tx_entry_v *txep; > - struct igb_tx_entry_seq *txsp; > uint32_t status; > - uint32_t n, k; > + uint32_t n; > #ifdef RTE_MBUF_SCATTER_GATHER > uint32_t i; > int nb_free = 0; > @@ -423,25 +422,36 @@ ixgbe_tx_free_bufs(struct igb_tx_queue *txq) > */ > txep = &((struct igb_tx_entry_v *)txq->sw_ring)[txq->tx_next_dd - > (n - 1)]; > - txsp = &txq->sw_ring_seq[txq->tx_next_dd - (n - 1)]; > > - while (n > 0) { > - k = RTE_MIN(n, txsp[n-1].same_pool); > #ifdef RTE_MBUF_SCATTER_GATHER > - for (i = 0; i < k; i++) { > - m = __rte_pktmbuf_prefree_seg((txep+n-k+i)->mbuf); > + m = __rte_pktmbuf_prefree_seg(txep[0].mbuf); > + if (likely(m != NULL)) { > + free[0] = m; > + nb_free = 1; > + for (i = 1; i < n; i++) { > + m = __rte_pktmbuf_prefree_seg(txep[i].mbuf); > + if (likely(m != NULL)) { > + if (likely(m->pool == free[0]->pool)) > + free[nb_free++] = m; > + else > + rte_mempool_put(m->pool, m); > + } > + } > + rte_mempool_put_bulk(free[0]->pool, (void **)free, nb_free); > + } > + else { > + for (i = 1; i < n; i++) { > + m = __rte_pktmbuf_prefree_seg(txep[i].mbuf); > if (m != NULL) > - free[nb_free++] = m; > + rte_mempool_put(m->pool, m); > } > - rte_mempool_put_bulk((void *)txsp[n-1].pool, > - (void **)free, nb_free); > -#else > - rte_mempool_put_bulk((void *)txsp[n-1].pool, > - (void **)(txep+n-k), k); > -#endif > - n -= k; > } > > +#else /* no scatter_gather */ > + for (i = 0; i < n; i++) > + rte_mempool_put(m->pool, txep[i].mbuf); > +#endif /* scatter_gather */ > + > /* buffers were freed, update counters */ > txq->nb_tx_free = (uint16_t)(txq->nb_tx_free + txq->tx_rs_thresh); > txq->tx_next_dd = (uint16_t)(txq->tx_next_dd + txq->tx_rs_thresh); > @@ -453,19 +463,11 @@ ixgbe_tx_free_bufs(struct igb_tx_queue *txq) > > static inline void __attribute__((always_inline)) > tx_backlog_entry(struct igb_tx_entry_v *txep, > - struct igb_tx_entry_seq *txsp, > struct rte_mbuf **tx_pkts, uint16_t nb_pkts) > { > int i; > - for (i = 0; i < (int)nb_pkts; ++i) { > + for (i = 0; i < (int)nb_pkts; ++i) > txep[i].mbuf = tx_pkts[i]; > - /* check and update sequence number */ > - txsp[i].pool = tx_pkts[i]->pool; > - if (txsp[i-1].pool == tx_pkts[i]->pool) > - txsp[i].same_pool = txsp[i-1].same_pool + 1; > - else > - txsp[i].same_pool = 1; > - } > } > > uint16_t > @@ -475,7 +477,6 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf > **tx_pkts, > struct igb_tx_queue *txq = (struct igb_tx_queue *)tx_queue; > volatile union ixgbe_adv_tx_desc *txdp; > struct igb_tx_entry_v *txep; > - struct igb_tx_entry_seq *txsp; > uint16_t n, nb_commit, tx_id; > __m128i flags = _mm_set_epi32(DCMD_DTYP_FLAGS, 0, 0, 0); > __m128i rs = _mm_set_epi32(IXGBE_ADVTXD_DCMD_RS|DCMD_DTYP_FLAGS, > @@ -495,14 +496,13 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf > **tx_pkts, > tx_id = txq->tx_tail; > txdp = &txq->tx_ring[tx_id]; > txep = &((struct igb_tx_entry_v *)txq->sw_ring)[tx_id]; > - txsp = &txq->sw_ring_seq[tx_id]; > > txq->nb_tx_free = (uint16_t)(txq->nb_tx_free - nb_pkts); > > n = (uint16_t)(txq->nb_tx_desc - tx_id); > if (nb_commit >= n) { > > - tx_backlog_entry(txep, txsp, tx_pkts, n); > + tx_backlog_entry(txep, tx_pkts, n); > > for (i = 0; i < n - 1; ++i, ++tx_pkts, ++txdp) > vtx1(txdp, *tx_pkts, flags); > @@ -517,10 +517,9 @@ ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf > **tx_pkts, > /* avoid reach the end of ring */ > txdp = &(txq->tx_ring[tx_id]); > txep = &(((struct igb_tx_entry_v *)txq->sw_ring)[tx_id]); > - txsp = &(txq->sw_ring_seq[tx_id]); > } > > - tx_backlog_entry(txep, txsp, tx_pkts, nb_commit); > + tx_backlog_entry(txep, tx_pkts, nb_commit); > > vtx(txdp, tx_pkts, nb_commit, flags); > > @@ -544,7 +543,6 @@ ixgbe_tx_queue_release_mbufs(struct igb_tx_queue *txq) > { > unsigned i; > struct igb_tx_entry_v *txe; > - struct igb_tx_entry_seq *txs; > uint16_t nb_free, max_desc; > > if (txq->sw_ring != NULL) { > @@ -562,10 +560,6 @@ ixgbe_tx_queue_release_mbufs(struct igb_tx_queue *txq) > for (i = 0; i < txq->nb_tx_desc; i++) { > txe = (struct igb_tx_entry_v *)&txq->sw_ring[i]; > txe->mbuf = NULL; > - > - txs = &txq->sw_ring_seq[i]; > - txs->pool = NULL; > - txs->same_pool = 0; > } > } > } > @@ -580,11 +574,6 @@ ixgbe_tx_free_swring(struct igb_tx_queue *txq) > rte_free((struct igb_rx_entry *)txq->sw_ring - 1); > txq->sw_ring = NULL; > } > - > - if (txq->sw_ring_seq != NULL) { > - rte_free(txq->sw_ring_seq - 1); > - txq->sw_ring_seq = NULL; > - } > } > > static void > @@ -593,7 +582,6 @@ ixgbe_reset_tx_queue(struct igb_tx_queue *txq) > static const union ixgbe_adv_tx_desc zeroed_desc = { .read = { > .buffer_addr = 0} }; > struct igb_tx_entry_v *txe = (struct igb_tx_entry_v *)txq->sw_ring; > - struct igb_tx_entry_seq *txs = txq->sw_ring_seq; > uint16_t i; > > /* Zero out HW ring memory */ > @@ -605,8 +593,6 @@ ixgbe_reset_tx_queue(struct igb_tx_queue *txq) > volatile union ixgbe_adv_tx_desc *txd = &txq->tx_ring[i]; > txd->wb.status = IXGBE_TXD_STAT_DD; > txe[i].mbuf = NULL; > - txs[i].pool = NULL; > - txs[i].same_pool = 0; > } > > txq->tx_next_dd = (uint16_t)(txq->tx_rs_thresh - 1); > @@ -632,27 +618,14 @@ static struct ixgbe_txq_ops vec_txq_ops = { > }; > > int ixgbe_txq_vec_setup(struct igb_tx_queue *txq, > - unsigned int socket_id) > + __rte_unused unsigned int socket_id) > { > - uint16_t nb_desc; > - > if (txq->sw_ring == NULL) > return -1; > > - /* request addtional one entry for continous sequence check */ > - nb_desc = (uint16_t)(txq->nb_tx_desc + 1); > - > - txq->sw_ring_seq = rte_zmalloc_socket("txq->sw_ring_seq", > - sizeof(struct igb_tx_entry_seq) * nb_desc, > - CACHE_LINE_SIZE, socket_id); > - if (txq->sw_ring_seq == NULL) > - return -1; > - > - > /* leave the first one for overflow */ > txq->sw_ring = (struct igb_tx_entry *) > ((struct igb_tx_entry_v *)txq->sw_ring + 1); > - txq->sw_ring_seq += 1; > txq->ops = &vec_txq_ops; > > return 0;