* [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT
@ 2014-10-24 8:10 Ouyang Changchun
2014-10-24 8:10 ` [dpdk-dev] [PATCH 1/3] mbuf: Use EXTERNAL_MBUF to indicate external buffer Ouyang Changchun
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Ouyang Changchun @ 2014-10-24 8:10 UTC (permalink / raw)
To: dev
To remove the dependency of RTE_MBUF_REFCNT for vhost zero copy,
the mbuf need introduce EXTERNAL_MBUF(in ol_flags) to indicate it
attaches to an external buffer, say, from guest space. And don't
free the external buffer when freeing the mbuf itself in host, in
addition, RX function in PMD need make sure not overwrite this flag
when filling ol_flags from descriptors to mbuf.
Changchun Ouyang (3):
mbuf use EXTERNAL_MBUF in ol_flags to indicate it is an external
buffer, when freeing such kind of mbuf, just need put mbuf
itself back into mempool, doesn't free the attached external
buffer, user/caller need take care of detaching and freeing the
external buffer.
Every pmd RX function need keep the EXTERNAL_MBUF flag in
mbuf.ol_flags, and can't overwrite it when filling ol_flags from
descriptor to mbuf, otherwise, it probably cause to crash when
freeing a mbuf and trying to freeing its attached external
buffer, say, from guest space.
vhost zero copy removes the dependency on macro REFCNT by using
EXTERNAL_MBUF flag in mbuf.ol_flags to indicate it is an
external buffer from guest.
examples/vhost/main.c | 19 +++++--------------
lib/librte_mbuf/rte_mbuf.h | 5 ++++-
lib/librte_pmd_e1000/igb_rxtx.c | 5 +++--
lib/librte_pmd_i40e/i40e_rxtx.c | 8 +++++---
lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 +++++---
lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c | 12 ++++++++----
6 files changed, 30 insertions(+), 27 deletions(-)
--
1.8.4.2
^ permalink raw reply [flat|nested] 12+ messages in thread* [dpdk-dev] [PATCH 1/3] mbuf: Use EXTERNAL_MBUF to indicate external buffer 2014-10-24 8:10 [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT Ouyang Changchun @ 2014-10-24 8:10 ` Ouyang Changchun 2014-10-24 8:10 ` [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag Ouyang Changchun ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Ouyang Changchun @ 2014-10-24 8:10 UTC (permalink / raw) To: dev mbuf uses EXTERNAL_MBUF in ol_flags to indicate it is an external buffer, when freeing such kind of mbuf, just need put mbuf itself back into mempool, doesn't free the attached external buffer, user/caller need take care of detaching and freeing the external buffer. Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> --- lib/librte_mbuf/rte_mbuf.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h index ddadc21..8cee8fa 100644 --- a/lib/librte_mbuf/rte_mbuf.h +++ b/lib/librte_mbuf/rte_mbuf.h @@ -114,6 +114,9 @@ extern "C" { /* Bit 51 - IEEE1588*/ #define PKT_TX_IEEE1588_TMST (1ULL << 51) /**< TX IEEE1588 packet to timestamp. */ +/* Bit 62 - Indicate it is external buffer */ +#define EXTERNAL_MBUF (1ULL << 62) /**< External buffer. */ + /* Use final bit of flags to indicate a control mbuf */ #define CTRL_MBUF_FLAG (1ULL << 63) /**< Mbuf contains control data */ @@ -670,7 +673,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m) * - detach mbuf * - free attached mbuf segment */ - if (unlikely (md != m)) { + if (unlikely((md != m) && !(m->ol_flags & EXTERNAL_MBUF))) { rte_pktmbuf_detach(m); if (rte_mbuf_refcnt_update(md, -1) == 0) __rte_mbuf_raw_free(md); -- 1.8.4.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag 2014-10-24 8:10 [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT Ouyang Changchun 2014-10-24 8:10 ` [dpdk-dev] [PATCH 1/3] mbuf: Use EXTERNAL_MBUF to indicate external buffer Ouyang Changchun @ 2014-10-24 8:10 ` Ouyang Changchun 2014-10-24 10:46 ` Ananyev, Konstantin 2014-10-24 8:10 ` [dpdk-dev] [PATCH 3/3] vhost: Removes dependency on REFCNT for zero copy Ouyang Changchun 2014-10-24 9:47 ` [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT Thomas Monjalon 3 siblings, 1 reply; 12+ messages in thread From: Ouyang Changchun @ 2014-10-24 8:10 UTC (permalink / raw) To: dev Every pmd RX function need keep the EXTERNAL_MBUF flag in mbuf.ol_flags, and can't overwrite it when filling ol_flags from descriptor to mbuf, otherwise, it probably cause to crash when freeing a mbuf and trying to freeing its attached external buffer, say, from guest space. Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> --- lib/librte_pmd_e1000/igb_rxtx.c | 5 +++-- lib/librte_pmd_i40e/i40e_rxtx.c | 8 +++++--- lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 +++++--- lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c | 12 ++++++++---- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/lib/librte_pmd_e1000/igb_rxtx.c b/lib/librte_pmd_e1000/igb_rxtx.c index f09c525..4123310 100644 --- a/lib/librte_pmd_e1000/igb_rxtx.c +++ b/lib/librte_pmd_e1000/igb_rxtx.c @@ -786,7 +786,7 @@ eth_igb_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(hlen_type_rss); pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr); pkt_flags = pkt_flags | rx_desc_error_to_pkt_flags(staterr); - rxm->ol_flags = pkt_flags; + rxm->ol_flags = pkt_flags | (rxm->ol_flags & EXTERNAL_MBUF); /* * Store the mbuf address into the next entry of the array @@ -1020,7 +1020,8 @@ eth_igb_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(hlen_type_rss); pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr); pkt_flags = pkt_flags | rx_desc_error_to_pkt_flags(staterr); - first_seg->ol_flags = pkt_flags; + first_seg->ol_flags = pkt_flags | + (first_seg->ol_flags & EXTERNAL_MBUF); /* Prefetch data of first segment, if configured to do so. */ rte_packet_prefetch((char *)first_seg->buf_addr + diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c index 2b53677..68c3695 100644 --- a/lib/librte_pmd_i40e/i40e_rxtx.c +++ b/lib/librte_pmd_i40e/i40e_rxtx.c @@ -637,7 +637,8 @@ i40e_rx_scan_hw_ring(struct i40e_rx_queue *rxq) pkt_flags = i40e_rxd_status_to_pkt_flags(qword1); pkt_flags |= i40e_rxd_error_to_pkt_flags(qword1); pkt_flags |= i40e_rxd_ptype_to_pkt_flags(qword1); - mb->ol_flags = pkt_flags; + mb->ol_flags = pkt_flags | + (mb->ol_flags & EXTERNAL_MBUF); if (pkt_flags & PKT_RX_RSS_HASH) mb->hash.rss = rte_le_to_cpu_32(\ rxdp->wb.qword0.hi_dword.rss); @@ -873,7 +874,7 @@ i40e_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts) pkt_flags = i40e_rxd_status_to_pkt_flags(qword1); pkt_flags |= i40e_rxd_error_to_pkt_flags(qword1); pkt_flags |= i40e_rxd_ptype_to_pkt_flags(qword1); - rxm->ol_flags = pkt_flags; + rxm->ol_flags = pkt_flags | (rxm->ol_flags & EXTERNAL_MBUF); if (pkt_flags & PKT_RX_RSS_HASH) rxm->hash.rss = rte_le_to_cpu_32(rxd.wb.qword0.hi_dword.rss); @@ -1027,7 +1028,8 @@ i40e_recv_scattered_pkts(void *rx_queue, pkt_flags = i40e_rxd_status_to_pkt_flags(qword1); pkt_flags |= i40e_rxd_error_to_pkt_flags(qword1); pkt_flags |= i40e_rxd_ptype_to_pkt_flags(qword1); - first_seg->ol_flags = pkt_flags; + first_seg->ol_flags = pkt_flags | + (first_seg->ol_flags & EXTERNAL_MBUF); if (pkt_flags & PKT_RX_RSS_HASH) rxm->hash.rss = rte_le_to_cpu_32(rxd.wb.qword0.hi_dword.rss); diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c index 1aefe5c..77e8689 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c @@ -949,7 +949,8 @@ ixgbe_rx_scan_hw_ring(struct igb_rx_queue *rxq) /* reuse status field from scan list */ pkt_flags |= rx_desc_status_to_pkt_flags(s[j]); pkt_flags |= rx_desc_error_to_pkt_flags(s[j]); - mb->ol_flags = pkt_flags; + mb->ol_flags = pkt_flags | + (mb->ol_flags & EXTERNAL_MBUF); if (likely(pkt_flags & PKT_RX_RSS_HASH)) mb->hash.rss = rxdp[j].wb.lower.hi_dword.rss; @@ -1271,7 +1272,7 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(hlen_type_rss); pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr); pkt_flags = pkt_flags | rx_desc_error_to_pkt_flags(staterr); - rxm->ol_flags = pkt_flags; + rxm->ol_flags = pkt_flags | (rxm->ol_flags & EXTERNAL_MBUF); if (likely(pkt_flags & PKT_RX_RSS_HASH)) rxm->hash.rss = rxd.wb.lower.hi_dword.rss; @@ -1515,7 +1516,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, rx_desc_status_to_pkt_flags(staterr)); pkt_flags = (uint16_t)(pkt_flags | rx_desc_error_to_pkt_flags(staterr)); - first_seg->ol_flags = pkt_flags; + first_seg->ol_flags = pkt_flags | + (first_seg->ol_flags & EXTERNAL_MBUF); if (likely(pkt_flags & PKT_RX_RSS_HASH)) first_seg->hash.rss = rxd.wb.lower.hi_dword.rss; diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c index e813e43..af7b1cd 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c @@ -156,10 +156,14 @@ desc_to_olflags_v(__m128i descs[4], struct rte_mbuf **rx_pkts) ptype1 = _mm_or_si128(ptype1, vtag1); vol.dword = _mm_cvtsi128_si64(ptype1) & OLFLAGS_MASK_V; - rx_pkts[0]->ol_flags = vol.e[0]; - rx_pkts[1]->ol_flags = vol.e[1]; - rx_pkts[2]->ol_flags = vol.e[2]; - rx_pkts[3]->ol_flags = vol.e[3]; + rx_pkts[0]->ol_flags = vol.e[0] | + (rx_pkts[0]->ol_flags & EXTERNAL_MBUF); + rx_pkts[1]->ol_flags = vol.e[1] | + (rx_pkts[1]->ol_flags & EXTERNAL_MBUF); + rx_pkts[2]->ol_flags = vol.e[2] | + (rx_pkts[2]->ol_flags & EXTERNAL_MBUF); + rx_pkts[3]->ol_flags = vol.e[3] | + (rx_pkts[3]->ol_flags & EXTERNAL_MBUF); } #else #define desc_to_olflags_v(desc, rx_pkts) do {} while (0) -- 1.8.4.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag 2014-10-24 8:10 ` [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag Ouyang Changchun @ 2014-10-24 10:46 ` Ananyev, Konstantin 2014-10-24 12:34 ` Bruce Richardson 0 siblings, 1 reply; 12+ messages in thread From: Ananyev, Konstantin @ 2014-10-24 10:46 UTC (permalink / raw) To: Ouyang, Changchun, dev Hi Changchun, > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang Changchun > Sent: Friday, October 24, 2014 9:10 AM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag > > Every pmd RX function need keep the EXTERNAL_MBUF flag > in mbuf.ol_flags, and can't overwrite it when filling ol_flags from > descriptor to mbuf, otherwise, it probably cause to crash when freeing a mbuf > and trying to freeing its attached external buffer, say, from guest space. > Don't really like the idea to put: mb->ol_flags = pkt_flags | (mb->ol_flags & EXTERNAL_MBUF); in each and every PMD from now on... >From other side, it is probably not very good that RX functions update whole ol_flags, not only RX related part. Wonder can we reserve low 32bits of ol_flags for RX, and high 32bits for TX and generic stuff. So our ol_flags will look something like that: union { uint64_t ol_raw_flags; struct { uint32_t rx; uint32_t gen_tx; } ol_flags }; And make all PMD RX functions to operate on rx part of the flags only: mb->ol_flags.rx = pkt_flags; ? Konstantin > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> > --- > lib/librte_pmd_e1000/igb_rxtx.c | 5 +++-- > lib/librte_pmd_i40e/i40e_rxtx.c | 8 +++++--- > lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 +++++--- > lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c | 12 ++++++++---- > 4 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/lib/librte_pmd_e1000/igb_rxtx.c b/lib/librte_pmd_e1000/igb_rxtx.c > index f09c525..4123310 100644 > --- a/lib/librte_pmd_e1000/igb_rxtx.c > +++ b/lib/librte_pmd_e1000/igb_rxtx.c > @@ -786,7 +786,7 @@ eth_igb_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(hlen_type_rss); > pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr); > pkt_flags = pkt_flags | rx_desc_error_to_pkt_flags(staterr); > - rxm->ol_flags = pkt_flags; > + rxm->ol_flags = pkt_flags | (rxm->ol_flags & EXTERNAL_MBUF); > > /* > * Store the mbuf address into the next entry of the array > @@ -1020,7 +1020,8 @@ eth_igb_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(hlen_type_rss); > pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr); > pkt_flags = pkt_flags | rx_desc_error_to_pkt_flags(staterr); > - first_seg->ol_flags = pkt_flags; > + first_seg->ol_flags = pkt_flags | > + (first_seg->ol_flags & EXTERNAL_MBUF); > > /* Prefetch data of first segment, if configured to do so. */ > rte_packet_prefetch((char *)first_seg->buf_addr + > diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c > index 2b53677..68c3695 100644 > --- a/lib/librte_pmd_i40e/i40e_rxtx.c > +++ b/lib/librte_pmd_i40e/i40e_rxtx.c > @@ -637,7 +637,8 @@ i40e_rx_scan_hw_ring(struct i40e_rx_queue *rxq) > pkt_flags = i40e_rxd_status_to_pkt_flags(qword1); > pkt_flags |= i40e_rxd_error_to_pkt_flags(qword1); > pkt_flags |= i40e_rxd_ptype_to_pkt_flags(qword1); > - mb->ol_flags = pkt_flags; > + mb->ol_flags = pkt_flags | > + (mb->ol_flags & EXTERNAL_MBUF); > if (pkt_flags & PKT_RX_RSS_HASH) > mb->hash.rss = rte_le_to_cpu_32(\ > rxdp->wb.qword0.hi_dword.rss); > @@ -873,7 +874,7 @@ i40e_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts) > pkt_flags = i40e_rxd_status_to_pkt_flags(qword1); > pkt_flags |= i40e_rxd_error_to_pkt_flags(qword1); > pkt_flags |= i40e_rxd_ptype_to_pkt_flags(qword1); > - rxm->ol_flags = pkt_flags; > + rxm->ol_flags = pkt_flags | (rxm->ol_flags & EXTERNAL_MBUF); > if (pkt_flags & PKT_RX_RSS_HASH) > rxm->hash.rss = > rte_le_to_cpu_32(rxd.wb.qword0.hi_dword.rss); > @@ -1027,7 +1028,8 @@ i40e_recv_scattered_pkts(void *rx_queue, > pkt_flags = i40e_rxd_status_to_pkt_flags(qword1); > pkt_flags |= i40e_rxd_error_to_pkt_flags(qword1); > pkt_flags |= i40e_rxd_ptype_to_pkt_flags(qword1); > - first_seg->ol_flags = pkt_flags; > + first_seg->ol_flags = pkt_flags | > + (first_seg->ol_flags & EXTERNAL_MBUF); > if (pkt_flags & PKT_RX_RSS_HASH) > rxm->hash.rss = > rte_le_to_cpu_32(rxd.wb.qword0.hi_dword.rss); > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > index 1aefe5c..77e8689 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > @@ -949,7 +949,8 @@ ixgbe_rx_scan_hw_ring(struct igb_rx_queue *rxq) > /* reuse status field from scan list */ > pkt_flags |= rx_desc_status_to_pkt_flags(s[j]); > pkt_flags |= rx_desc_error_to_pkt_flags(s[j]); > - mb->ol_flags = pkt_flags; > + mb->ol_flags = pkt_flags | > + (mb->ol_flags & EXTERNAL_MBUF); > > if (likely(pkt_flags & PKT_RX_RSS_HASH)) > mb->hash.rss = rxdp[j].wb.lower.hi_dword.rss; > @@ -1271,7 +1272,7 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(hlen_type_rss); > pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr); > pkt_flags = pkt_flags | rx_desc_error_to_pkt_flags(staterr); > - rxm->ol_flags = pkt_flags; > + rxm->ol_flags = pkt_flags | (rxm->ol_flags & EXTERNAL_MBUF); > > if (likely(pkt_flags & PKT_RX_RSS_HASH)) > rxm->hash.rss = rxd.wb.lower.hi_dword.rss; > @@ -1515,7 +1516,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > rx_desc_status_to_pkt_flags(staterr)); > pkt_flags = (uint16_t)(pkt_flags | > rx_desc_error_to_pkt_flags(staterr)); > - first_seg->ol_flags = pkt_flags; > + first_seg->ol_flags = pkt_flags | > + (first_seg->ol_flags & EXTERNAL_MBUF); > > if (likely(pkt_flags & PKT_RX_RSS_HASH)) > first_seg->hash.rss = rxd.wb.lower.hi_dword.rss; > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > index e813e43..af7b1cd 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > @@ -156,10 +156,14 @@ desc_to_olflags_v(__m128i descs[4], struct rte_mbuf **rx_pkts) > ptype1 = _mm_or_si128(ptype1, vtag1); > vol.dword = _mm_cvtsi128_si64(ptype1) & OLFLAGS_MASK_V; > > - rx_pkts[0]->ol_flags = vol.e[0]; > - rx_pkts[1]->ol_flags = vol.e[1]; > - rx_pkts[2]->ol_flags = vol.e[2]; > - rx_pkts[3]->ol_flags = vol.e[3]; > + rx_pkts[0]->ol_flags = vol.e[0] | > + (rx_pkts[0]->ol_flags & EXTERNAL_MBUF); > + rx_pkts[1]->ol_flags = vol.e[1] | > + (rx_pkts[1]->ol_flags & EXTERNAL_MBUF); > + rx_pkts[2]->ol_flags = vol.e[2] | > + (rx_pkts[2]->ol_flags & EXTERNAL_MBUF); > + rx_pkts[3]->ol_flags = vol.e[3] | > + (rx_pkts[3]->ol_flags & EXTERNAL_MBUF); > } > #else > #define desc_to_olflags_v(desc, rx_pkts) do {} while (0) > -- > 1.8.4.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag 2014-10-24 10:46 ` Ananyev, Konstantin @ 2014-10-24 12:34 ` Bruce Richardson 2014-10-24 15:43 ` Bruce Richardson 0 siblings, 1 reply; 12+ messages in thread From: Bruce Richardson @ 2014-10-24 12:34 UTC (permalink / raw) To: Ananyev, Konstantin; +Cc: dev On Fri, Oct 24, 2014 at 10:46:06AM +0000, Ananyev, Konstantin wrote: > Hi Changchun, > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang Changchun > > Sent: Friday, October 24, 2014 9:10 AM > > To: dev@dpdk.org > > Subject: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag > > > > Every pmd RX function need keep the EXTERNAL_MBUF flag > > in mbuf.ol_flags, and can't overwrite it when filling ol_flags from > > descriptor to mbuf, otherwise, it probably cause to crash when freeing a mbuf > > and trying to freeing its attached external buffer, say, from guest space. > > > > Don't really like the idea to put: > mb->ol_flags = pkt_flags | (mb->ol_flags & EXTERNAL_MBUF); > in each and every PMD from now on... > > From other side, it is probably not very good that RX functions update whole ol_flags, not only RX related part. > Wonder can we reserve low 32bits of ol_flags for RX, and high 32bits for TX and generic stuff. > So our ol_flags will look something like that: > > union { > uint64_t ol_raw_flags; > struct { > uint32_t rx; > uint32_t gen_tx; > } ol_flags > }; > > And make all PMD RX functions to operate on rx part of the flags only: > mb->ol_flags.rx = pkt_flags; > ? > > Konstantin > I would tend to agree with this. Changchun, did you get to assess the performance impact of making this change to the PMDs? I suspect that making the changes to each PMD would impact performance, while Konstantin's suggestion should eliminate that impact. The downside there is that we are limiting the flexibility we have in expanding beyond 32 RX flags and 24 TX flags. :-( /Bruce ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag 2014-10-24 12:34 ` Bruce Richardson @ 2014-10-24 15:43 ` Bruce Richardson 2014-10-24 15:58 ` Ananyev, Konstantin 0 siblings, 1 reply; 12+ messages in thread From: Bruce Richardson @ 2014-10-24 15:43 UTC (permalink / raw) To: Ananyev, Konstantin; +Cc: dev On Fri, Oct 24, 2014 at 01:34:58PM +0100, Bruce Richardson wrote: > On Fri, Oct 24, 2014 at 10:46:06AM +0000, Ananyev, Konstantin wrote: > > Hi Changchun, > > > > > -----Original Message----- > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang Changchun > > > Sent: Friday, October 24, 2014 9:10 AM > > > To: dev@dpdk.org > > > Subject: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag > > > > > > Every pmd RX function need keep the EXTERNAL_MBUF flag > > > in mbuf.ol_flags, and can't overwrite it when filling ol_flags from > > > descriptor to mbuf, otherwise, it probably cause to crash when freeing a mbuf > > > and trying to freeing its attached external buffer, say, from guest space. > > > > > > > Don't really like the idea to put: > > mb->ol_flags = pkt_flags | (mb->ol_flags & EXTERNAL_MBUF); > > in each and every PMD from now on... > > > > From other side, it is probably not very good that RX functions update whole ol_flags, not only RX related part. > > Wonder can we reserve low 32bits of ol_flags for RX, and high 32bits for TX and generic stuff. > > So our ol_flags will look something like that: > > > > union { > > uint64_t ol_raw_flags; > > struct { > > uint32_t rx; > > uint32_t gen_tx; > > } ol_flags > > }; > > > > And make all PMD RX functions to operate on rx part of the flags only: > > mb->ol_flags.rx = pkt_flags; > > ? > > > > Konstantin > > > I would tend to agree with this. Changchun, did you get to assess the > performance impact of making this change to the PMDs? I suspect that making > the changes to each PMD would impact performance, while Konstantin's > suggestion should eliminate that impact. > The downside there is that we are limiting the flexibility we have in > expanding beyond 32 RX flags and 24 TX flags. :-( > > /Bruce > How about switching things about in terms of the flag. Instead of having to manage a flag across the baord to indicate if an mbuf is pointing to external memory, I think we should use the flag to indicate that an mbuf is attached to the memory space of another mbuf. My reasons for suggesting this are: 1. Mbufs pointing to externally managed memory are not really the problem to be dealt with on free, since they can be handled the same as mbufs with the data pointer pointing internally, it's mbufs attached to other mbufs which are - so that's what we need to track using a flag. 2. Setting the flag to indicate an indirect mbuf should have no impact on the driver, as an mbuf that has just been allocated from mempool cannot be an indirect one. 3. The only place we would need to worry about such a flag is in the attach, detach and free mbuf functions - and on free we would simply need to replace the existing check for "md != m" with a new check for the new flag. It would be a contained change. Thoughts? /Bruce ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag 2014-10-24 15:43 ` Bruce Richardson @ 2014-10-24 15:58 ` Ananyev, Konstantin 2014-10-25 2:08 ` Ouyang, Changchun 0 siblings, 1 reply; 12+ messages in thread From: Ananyev, Konstantin @ 2014-10-24 15:58 UTC (permalink / raw) To: Richardson, Bruce; +Cc: dev > -----Original Message----- > From: Richardson, Bruce > Sent: Friday, October 24, 2014 4:43 PM > To: Ananyev, Konstantin > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag > > On Fri, Oct 24, 2014 at 01:34:58PM +0100, Bruce Richardson wrote: > > On Fri, Oct 24, 2014 at 10:46:06AM +0000, Ananyev, Konstantin wrote: > > > Hi Changchun, > > > > > > > -----Original Message----- > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang Changchun > > > > Sent: Friday, October 24, 2014 9:10 AM > > > > To: dev@dpdk.org > > > > Subject: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag > > > > > > > > Every pmd RX function need keep the EXTERNAL_MBUF flag > > > > in mbuf.ol_flags, and can't overwrite it when filling ol_flags from > > > > descriptor to mbuf, otherwise, it probably cause to crash when freeing a mbuf > > > > and trying to freeing its attached external buffer, say, from guest space. > > > > > > > > > > Don't really like the idea to put: > > > mb->ol_flags = pkt_flags | (mb->ol_flags & EXTERNAL_MBUF); > > > in each and every PMD from now on... > > > > > > From other side, it is probably not very good that RX functions update whole ol_flags, not only RX related part. > > > Wonder can we reserve low 32bits of ol_flags for RX, and high 32bits for TX and generic stuff. > > > So our ol_flags will look something like that: > > > > > > union { > > > uint64_t ol_raw_flags; > > > struct { > > > uint32_t rx; > > > uint32_t gen_tx; > > > } ol_flags > > > }; > > > > > > And make all PMD RX functions to operate on rx part of the flags only: > > > mb->ol_flags.rx = pkt_flags; > > > ? > > > > > > Konstantin > > > > > I would tend to agree with this. Changchun, did you get to assess the > > performance impact of making this change to the PMDs? I suspect that making > > the changes to each PMD would impact performance, while Konstantin's > > suggestion should eliminate that impact. > > The downside there is that we are limiting the flexibility we have in > > expanding beyond 32 RX flags and 24 TX flags. :-( > > > > /Bruce > > > > How about switching things about in terms of the flag. Instead of having to > manage a flag across the baord to indicate if an mbuf is pointing to > external memory, I think we should use the flag to indicate that an mbuf is > attached to the memory space of another mbuf. > > My reasons for suggesting this are: > 1. Mbufs pointing to externally managed memory are not really the problem to > be dealt with on free, since they can be handled the same as mbufs with the > data pointer pointing internally, it's mbufs attached to other mbufs which > are - so that's what we need to track using a flag. > 2. Setting the flag to indicate an indirect mbuf should have no impact on > the driver, as an mbuf that has just been allocated from mempool cannot be > an indirect one. > 3. The only place we would need to worry about such a flag is in the attach, > detach and free mbuf functions - and on free we would simply need to replace > the existing check for "md != m" with a new check for the new flag. It would > be a contained change. > Sounds good to me. That's' definitely much better than my proposal. Plus, if we'll stop to rely on: md = RTE_MBUF_FROM_BADDR(m->buf_addr); if (unlikely (md != m)) { That will allow us to set buf_addr to some other valid offset inside mbuf and that fix an old problem with mbufs extra metadata (userdata) stored in the packet's headroom. Konstantin > Thoughts? > /Bruce ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag 2014-10-24 15:58 ` Ananyev, Konstantin @ 2014-10-25 2:08 ` Ouyang, Changchun 0 siblings, 0 replies; 12+ messages in thread From: Ouyang, Changchun @ 2014-10-25 2:08 UTC (permalink / raw) To: Ananyev, Konstantin, Richardson, Bruce; +Cc: dev > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, > Konstantin > Sent: Friday, October 24, 2014 11:58 PM > To: Richardson, Bruce > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep > EXTERNAL_MBUF flag > > > > > -----Original Message----- > > From: Richardson, Bruce > > Sent: Friday, October 24, 2014 4:43 PM > > To: Ananyev, Konstantin > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep > > EXTERNAL_MBUF flag > > > > On Fri, Oct 24, 2014 at 01:34:58PM +0100, Bruce Richardson wrote: > > > On Fri, Oct 24, 2014 at 10:46:06AM +0000, Ananyev, Konstantin wrote: > > > > Hi Changchun, > > > > > > > > > -----Original Message----- > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang > > > > > Changchun > > > > > Sent: Friday, October 24, 2014 9:10 AM > > > > > To: dev@dpdk.org > > > > > Subject: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep > > > > > EXTERNAL_MBUF flag > > > > > > > > > > Every pmd RX function need keep the EXTERNAL_MBUF flag in > > > > > mbuf.ol_flags, and can't overwrite it when filling ol_flags from > > > > > descriptor to mbuf, otherwise, it probably cause to crash when > > > > > freeing a mbuf and trying to freeing its attached external buffer, say, > from guest space. > > > > > > > > > > > > > Don't really like the idea to put: > > > > mb->ol_flags = pkt_flags | (mb->ol_flags & EXTERNAL_MBUF); > > > > in each and every PMD from now on... > > > > > > > > From other side, it is probably not very good that RX functions update > whole ol_flags, not only RX related part. > > > > Wonder can we reserve low 32bits of ol_flags for RX, and high 32bits for > TX and generic stuff. > > > > So our ol_flags will look something like that: > > > > > > > > union { > > > > uint64_t ol_raw_flags; > > > > struct { > > > > uint32_t rx; > > > > uint32_t gen_tx; > > > > } ol_flags > > > > }; > > > > > > > > And make all PMD RX functions to operate on rx part of the flags only: > > > > mb->ol_flags.rx = pkt_flags; > > > > ? > > > > > > > > Konstantin > > > > > > > I would tend to agree with this. Changchun, did you get to assess > > > the performance impact of making this change to the PMDs? I suspect > > > that making the changes to each PMD would impact performance, while > > > Konstantin's suggestion should eliminate that impact. > > > The downside there is that we are limiting the flexibility we have > > > in expanding beyond 32 RX flags and 24 TX flags. :-( > > > > > > /Bruce > > > > > > > How about switching things about in terms of the flag. Instead of > > having to manage a flag across the baord to indicate if an mbuf is > > pointing to external memory, I think we should use the flag to > > indicate that an mbuf is attached to the memory space of another mbuf. > > > > My reasons for suggesting this are: > > 1. Mbufs pointing to externally managed memory are not really the > > problem to be dealt with on free, since they can be handled the same > > as mbufs with the data pointer pointing internally, it's mbufs > > attached to other mbufs which are - so that's what we need to track using a > flag. > > 2. Setting the flag to indicate an indirect mbuf should have no impact > > on the driver, as an mbuf that has just been allocated from mempool > > cannot be an indirect one. > > 3. The only place we would need to worry about such a flag is in the > > attach, detach and free mbuf functions - and on free we would simply > > need to replace the existing check for "md != m" with a new check for > > the new flag. It would be a contained change. > > > > Sounds good to me. > That's' definitely much better than my proposal. > Plus, if we'll stop to rely on: > > md = RTE_MBUF_FROM_BADDR(m->buf_addr); > if (unlikely (md != m)) { > Currently seems good to me, too. But need more practice on it. > That will allow us to set buf_addr to some other valid offset inside mbuf and > that fix an old problem with mbufs extra metadata (userdata) stored in the > packet's headroom. > Not fully understand this. Konstantin, would you explain more? Thanks Changchun ^ permalink raw reply [flat|nested] 12+ messages in thread
* [dpdk-dev] [PATCH 3/3] vhost: Removes dependency on REFCNT for zero copy 2014-10-24 8:10 [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT Ouyang Changchun 2014-10-24 8:10 ` [dpdk-dev] [PATCH 1/3] mbuf: Use EXTERNAL_MBUF to indicate external buffer Ouyang Changchun 2014-10-24 8:10 ` [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag Ouyang Changchun @ 2014-10-24 8:10 ` Ouyang Changchun 2014-10-24 9:47 ` [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT Thomas Monjalon 3 siblings, 0 replies; 12+ messages in thread From: Ouyang Changchun @ 2014-10-24 8:10 UTC (permalink / raw) To: dev Vhost zero copy removes the dependency on macro REFCNT by using EXTERNAL_MBUF flag in mbuf.ol_flags to indicate it is an external buffer from guest. Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> --- examples/vhost/main.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/examples/vhost/main.c b/examples/vhost/main.c index fa0ad0c..e3b1884 100644 --- a/examples/vhost/main.c +++ b/examples/vhost/main.c @@ -713,19 +713,6 @@ us_vhost_parse_args(int argc, char **argv) return -1; } else zero_copy = ret; - - if (zero_copy) { -#ifdef RTE_MBUF_REFCNT - RTE_LOG(ERR, VHOST_CONFIG, "Before running " - "zero copy vhost APP, please " - "disable RTE_MBUF_REFCNT\n" - "in config file and then rebuild DPDK " - "core lib!\n" - "Otherwise please disable zero copy " - "flag in command line!\n"); - return -1; -#endif - } } /* Specify the descriptor number on RX. */ @@ -1453,6 +1440,7 @@ attach_rxmbuf_zcp(struct virtio_net *dev) mbuf->buf_physaddr = phys_addr - RTE_PKTMBUF_HEADROOM; mbuf->data_len = desc->len; MBUF_HEADROOM_UINT32(mbuf) = (uint32_t)desc_idx; + mbuf->ol_flags |= EXTERNAL_MBUF; LOG_DEBUG(VHOST_DATA, "(%"PRIu64") in attach_rxmbuf_zcp: res base idx:%d, " @@ -1489,6 +1477,8 @@ static inline void pktmbuf_detach_zcp(struct rte_mbuf *m) m->data_off = buf_ofs; m->data_len = 0; + + m->ol_flags &= ~EXTERNAL_MBUF; } /* @@ -1805,8 +1795,9 @@ virtio_tx_route_zcp(struct virtio_net *dev, struct rte_mbuf *m, mbuf->data_off = m->data_off; mbuf->buf_physaddr = m->buf_physaddr; mbuf->buf_addr = m->buf_addr; + mbuf->ol_flags |= EXTERNAL_MBUF; } - mbuf->ol_flags = PKT_TX_VLAN_PKT; + mbuf->ol_flags |= PKT_TX_VLAN_PKT; mbuf->vlan_tci = vlan_tag; mbuf->l2_len = sizeof(struct ether_hdr); mbuf->l3_len = sizeof(struct ipv4_hdr); -- 1.8.4.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT 2014-10-24 8:10 [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT Ouyang Changchun ` (2 preceding siblings ...) 2014-10-24 8:10 ` [dpdk-dev] [PATCH 3/3] vhost: Removes dependency on REFCNT for zero copy Ouyang Changchun @ 2014-10-24 9:47 ` Thomas Monjalon 2014-10-24 10:47 ` Bruce Richardson 2014-10-25 1:01 ` Ouyang, Changchun 3 siblings, 2 replies; 12+ messages in thread From: Thomas Monjalon @ 2014-10-24 9:47 UTC (permalink / raw) To: Ouyang Changchun; +Cc: dev 2014-10-24 16:10, Ouyang Changchun: > To remove the dependency of RTE_MBUF_REFCNT for vhost zero copy, > the mbuf need introduce EXTERNAL_MBUF(in ol_flags) to indicate it > attaches to an external buffer, say, from guest space. And don't > free the external buffer when freeing the mbuf itself in host, in > addition, RX function in PMD need make sure not overwrite this flag > when filling ol_flags from descriptors to mbuf. So you are replacing refcnt by something else which requires special handling in drivers. I feel this is not the right design. Why do you want to remove refcnt dependency? -- Thomas ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT 2014-10-24 9:47 ` [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT Thomas Monjalon @ 2014-10-24 10:47 ` Bruce Richardson 2014-10-25 1:01 ` Ouyang, Changchun 1 sibling, 0 replies; 12+ messages in thread From: Bruce Richardson @ 2014-10-24 10:47 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev On Fri, Oct 24, 2014 at 11:47:46AM +0200, Thomas Monjalon wrote: > 2014-10-24 16:10, Ouyang Changchun: > > To remove the dependency of RTE_MBUF_REFCNT for vhost zero copy, > > the mbuf need introduce EXTERNAL_MBUF(in ol_flags) to indicate it > > attaches to an external buffer, say, from guest space. And don't > > free the external buffer when freeing the mbuf itself in host, in > > addition, RX function in PMD need make sure not overwrite this flag > > when filling ol_flags from descriptors to mbuf. > > So you are replacing refcnt by something else which requires special > handling in drivers. > I feel this is not the right design. > Why do you want to remove refcnt dependency? > Ignoring the implementation of the patchset for now - as I haven't reviewed it in depth yet, I think the removal of the dependency on REFCNT in this vhost code is a good thing. This is the only place in DPDK which depends on the REFCNT being *disabled*. We have lots of things which rely on using having a reference count enabled in the mbuf, and lots and lots of #ifdefs in the code to work around the possibility of it being disabled. If we can remove the need for the reference count to be disabled here we can look to do some major cleanup, by removing completely the option to disable the reference counting. Regards, /Bruce ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT 2014-10-24 9:47 ` [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT Thomas Monjalon 2014-10-24 10:47 ` Bruce Richardson @ 2014-10-25 1:01 ` Ouyang, Changchun 1 sibling, 0 replies; 12+ messages in thread From: Ouyang, Changchun @ 2014-10-25 1:01 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev Hi Thomas > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Friday, October 24, 2014 5:48 PM > To: Ouyang, Changchun > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of > REFCNT > > 2014-10-24 16:10, Ouyang Changchun: > > To remove the dependency of RTE_MBUF_REFCNT for vhost zero copy, > the > > mbuf need introduce EXTERNAL_MBUF(in ol_flags) to indicate it attaches > > to an external buffer, say, from guest space. And don't free the > > external buffer when freeing the mbuf itself in host, in addition, RX > > function in PMD need make sure not overwrite this flag when filling > > ol_flags from descriptors to mbuf. > > So you are replacing refcnt by something else which requires special handling > in drivers. To some extent, You are right, but I'd like regard as: our current mbuf implement has no consideration of external buffer. While vhost zero copy need it work well when it is external buffer, so need this special handling. Very appreciate if you have a better solution, design idea and let me know. > I feel this is not the right design. > Why do you want to remove refcnt dependency? > Bruce has some explanation about the reason in another mail. For vhost zero copy itself, it doesn't depend on most of code REFCNT, it just need mbuf can support external buffer. Before the change, undefine the REFCNT, make mbuf has workaround for supporting external buffer. Thanks and regards, Changchun ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-10-25 2:00 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-10-24 8:10 [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT Ouyang Changchun 2014-10-24 8:10 ` [dpdk-dev] [PATCH 1/3] mbuf: Use EXTERNAL_MBUF to indicate external buffer Ouyang Changchun 2014-10-24 8:10 ` [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag Ouyang Changchun 2014-10-24 10:46 ` Ananyev, Konstantin 2014-10-24 12:34 ` Bruce Richardson 2014-10-24 15:43 ` Bruce Richardson 2014-10-24 15:58 ` Ananyev, Konstantin 2014-10-25 2:08 ` Ouyang, Changchun 2014-10-24 8:10 ` [dpdk-dev] [PATCH 3/3] vhost: Removes dependency on REFCNT for zero copy Ouyang Changchun 2014-10-24 9:47 ` [dpdk-dev] [PATCH 0/3] Vhost app removes dependency of REFCNT Thomas Monjalon 2014-10-24 10:47 ` Bruce Richardson 2014-10-25 1:01 ` Ouyang, Changchun
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).