From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 7FA625A6D for ; Wed, 11 Mar 2015 18:19:01 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP; 11 Mar 2015 10:17:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,382,1422950400"; d="scan'208";a="465862743" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by FMSMGA003.fm.intel.com with ESMTP; 11 Mar 2015 10:12:11 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.117]) by IRSMSX103.ger.corp.intel.com ([169.254.3.247]) with mapi id 14.03.0195.001; Wed, 11 Mar 2015 17:18:58 +0000 From: "Ananyev, Konstantin" To: Vlad Zolotarov , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2 3/3] ixgbe: Unify the rx_pkt_bulk callback initialization Thread-Index: AQHQXBGxcXAvSqFUrEa+612bbpH2r50Xbiww Date: Wed, 11 Mar 2015 17:18:56 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258213F58F0@irsmsx105.ger.corp.intel.com> References: <1425918532-8601-1-git-send-email-vladz@cloudius-systems.com> <1425918532-8601-4-git-send-email-vladz@cloudius-systems.com> <2601191342CEEE43887BDE71AB977258213F579B@irsmsx105.ger.corp.intel.com> <550061DD.6010403@cloudius-systems.com> In-Reply-To: <550061DD.6010403@cloudius-systems.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 3/3] ixgbe: Unify the rx_pkt_bulk callback initialization 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: Wed, 11 Mar 2015 17:19:02 -0000 > -----Original Message----- > From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] > Sent: Wednesday, March 11, 2015 3:40 PM > To: Ananyev, Konstantin; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2 3/3] ixgbe: Unify the rx_pkt_bulk callb= ack initialization >=20 >=20 >=20 > On 03/11/15 14:45, Ananyev, Konstantin wrote: > > > >> -----Original Message----- > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vlad Zolotarov > >> Sent: Monday, March 09, 2015 4:29 PM > >> To: dev@dpdk.org > >> Subject: [dpdk-dev] [PATCH v2 3/3] ixgbe: Unify the rx_pkt_bulk callba= ck initialization > >> > >> - Set the callback in a single function that is called from > >> ixgbe_dev_rx_init() for a primary process and from eth_ixgbe_dev= _init() > >> for a secondary processes. This is instead of multiple, hard to = track places. > >> - Added ixgbe_hw.rx_bulk_alloc_allowed - see ixgbe_hw.rx_vec_allow= ed description below. > >> - Added ixgbe_hw.rx_vec_allowed: like with Bulk Allocation, Vector= Rx is > >> enabled or disabled on a per-port level. All queues have to meet= the appropriate > >> preconditions and if any of them doesn't - the feature has to be= disabled. > >> Therefore ixgbe_hw.rx_vec_allowed will be updated during each qu= eues configuration > >> (rte_eth_rx_queue_setup()) and then used in rte_eth_dev_start() = to configure the > >> appropriate callbacks. The same happens with ixgbe_hw.rx_vec_all= owed in a Bulk Allocation > >> context. > >> - Bugs fixed: > >> - Vector scattered packets callback was called regardless the a= ppropriate > >> preconditions: > >> - Vector Rx specific preconditions. > >> - Bulk Allocation preconditions. > >> - Vector Rx was enabled/disabled according to the last queue se= tting and not > >> based on all queues setting (which may be different for each = queue). > >> > >> Signed-off-by: Vlad Zolotarov > >> --- > >> New in v2: > >> - Fixed an broken compilation. > >> --- > >> lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h | 2 + > >> lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 13 ++- > >> lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 183 +++++++++++++++++++++-= ---------- > >> lib/librte_pmd_ixgbe/ixgbe_rxtx.h | 22 +++- > >> 4 files changed, 152 insertions(+), 68 deletions(-) > >> > >> diff --git a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h b/lib/librte_pmd_= ixgbe/ixgbe/ixgbe_type.h > >> index c67d462..9a66370 100644 > >> --- a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h > >> +++ b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h > >> @@ -3657,6 +3657,8 @@ struct ixgbe_hw { > >> bool force_full_reset; > >> bool allow_unsupported_sfp; > >> bool wol_enabled; > >> + bool rx_bulk_alloc_allowed; > >> + bool rx_vec_allowed; > >> }; > >> > >> #define ixgbe_call_func(hw, func, params, error) \ > >> diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgb= e/ixgbe_ethdev.c > >> index 9bdc046..9d3de1a 100644 > >> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c > >> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c > >> @@ -760,8 +760,8 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct = eth_driver *eth_drv, > >> "Using default TX function."); > >> } > >> > >> - if (eth_dev->data->scattered_rx) > >> - eth_dev->rx_pkt_burst =3D ixgbe_recv_scattered_pkts; > >> + set_rx_function(eth_dev); > >> + > >> return 0; > >> } > >> pci_dev =3D eth_dev->pci_dev; > >> @@ -772,6 +772,13 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct= eth_driver *eth_drv, > >> hw->hw_addr =3D (void *)pci_dev->mem_resource[0].addr; > >> hw->allow_unsupported_sfp =3D 1; > >> > >> + /* > >> + * Initialize to TRUE. If any of Rx queues doesn't meet the bulk > >> + * allocation or vector Rx preconditions we will reset it. > >> + */ > >> + hw->rx_bulk_alloc_allowed =3D true; > >> + hw->rx_vec_allowed =3D true; > >> + > >> /* Initialize the shared code (base driver) */ > >> #ifdef RTE_NIC_BYPASS > >> diag =3D ixgbe_bypass_init_shared_code(hw); > >> @@ -1641,6 +1648,8 @@ ixgbe_dev_stop(struct rte_eth_dev *dev) > >> > >> /* Clear stored conf */ > >> dev->data->scattered_rx =3D 0; > >> + hw->rx_bulk_alloc_allowed =3D false; > >> + hw->rx_vec_allowed =3D false; > > If dev_stop() sets it to 'false', who will reset it back to 'true' then= ? >=20 > Oops. I'm afraid I've messed the things up a bit... ;) I've seen this > issue when I checked the fix for the dev_stop()->dev_start() issue in > the LRO patch. To remind, u have noticed that I wasn't freeing the > not-yet-completed RSC aggregations in ixgbe_rx_queue_release_mbufs(). So > when I was verifying the fix I've noticed the issue with the flags above > and fixed it. But I've put the fix in the PATCH3 v7 of LRO series > instead of putting it here... ;) I think I'll have to respin both series > again... Ufff.... :( >=20 > > > >> /* Clear recorded link status */ > >> memset(&link, 0, sizeof(link)); > >> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/= ixgbe_rxtx.c > >> index ce9658e..a00f5c9 100644 > >> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > >> @@ -2074,12 +2074,12 @@ check_rx_burst_bulk_alloc_preconditions(__rte_= unused struct igb_rx_queue *rxq) > >> > >> /* Reset dynamic igb_rx_queue fields back to defaults */ > >> static void > >> -ixgbe_reset_rx_queue(struct igb_rx_queue *rxq) > >> +ixgbe_reset_rx_queue(struct ixgbe_hw *hw, struct igb_rx_queue *rxq) > >> { > >> static const union ixgbe_adv_rx_desc zeroed_desc =3D { .read =3D { > >> .pkt_addr =3D 0}}; > >> unsigned i; > >> - uint16_t len; > >> + uint16_t len =3D rxq->nb_rx_desc; > >> > >> /* > >> * By default, the Rx queue setup function allocates enough memory = for > >> @@ -2091,14 +2091,9 @@ ixgbe_reset_rx_queue(struct igb_rx_queue *rxq) > >> * constraints here to see if we need to zero out memory after the = end > >> * of the H/W descriptor ring. > >> */ > >> -#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC > >> - if (check_rx_burst_bulk_alloc_preconditions(rxq) =3D=3D 0) > >> + if (hw->rx_bulk_alloc_allowed) > >> /* zero out extra memory */ > >> - len =3D (uint16_t)(rxq->nb_rx_desc + RTE_PMD_IXGBE_RX_MAX_BURST); > >> - else > >> -#endif > >> - /* do not zero out extra memory */ > >> - len =3D rxq->nb_rx_desc; > >> + len +=3D RTE_PMD_IXGBE_RX_MAX_BURST; > >> > >> /* > >> * Zero out HW ring memory. Zero out extra memory at the end of > >> @@ -2140,7 +2135,6 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev= , > >> const struct rte_memzone *rz; > >> struct igb_rx_queue *rxq; > >> struct ixgbe_hw *hw; > >> - int use_def_burst_func =3D 1; > >> uint16_t len; > >> > >> PMD_INIT_FUNC_TRACE(); > >> @@ -2221,15 +2215,27 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *d= ev, > >> rxq->rx_ring =3D (union ixgbe_adv_rx_desc *) rz->addr; > >> > >> /* > >> + * Certain constraints must be met in order to use the bulk buffer > >> + * allocation Rx burst function. If any of Rx queues doesn't meet th= em > >> + * the feature should be disabled for the whole port. > >> + */ > >> + if (check_rx_burst_bulk_alloc_preconditions(rxq)) { > >> + PMD_INIT_LOG(DEBUG, "queue[%d] doesn't meet Rx Bulk Alloc " > >> + "preconditions - canceling the feature for " > >> + "the whole port[%d]", > >> + rxq->queue_id, rxq->port_id); > >> + hw->rx_bulk_alloc_allowed =3D false; > >> + } > >> + > >> + /* > >> * Allocate software ring. Allow for space at the end of the > >> * S/W ring to make sure look-ahead logic in bulk alloc Rx burst > >> * function does not access an invalid memory region. > >> */ > >> -#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC > >> - len =3D (uint16_t)(nb_desc + RTE_PMD_IXGBE_RX_MAX_BURST); > >> -#else > >> len =3D nb_desc; > >> -#endif > >> + if (hw->rx_bulk_alloc_allowed) > >> + len +=3D RTE_PMD_IXGBE_RX_MAX_BURST; > >> + > > Looking at it: it is probably easier and cleaner to remove all these if= defs if/else > > And always setup len =3D nb_desc + RTE_PMD_IXGBE_RX_MAX_BURST; >=20 > Sounds reasonable. Since we talk about it - could u, pls., explain to me > why is all this padding needed the first place? I must say I don't > exactly understand why is it needed at all... ;) As I remember: to speedup things ixgbe_rx_scan_hw_ring() always scans HW de= scriptors in bulk of 8.=20 So even if we have 128 RXDs used by HW and rx_tail=3D=3D125, it would scan = rx_ring[125-132] entries. So after proper 128 RXDs that will be accessed by HW, we setup RTE_PMD_IXG= BE_RX_MAX_BURST RXDs. They supposed to be zeroed by SW at startup and should never be overw= ritten by HW. So their DD bit will always be 0. Same thing for corresponding sw_ring[] entries. Now while thinking about it couldn't remember why do we need whole RTE_PMD_= IXGBE_RX_MAX_BURST fake entries, not just LOOK_AHEAD... But anyway, that's the idea behind it. About always setup len =3D nb_desc + RTE_PMD_IXGBE_RX_MAX_BURST - I am probably spoke too early :( After second thought - it could be situation when use requested IXGBE_MAX_R= ING_DESC RXDs in total with 'plain' (no bulk) recv. In that case our len might be bigger than rx_ring memzone. Of course we could always allocate for rx_ring (IXGBE_MAX_RING_DESC + RTE_P= MD_IXGBE_RX_MAX_BURST) entries... But that's probably too many unrelated changes in that patch. Safer for to leave it as it is for now. >=20 > > > > > >> rxq->sw_ring =3D rte_zmalloc_socket("rxq->sw_ring", > >> sizeof(struct igb_rx_entry) * len, > >> RTE_CACHE_LINE_SIZE, socket_id); > >> @@ -2240,42 +2246,18 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *d= ev, > >> PMD_INIT_LOG(DEBUG, "sw_ring=3D%p hw_ring=3D%p dma_addr=3D0x%"PRIx6= 4, > >> rxq->sw_ring, rxq->rx_ring, rxq->rx_ring_phys_addr); > >> > >> - /* > >> - * Certain constraints must be met in order to use the bulk buffer > >> - * allocation Rx burst function. > >> - */ > >> - use_def_burst_func =3D check_rx_burst_bulk_alloc_preconditions(rxq); > >> + if (!rte_is_power_of_2(nb_desc)) { > >> + PMD_INIT_LOG(DEBUG, "queue[%d] doesn't meet Vector Rx " > >> + "preconditions - canceling the feature for " > >> + "the whole port[%d]", > >> + rxq->queue_id, rxq->port_id); > >> + hw->rx_vec_allowed =3D false; > >> + } else > >> + ixgbe_rxq_vec_setup(rxq); > >> > >> -#ifdef RTE_IXGBE_INC_VECTOR > >> - ixgbe_rxq_vec_setup(rxq); > >> -#endif > >> - /* Check if pre-conditions are satisfied, and no Scattered Rx */ > >> - if (!use_def_burst_func && !dev->data->scattered_rx) { > >> -#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC > >> - PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions are " > >> - "satisfied. Rx Burst Bulk Alloc function will be " > >> - "used on port=3D%d, queue=3D%d.", > >> - rxq->port_id, rxq->queue_id); > >> - dev->rx_pkt_burst =3D ixgbe_recv_pkts_bulk_alloc; > >> -#ifdef RTE_IXGBE_INC_VECTOR > >> - if (!ixgbe_rx_vec_condition_check(dev) && > >> - (rte_is_power_of_2(nb_desc))) { > >> - PMD_INIT_LOG(INFO, "Vector rx enabled, please make " > >> - "sure RX burst size no less than 32."); > >> - dev->rx_pkt_burst =3D ixgbe_recv_pkts_vec; > >> - } > >> -#endif > >> -#endif > >> - } else { > >> - PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions " > >> - "are not satisfied, Scattered Rx is requested, " > >> - "or RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC is not " > >> - "enabled (port=3D%d, queue=3D%d).", > >> - rxq->port_id, rxq->queue_id); > >> - } > >> dev->data->rx_queues[queue_idx] =3D rxq; > >> > >> - ixgbe_reset_rx_queue(rxq); > >> + ixgbe_reset_rx_queue(hw, rxq); > >> > >> return 0; > >> } > >> @@ -2329,6 +2311,7 @@ void > >> ixgbe_dev_clear_queues(struct rte_eth_dev *dev) > >> { > >> unsigned i; > >> + struct ixgbe_hw *hw =3D IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_priva= te); > >> > >> PMD_INIT_FUNC_TRACE(); > >> > >> @@ -2344,7 +2327,7 @@ ixgbe_dev_clear_queues(struct rte_eth_dev *dev) > >> struct igb_rx_queue *rxq =3D dev->data->rx_queues[i]; > >> if (rxq !=3D NULL) { > >> ixgbe_rx_queue_release_mbufs(rxq); > >> - ixgbe_reset_rx_queue(rxq); > >> + ixgbe_reset_rx_queue(hw, rxq); > >> } > >> } > >> } > >> @@ -3506,6 +3489,58 @@ ixgbe_dev_mq_tx_configure(struct rte_eth_dev *d= ev) > >> return 0; > >> } > >> > >> +void set_rx_function(struct rte_eth_dev *dev) > >> +{ > >> + struct ixgbe_hw *hw =3D IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_priva= te); > >> + > >> + if (ixgbe_rx_vec_condition_check(dev)) { > >> + PMD_INIT_LOG(DEBUG, "Port[%d] doesn't meet Vector Rx " > >> + "preconditions or RTE_IXGBE_INC_VECTOR is " > >> + "not enabled", > >> + dev->data->port_id); > >> + hw->rx_vec_allowed =3D false; > >> + } > >> + > >> + /* Check if bulk alloc is allowed and no Scattered Rx */ > >> + if (hw->rx_bulk_alloc_allowed && !dev->data->scattered_rx) { > >> + PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions are " > >> + "satisfied. Rx Burst Bulk Alloc function " > >> + "will be used on port=3D%d.", > >> + dev->data->port_id); > >> + dev->rx_pkt_burst =3D ixgbe_recv_pkts_bulk_alloc; > >> + > >> + if (hw->rx_vec_allowed) { > >> + PMD_INIT_LOG(INFO, "Vector rx enabled, please make " > >> + "sure RX burst size no less " > >> + "than 32."); > >> + dev->rx_pkt_burst =3D ixgbe_recv_pkts_vec; > >> + } > >> + } else { > >> + dev->rx_pkt_burst =3D ixgbe_recv_pkts; > >> + > >> + PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions are not " > >> + "satisfied, or Scattered Rx is requested, " > >> + "or RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC " > >> + "is not enabled (port=3D%d).", > >> + dev->data->port_id); > >> + } > >> + > >> + if (dev->data->scattered_rx) { > >> + if (hw->rx_vec_allowed && hw->rx_bulk_alloc_allowed) { > >> + PMD_INIT_LOG(DEBUG, "Using Vector Scattered Rx " > >> + "callback (port=3D%d).", > >> + dev->data->port_id); > >> + dev->rx_pkt_burst =3D ixgbe_recv_scattered_pkts_vec; > >> + } else { > >> + PMD_INIT_LOG(DEBUG, "Using Regualr (non-vector) " > >> + "Scattered Rx callback " > >> + "(port=3D%d).", > >> + dev->data->port_id); > >> + dev->rx_pkt_burst =3D ixgbe_recv_scattered_pkts; > >> + } > >> + } > >> +} > > if scattered_rx !=3D 0 it will setup rx_pkt_burst twice. >=20 > I don't understand what u mean here. If scattered_rx !=3D 0 then if vecto= r > Rx preconditions are met then rx_pkt_burst will be set to a vector-ed > version of the callback and if not - to the non-vectored one. It will be > exactly one setting... What do I miss? ;) I meant for 'dev->data->scattered_rx =3D=3D 1' we at first would assign rx_= pkt_burst here:=20 if (hw->rx_bulk_alloc_allowed && !dev->data->scattered_rx) { .... } else { dev->rx_pkt_burst =3D ixgbe_recv_pkts; } And then re-assign again here: if (dev->data->scattered_rx) { ... } >=20 > > Nothing wrong, but looks a bit clumsy. > > Might be a bit clearer to reorder it a bit: > > > > If (scattered_rx) { > > ... > > } else if (rx_bulk_alloc_allowed) { > > ... > > } else { > > ... > > } > > > > > >> + > >> /* > >> * Initializes Receive Unit. > >> */ > >> @@ -3641,23 +3676,17 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev) > >> buf_size =3D (uint16_t) ((srrctl & IXGBE_SRRCTL_BSIZEPKT_MASK) << > >> IXGBE_SRRCTL_BSIZEPKT_SHIFT); > >> > >> - if (dev->data->dev_conf.rxmode.enable_scatter || > >> - /* It adds dual VLAN length for supporting dual VLAN */ > >> - (dev->data->dev_conf.rxmode.max_rx_pkt_len + > >> - 2 * IXGBE_VLAN_TAG_SIZE) > buf_size){ > >> - if (!dev->data->scattered_rx) > >> - PMD_INIT_LOG(DEBUG, "forcing scatter mode"); > >> + /* It adds dual VLAN length for supporting dual VLAN */ > >> + if (dev->data->dev_conf.rxmode.max_rx_pkt_len + > >> + 2 * IXGBE_VLAN_TAG_SIZE > buf_size) > >> dev->data->scattered_rx =3D 1; > >> -#ifdef RTE_IXGBE_INC_VECTOR > >> - if (rte_is_power_of_2(rxq->nb_rx_desc)) > >> - dev->rx_pkt_burst =3D > >> - ixgbe_recv_scattered_pkts_vec; > >> - else > >> -#endif > >> - dev->rx_pkt_burst =3D ixgbe_recv_scattered_pkts; > >> - } > >> } > >> > >> + if (dev->data->dev_conf.rxmode.enable_scatter) > >> + dev->data->scattered_rx =3D 1; > >> + > >> + set_rx_function(dev); > >> + > >> /* > >> * Device configured with multiple RX queues. > >> */ > >> @@ -3933,7 +3962,7 @@ ixgbe_dev_rx_queue_stop(struct rte_eth_dev *dev,= uint16_t rx_queue_id) > >> rte_delay_us(RTE_IXGBE_WAIT_100_US); > >> > >> ixgbe_rx_queue_release_mbufs(rxq); > >> - ixgbe_reset_rx_queue(rxq); > >> + ixgbe_reset_rx_queue(hw, rxq); > >> } else > >> return -1; > >> > >> @@ -4289,3 +4318,31 @@ ixgbevf_dev_rxtx_start(struct rte_eth_dev *dev) > >> > >> } > >> } > >> + > >> +/* Stubs needed for linkage when CONFIG_RTE_IXGBE_INC_VECTOR is set t= o 'n' */ > >> +#ifndef RTE_IXGBE_INC_VECTOR > > Instead of ifndef above, can these function below be defined with _attr= ibute__ ((weak))? >=20 > Yes they could. Do u think it would be more readable then? What I mean > here is that DPDK is full of ifdefs because there is a million of > configuration macros in rte_config.h. So, IMHO it would be better to > explicitly indicate when these stubs are going to be used. However I'm > also against the #ifdef's in general and actually like your proposal. > Pls., comment. My vote would be to remove ifndef and make them 'weak'. As you said - less ifdefs is better. >=20 > > > >> +int ixgbe_rx_vec_condition_check( > >> + struct rte_eth_dev __rte_unused *dev) > >> +{ > >> + return -1; > >> +} > >> + > >> +uint16_t > >> +ixgbe_recv_pkts_vec(void __rte_unused *rx_queue, > >> + struct rte_mbuf __rte_unused **rx_pkts, > >> + uint16_t __rte_unused nb_pkts) > >> +{ > >> + return 0; > >> +} > >> + > >> +uint16_t ixgbe_recv_scattered_pkts_vec(void __rte_unused *rx_queue, > >> + struct rte_mbuf __rte_unused **rx_pkts, uint16_t __rte_unused nb_pkt= s) > >> +{ > >> + return 0; > >> +} > >> + > >> +int ixgbe_rxq_vec_setup(struct igb_rx_queue __rte_unused *rxq) > >> +{ > >> + return -1; > >> +} > >> +#endif > >> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.h b/lib/librte_pmd_ixgbe/= ixgbe_rxtx.h > >> index 329007c..bbe5ff3 100644 > >> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.h > >> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.h > >> @@ -255,16 +255,32 @@ struct ixgbe_txq_ops { > >> */ > >> void set_tx_function(struct rte_eth_dev *dev, struct igb_tx_queue *t= xq); > >> > >> -#ifdef RTE_IXGBE_INC_VECTOR > >> +/** > >> + * Sets the rx_pkt_burst callback in the ixgbe rte_eth_dev instance. > >> + * > >> + * Sets the callback based on the device parameters: > >> + * - ixgbe_hw.rx_bulk_alloc_allowed > >> + * - rte_eth_dev_data.scattered_rx > >> + * - rte_eth_dev_data.lro > >> + * - conditions checked in ixgbe_rx_vec_condition_check() > >> + * > >> + * This means that the parameters above have to be configured prior = to calling > >> + * to this function. > >> + * > >> + * @dev rte_eth_dev handle > >> + */ > >> +void set_rx_function(struct rte_eth_dev *dev); > >> + > >> uint16_t ixgbe_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pk= ts, > >> uint16_t nb_pkts); > >> uint16_t ixgbe_recv_scattered_pkts_vec(void *rx_queue, > >> struct rte_mbuf **rx_pkts, uint16_t nb_pkts); > >> +int ixgbe_rx_vec_condition_check(struct rte_eth_dev *dev); > >> +int ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq); > > > >> +#ifdef RTE_IXGBE_INC_VECTOR > > Please add an empty line(s) around 'ifdef' - makes it much easier to r= ead. >=20 > NP. Here I just followed the previous code style of this section. ;) I > agree with u though... ;) >=20 > > > >> uint16_t ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pk= ts, > >> uint16_t nb_pkts); > >> int ixgbe_txq_vec_setup(struct igb_tx_queue *txq); > >> -int ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq); > >> -int ixgbe_rx_vec_condition_check(struct rte_eth_dev *dev); > >> #endif > >> > >> #endif > >> -- > >> 2.1.0