DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc
@ 2014-12-01 17:10 Thomas Monjalon
  2014-12-01 17:18 ` Bruce Richardson
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2014-12-01 17:10 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

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
-- 
Thomas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc
  2014-12-01 17:10 [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc Thomas Monjalon
@ 2014-12-01 17:18 ` Bruce Richardson
  2014-12-01 17:22   ` Thomas Monjalon
  0 siblings, 1 reply; 10+ messages in thread
From: Bruce Richardson @ 2014-12-01 17:18 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev

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
> -- 
> Thomas

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.

/Bruce

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc
  2014-12-01 17:18 ` Bruce Richardson
@ 2014-12-01 17:22   ` Thomas Monjalon
  2015-01-29 22:18     ` Thomas Monjalon
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2014-12-01 17:22 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

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.

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.

-- 
Thomas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc
  2014-12-01 17:22   ` Thomas Monjalon
@ 2015-01-29 22:18     ` Thomas Monjalon
  2015-01-29 23:27       ` Bruce Richardson
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2015-01-29 22:18 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

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_BULK_ALLOC),yn)
+$(error The ixgbe vpmd depends on Rx bulk alloc)
+endif
+
 include $(RTE_SDK)/mk/rte.lib.mk

Thanks
-- 
Thomas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc
  2015-01-29 22:18     ` Thomas Monjalon
@ 2015-01-29 23:27       ` Bruce Richardson
  2015-01-29 23:31         ` [dpdk-dev] [PATCH] ixgbe: forbid building vpmd without Rx " Thomas Monjalon
  2015-01-29 23:39         ` [dpdk-dev] [BUG] ixgbe vector cannot compile without " Liang, Cunming
  0 siblings, 2 replies; 10+ messages in thread
From: Bruce Richardson @ 2015-01-29 23:27 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev

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_BULK_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

> Thanks
> -- 
> Thomas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-dev] [PATCH] ixgbe: forbid building vpmd without Rx bulk alloc
  2015-01-29 23:27       ` Bruce Richardson
@ 2015-01-29 23:31         ` Thomas Monjalon
  2015-02-20 11:03           ` Thomas Monjalon
  2015-01-29 23:39         ` [dpdk-dev] [BUG] ixgbe vector cannot compile without " Liang, Cunming
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2015-01-29 23:31 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

CONFIG_RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC is a prerequisite
of CONFIG_RTE_IXGBE_INC_VECTOR.

Reported-by: Alexander Belyakov <abelyako@gmail.com>
Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 lib/librte_pmd_ixgbe/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_pmd_ixgbe/Makefile b/lib/librte_pmd_ixgbe/Makefile
index 3588047..33c17db 100644
--- 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_BULK_ALLOC),yn)
+$(error The ixgbe vpmd depends on Rx bulk alloc)
+endif
+
 include $(RTE_SDK)/mk/rte.lib.mk
-- 
2.2.2

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc
  2015-01-29 23:27       ` Bruce Richardson
  2015-01-29 23:31         ` [dpdk-dev] [PATCH] ixgbe: forbid building vpmd without Rx " Thomas Monjalon
@ 2015-01-29 23:39         ` Liang, Cunming
  2015-01-30  3:38           ` Bruce Richardson
  1 sibling, 1 reply; 10+ messages in thread
From: Liang, Cunming @ 2015-01-29 23:39 UTC (permalink / raw)
  To: Richardson, Bruce, Thomas Monjalon; +Cc: dev



> -----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.
> 
> > Thanks
> > --
> > Thomas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc
  2015-01-29 23:39         ` [dpdk-dev] [BUG] ixgbe vector cannot compile without " Liang, Cunming
@ 2015-01-30  3:38           ` Bruce Richardson
  2015-01-30 19:21             ` Liang, Cunming
  0 siblings, 1 reply; 10+ messages in thread
From: Bruce Richardson @ 2015-01-30  3:38 UTC (permalink / raw)
  To: Liang, Cunming; +Cc: dev

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc
  2015-01-30  3:38           ` Bruce Richardson
@ 2015-01-30 19:21             ` Liang, Cunming
  0 siblings, 0 replies; 10+ messages in thread
From: Liang, Cunming @ 2015-01-30 19:21 UTC (permalink / raw)
  To: Richardson, Bruce; +Cc: dev



> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, January 29, 2015 8:38 PM
> To: Liang, Cunming
> Cc: Thomas Monjalon; dev@dpdk.org
> Subject: Re: [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc
> 
> 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.
[Liang, Cunming] Agree.
> 
> /Bruce

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] ixgbe: forbid building vpmd without Rx bulk alloc
  2015-01-29 23:31         ` [dpdk-dev] [PATCH] ixgbe: forbid building vpmd without Rx " Thomas Monjalon
@ 2015-02-20 11:03           ` Thomas Monjalon
  0 siblings, 0 replies; 10+ messages in thread
From: Thomas Monjalon @ 2015-02-20 11:03 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

2015-01-30 00:31, Thomas Monjalon:
> CONFIG_RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC is a prerequisite
> of CONFIG_RTE_IXGBE_INC_VECTOR.
> 
> Reported-by: Alexander Belyakov <abelyako@gmail.com>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Applied

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-02-20 11:04 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-01 17:10 [dpdk-dev] [BUG] ixgbe vector cannot compile without bulk alloc Thomas Monjalon
2014-12-01 17:18 ` Bruce Richardson
2014-12-01 17:22   ` Thomas Monjalon
2015-01-29 22:18     ` Thomas Monjalon
2015-01-29 23:27       ` Bruce Richardson
2015-01-29 23:31         ` [dpdk-dev] [PATCH] ixgbe: forbid building vpmd without Rx " Thomas Monjalon
2015-02-20 11:03           ` Thomas Monjalon
2015-01-29 23:39         ` [dpdk-dev] [BUG] ixgbe vector cannot compile without " Liang, Cunming
2015-01-30  3:38           ` Bruce Richardson
2015-01-30 19:21             ` Liang, Cunming

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git