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

quoted

quoted

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

quoted

2014-12-01 18:22, Thomas Monjalon:

quoted

2014-12-01 17:18, Bruce Richardson:

quoted

On Mon, Dec 01, 2014 at 06:10:18PM +0100, Thomas Monjalon wrote:

quoted

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’

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

point

quoted

quoted

quoted

out you do need to turn on bulk alloc support in order to use the vector

PMD.

quoted

quoted

quoted

Why do you ask? There are no immediate plans to remove the

dependency

quoted

quoted

on our end.

quoted

So you confirm that the ixgbe vpmd really needs Rx bulk alloc and this kind

of

quoted

quoted

quoted

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)

quoted

quoted

quoted

                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;

quoted

        }

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

quoted

quoted

*/

quoted

        uint8_t             drop_en;  /**< If not 0, set

SRRCTL.Drop_En.

quoted

quoted

*/

quoted

        uint8_t             rx_deferred_start; /**< not in global dev

start.

quoted

quoted

*/

quoted

-#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC
        /** need to alloc dummy mbuf, for wraparound when scanning

hw

quoted

quoted

ring */

quoted

        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

quoted

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

quoted

quoted

quoted

quoted

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

quoted

 DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += lib/librte_mempool

lib/librte_mbuf

quoted

 DEPDIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += lib/librte_net

lib/librte_malloc

quoted

+ifeq

($(CONFIG_RTE_IXGBE_INC_VECTOR)$(CONFIG_RTE_LIBRTE_IXGBE_RX_ALLOW_B

quoted

quoted

ULK_ALLOC),yn)

quoted

+$(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.

quoted

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