From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id D6F612E81 for ; Fri, 30 Jan 2015 04:38:28 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 29 Jan 2015 19:32:11 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,490,1418112000"; d="scan'208";a="644856244" Received: from bricha3-mobl3.ger.corp.intel.com ([10.254.76.163]) by orsmga001.jf.intel.com with SMTP; 29 Jan 2015 19:38:26 -0800 Received: by (sSMTP sendmail emulation); Thu, 29 Jan 2015 20:38:24 -0600 Date: Thu, 29 Jan 2015 20:38:24 -0700 From: Bruce Richardson To: "Liang, Cunming" Message-ID: <20150130033823.GA2240@bricha3-MOBL3> References: <5418900.qsIIdEyxOF@xps13> <20141201171854.GH4856@bricha3-MOBL3> <1953153.vMxMoL6Atb@xps13> <9179246.fLSLRuqZit@xps13> <20150129232748.GA11276@bricha3-MOBL3> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc 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, 30 Jan 2015 03:38:29 -0000 On Thu, Jan 29, 2015 at 11:39:37PM +0000, Liang, Cunming wrote: > > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson > > Sent: Thursday, January 29, 2015 4:28 PM > > To: Thomas Monjalon > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc > > > > On Thu, Jan 29, 2015 at 11:18:01PM +0100, Thomas Monjalon wrote: > > > 2014-12-01 18:22, Thomas Monjalon: > > > > 2014-12-01 17:18, Bruce Richardson: > > > > > On Mon, Dec 01, 2014 at 06:10:18PM +0100, Thomas Monjalon wrote: > > > > > > These 2 configuration options are incompatible: > > > > > > CONFIG_RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC=n > > > > > > CONFIG_RTE_IXGBE_INC_VECTOR=y > > > > > > Building this config gives this error: > > > > > > lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c:69:24: > > > > > > error: ‘struct igb_rx_queue’ has no member named ‘fake_mbuf’ > > > > > > > > > > > > I'd like a confirmation that it will be always incompatible. > > > > > > Thanks > > > > > > > > > > Hi Thomas, > > > > > > > > > > I don't think these options should always be incompatible, though as you > > point > > > > > out you do need to turn on bulk alloc support in order to use the vector > > PMD. > > > > > Why do you ask? There are no immediate plans to remove the dependency > > on our end. > > > > > > So you confirm that the ixgbe vpmd really needs Rx bulk alloc and this kind of > > > patch cannot work at all (I don't know the design of vpmd): > > > > > > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > > > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > > > @@ -2119,12 +2119,12 @@ ixgbe_reset_rx_queue(struct igb_rx_queue *rxq) > > > rxq->rx_ring[i] = zeroed_desc; > > > } > > > > > > -#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC > > > /* > > > * initialize extra software ring entries. Space for these extra > > > * entries is always allocated > > > */ > > > memset(&rxq->fake_mbuf, 0x0, sizeof(rxq->fake_mbuf)); > > > +#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC > > > for (i = 0; i < RTE_PMD_IXGBE_RX_MAX_BURST; ++i) { > > > rxq->sw_ring[rxq->nb_rx_desc + i].mbuf = > > &rxq->fake_mbuf; > > > } > > > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.h > > > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.h > > > @@ -127,9 +127,9 @@ struct igb_rx_queue { > > > uint8_t crc_len; /**< 0 if CRC stripped, 4 otherwise. > > */ > > > uint8_t drop_en; /**< If not 0, set SRRCTL.Drop_En. > > */ > > > uint8_t rx_deferred_start; /**< not in global dev start. > > */ > > > -#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC > > > /** need to alloc dummy mbuf, for wraparound when scanning hw > > ring */ > > > struct rte_mbuf fake_mbuf; > > > +#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC > > > /** hold packets to return to application */ > > > struct rte_mbuf *rx_stage[RTE_PMD_IXGBE_RX_MAX_BURST*2]; > > > #endif > > > > > > > I think the compilation shouldn't fail without a proper message. > > > > In order to distinguish a real compilation error from an incompatibility, > > > > we should add a warning in the makefile. > > > > Ideally, the build system should handle dependencies. But waiting this ideal > > > > time, a warning would be graceful. > > > > > > Do you agree that something like this would be OK? > > > > > > --- a/lib/librte_pmd_ixgbe/Makefile > > > +++ b/lib/librte_pmd_ixgbe/Makefile > > > @@ -114,4 +114,8 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += > > lib/librte_eal lib/librte_ether > > > DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += lib/librte_mempool > > lib/librte_mbuf > > > DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += lib/librte_net > > lib/librte_malloc > > > > > > +ifeq > > ($(CONFIG_RTE_IXGBE_INC_VECTOR)$(CONFIG_RTE_LIBRTE_IXGBE_RX_ALLOW_B > > ULK_ALLOC),yn) > > > +$(error The ixgbe vpmd depends on Rx bulk alloc) > > > +endif > > > + > > > include $(RTE_SDK)/mk/rte.lib.mk > > > > > > > Something like the above looks like a good solution to me. > > > > /Bruce > [Liang, Cunming] To avoid compile complain, this one is ok. > It's doable to remove the dependence between two. > We can submit it in a separate patch. > > Sure, if that can be done, it sounds good. I don't see a huge problem with having a dependency between the two - I can't really see a use case for someone wanting the vector driver but to have the bulk-alloc scalar one disabled. So, I'm easy either way, with just flagging the warning or removing the dependency completely. Follow-on question - can we look to remove the bulk alloc switch completely. The user can force the selection of the RX function and TX functions at run time via the nic setup parameters, so I don't see the need to limit the choices at compile time - other than the vpmd which obviously has an instruction set dependency. /Bruce