From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [BUG] ixgbe vector cannot compile without bulk alloc Date: Thu, 29 Jan 2015 20:38:24 -0700 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-Transfer-Encoding: quoted-printable Cc: "dev-VfR2kkLFssw@public.gmane.org" To: "Liang, Cunming" Return-path: Content-Disposition: inline In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Thu, Jan 29, 2015 at 11:39:37PM +0000, Liang, Cunming wrote: >=20 >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Bruce Richardson > > Sent: Thursday, January 29, 2015 4:28 PM > > To: Thomas Monjalon > > Cc: dev-VfR2kkLFssw@public.gmane.org > > Subject: Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bul= k alloc > >=20 > > 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=3Dn > > > > > > CONFIG_RTE_IXGBE_INC_VECTOR=3Dy > > > > > > Building this config gives this error: > > > > > > lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c:69:24: > > > > > > error: =E2=80=98struct igb_rx_queue=E2=80=99 has no member n= amed =E2=80=98fake_mbuf=E2=80=99 > > > > > > > > > > > > I'd like a confirmation that it will be always incompatible. > > > > > > Thanks > > > > > > > > > > Hi Thomas, > > > > > > > > > > I don't think these options should always be incompatible, thou= gh as you > > point > > > > > out you do need to turn on bulk alloc support in order to use t= he vector > > PMD. > > > > > Why do you ask? There are no immediate plans to remove the depe= ndency > > on our end. > > > > > > So you confirm that the ixgbe vpmd really needs Rx bulk alloc and t= his 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 *r= xq) > > > rxq->rx_ring[i] =3D 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 =3D 0; i < RTE_PMD_IXGBE_RX_MAX_BURST; ++i) { > > > rxq->sw_ring[rxq->nb_rx_desc + i].mbuf =3D > > &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 oth= erwise. > > */ > > > uint8_t drop_en; /**< If not 0, set SRRCTL.Dro= p_En. > > */ > > > uint8_t rx_deferred_start; /**< not in global d= ev 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 incompat= ibility, > > > > 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) +=3D > > lib/librte_eal lib/librte_ether > > > DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) +=3D lib/librte_mempool > > lib/librte_mbuf > > > DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) +=3D 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 > > > > >=20 > > Something like the above looks like a good solution to me. > >=20 > > /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. > >=20 Sure, if that can be done, it sounds good. I don't see a huge problem wit= h having a dependency between the two - I can't really see a use case for s= omeone 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 complete= ly. The user can force the selection of the RX function and TX functions at r= un time via the nic setup parameters, so I don't see the need to limit the choice= s at compile time - other than the vpmd which obviously has an instruction set= =20 dependency. /Bruce