From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 3CAC77E1B for ; Fri, 5 Dec 2014 11:39:15 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 05 Dec 2014 02:39:00 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,521,1413270000"; d="scan'208";a="642880030" Received: from bricha3-mobl3.ger.corp.intel.com ([10.243.20.39]) by fmsmga002.fm.intel.com with SMTP; 05 Dec 2014 02:38:58 -0800 Received: by (sSMTP sendmail emulation); Fri, 05 Dec 2014 10:38:58 +0025 Date: Fri, 5 Dec 2014 10:38:58 +0000 From: Bruce Richardson To: "Ananyev, Konstantin" Message-ID: <20141205103857.GA8184@bricha3-MOBL3> References: <1417716553-1506-1-git-send-email-jean-mickael.guerin@6wind.com> <2601191342CEEE43887BDE71AB977258213BCCB6@IRSMSX105.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB977258213BCCB6@IRSMSX105.ger.corp.intel.com> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2] ixgbe: don't override mbuf buffer length X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Dec 2014 10:39:16 -0000 On Fri, Dec 05, 2014 at 01:10:28AM +0000, Ananyev, Konstantin wrote: > > > > -----Original Message----- > > From: Jean-Mickael Guerin [mailto:jean-mickael.guerin@6wind.com] > > Sent: Thursday, December 04, 2014 6:09 PM > > To: dev@dpdk.org > > Cc: Richardson, Bruce; Ananyev, Konstantin > > Subject: [PATCH v2] ixgbe: don't override mbuf buffer length > > > > The template mbuf_initializer is hard coded with a buflen which > > might have been set differently by the application at the time of > > mbuf pool creation. > > > > Switch to a mbuf allocation, to fetch the correct default values. > > There is no performance impact because this is not a data-plane API. > > > > Signed-off-by: Jean-Mickael Guerin > > Acked-by: David Marchand > > Fixes: 0ff3324da2 ("ixgbe: rework vector pmd following mbuf changes") > > --- > > > > v2: check returned value of ixgbe_rxq_vec_setup > > > > lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 5 ++++- > > lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c | 19 ++++++++++++------- > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > > index 5c36bff..7994da1 100644 > > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > > @@ -2244,7 +2244,10 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev, > > use_def_burst_func = check_rx_burst_bulk_alloc_preconditions(rxq); > > > > #ifdef RTE_IXGBE_INC_VECTOR > > - ixgbe_rxq_vec_setup(rxq); > > + if (ixgbe_rxq_vec_setup(rxq) < 0) { > > + ixgbe_rx_queue_release(rxq); > > + return (-ENOMEM); > > + } > > #endif > > /* Check if pre-conditions are satisfied, and no Scattered Rx */ > > if (!use_def_burst_func && !dev->data->scattered_rx) { > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > > index c1b5a78..f7b02f5 100644 > > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > > @@ -732,17 +732,22 @@ static struct ixgbe_txq_ops vec_txq_ops = { > > int > > ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq) > > { > > - struct rte_mbuf mb_def = { .buf_addr = 0 }; /* zeroed mbuf */ > > + struct rte_mbuf *mb_def; > > > > - mb_def.nb_segs = 1; > > - mb_def.data_off = RTE_PKTMBUF_HEADROOM; > > - mb_def.buf_len = rxq->mb_pool->elt_size - sizeof(struct rte_mbuf); > > - mb_def.port = rxq->port_id; > > - rte_mbuf_refcnt_set(&mb_def, 1); > > + mb_def = rte_pktmbuf_alloc(rxq->mb_pool); > > + if (mb_def == NULL) { > > + PMD_INIT_LOG(ERR, "ixgbe_rxq_vec_setup: could not allocate one mbuf"); > > + return -1; > > + } > > + /* nb_segs, refcnt, data_off and buf_len are already set */ > > + mb_def->port = rxq->port_id; > > > > /* prevent compiler reordering: rearm_data covers previous fields */ > > rte_compiler_barrier(); > > - rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data); > > + rxq->mbuf_initializer = *((uint64_t *)&mb_def->rearm_data); > > + > > + rte_pktmbuf_free(mb_def); > > + > > return 0; > > } > > > > -- > > 2.1.3 > > As I said in another mail, I don't think it is a proper fix. > What we did here - just changed one assumption to another. > Current assumption - all mbuf obj_init() would setup buf_len in exactly the same manner as rte_pktmbuf_init() does: > buf_len = mp->elt_size - sizeof(struct rte_mbuf); > New assumption - all mbuf obj_init() would setup buf_len for all mbufs in the pool to the same value. > Both assumptions, I believe, are not always correct. > Though, probably the new one would be true more often. > > I still think the proper fix is not to update mbuf's buf_len field at ixgbe_rxq_rearm() at all. > We should just leave the original value unmodified. > Actually, while looking at ixgbe_rxq_rearm(), I don't see any reason why we need to update buf_len field. > It is not the data that need to be rearmed. > The fields that need to be rearmed are: > uint16_t data_off; > uint16_t refcnt > uint8_t nb_segs; > uint8_t port; > > 6B in total. > We probably would like to keep rearming as one 64bit load/store. > Though straight below them we have: > uint64_t ol_flags; > > As RX fully override ol_flags anyway, we can safely overwrite first 2B of it. > That would allow us to still read/write whole 64bits and avoid overwriting buf_len. > I am talking about something like patch below. > I admit that it looks not so pretty, but I think it is much safer and correct. > Konstantin I just don't see this as worthwhile doing. We are looking here at an mbuf pool which is to be used for packet buffers for RX. If the packet buffers in that pool are all of different sizes then we need to go back and look at other places throughout the code too. For instance, we query the buffer length of the mbuf pool when initializing the RX queues to determine if we need to enable scattered RX. If the mbufs in the pool can potentially be of different sizes, we need to turn off the no-scattered-packets optimization and always use the scatter packets code path - because the assumption that buffers don't get resized could also be false. Similarly here, if the mbufs are going to be of different sizes then the user should disable the vector PMD, and use RX code that doesn't override the buf_len each time. Furthermore, we still don't have an actual use-case where the user would want to have different size mbufs in an mbuf pool used for RX. They can still have variable-sized mbufs in other pools, but having all buffers the same size in the pool used to receive packets seems a perfectly fair restriction to have. If someone has an app that they are creating that needs this functionality, I'll reconsider this opinion, but right now this is all theoretical. Regards, /Bruce > > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c > @@ -79,13 +79,19 @@ ixgbe_rxq_rearm(struct igb_rx_queue *rxq) > /* Initialize the mbufs in vector, process 2 mbufs in one loop */ > for (i = 0; i < RTE_IXGBE_RXQ_REARM_THRESH; i += 2, rxep += 2) { > __m128i vaddr0, vaddr1; > + uintptr_t p0, p1; > > mb0 = rxep[0].mbuf; > mb1 = rxep[1].mbuf; > > /* flush mbuf with pkt template */ > - mb0->rearm_data[0] = rxq->mbuf_initializer; > - mb1->rearm_data[0] = rxq->mbuf_initializer; > + p0 = (uintptr_t)&mb0->data_off; > + *(uint64_t *)p0 = rxq->mbuf_initializer; > + p1 = (uintptr_t)&mb1->data_off; > + *(uint64_t *)p1 = rxq->mbuf_initializer; > + > + //mb0->rearm_data[0] = rxq->mbuf_initializer; > + //mb1->rearm_data[0] = rxq->mbuf_initializer; > > /* load buf_addr(lo 64bit) and buf_physaddr(hi 64bit) */ > vaddr0 = _mm_loadu_si128((__m128i *)&(mb0->buf_addr)); > @@ -732,6 +738,7 @@ static struct ixgbe_txq_ops vec_txq_ops = { > int > ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq) > { > + uintptr_t p; > struct rte_mbuf mb_def = { .buf_addr = 0 }; /* zeroed mbuf */ > > mb_def.nb_segs = 1; > @@ -739,7 +746,8 @@ ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq) > mb_def.buf_len = rxq->mb_pool->elt_size - sizeof(struct rte_mbuf); > mb_def.port = rxq->port_id; > rte_mbuf_refcnt_set(&mb_def, 1); > - rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data); > + p = (uintptr_t)&mb_def.data_off; > + rxq->mbuf_initializer = *(uint64_t *)p; > return 0; > } > >