From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5F4B1A056A; Fri, 6 Mar 2020 09:48:51 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 966E11BFDD; Fri, 6 Mar 2020 09:48:50 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id B9F362BA8; Fri, 6 Mar 2020 09:48:48 +0100 (CET) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Mar 2020 00:48:47 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,521,1574150400"; d="scan'208";a="241116622" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.17]) by orsmga003.jf.intel.com with ESMTP; 06 Mar 2020 00:48:45 -0800 Date: Fri, 6 Mar 2020 16:46:26 +0800 From: Ye Xiaolong To: Qi Zhang Cc: beilei.xing@intel.com, dev@dpdk.org, stable@dpdk.org Message-ID: <20200306084626.GB18727@intel.com> References: <20200304114038.39644-1-qi.z.zhang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200304114038.39644-1-qi.z.zhang@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH] net/ice: remove bulk alloc compile option X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, Qi Thanks for the cleanup. On 03/04, Qi Zhang wrote: >Remove CONFIG_RTE_LIBRTE_ICE_RX_ALLOW_BULK_ALLOC with below >consideration: > >1. a default Rx path can always be selected by setting a proper > rx_free_thresh value at runtime, see > ice_check_rx_burst_bulk_alloc_preconditions. > >2. its not a big deal to always reserve more space for desc ring. > "ring_size = (uint16_t)(rxq->nb_rx_desc + ICE_RX_MAX_BURST);" > >3. Fixes a potential invalid memory access in ice_reset_rx_queue. > if CONFIG_RTE_LIBRTE_ICE_RX_ALLOW_BULK_ALLOC is turned on while > ice_check_rx_burst_bulk_alloc_preconditions return fail. > below code will have problem. > > for (i = 0; i < ICE_RX_MAX_BURST; ++i) > rxq->sw_ring[rxq->nb_rx_desc + i].mbuf = &rxq->fake_mbuf; > >Fixes: 50370662b727 ("net/ice: support device and queue ops") >Cc: stable@dpdk.org > >Signed-off-by: Qi Zhang >--- > config/common_base | 1 - > doc/guides/nics/ice.rst | 4 --- > drivers/net/ice/ice_rxtx.c | 64 ++++++++++------------------------------------ > 3 files changed, 13 insertions(+), 56 deletions(-) > >diff --git a/config/common_base b/config/common_base >index 7ca2f28b1..c31175f9d 100644 >--- a/config/common_base >+++ b/config/common_base >@@ -337,7 +337,6 @@ CONFIG_RTE_LIBRTE_ICE_PMD=y > CONFIG_RTE_LIBRTE_ICE_DEBUG_RX=n > CONFIG_RTE_LIBRTE_ICE_DEBUG_TX=n > CONFIG_RTE_LIBRTE_ICE_DEBUG_TX_FREE=n >-CONFIG_RTE_LIBRTE_ICE_RX_ALLOW_BULK_ALLOC=y > CONFIG_RTE_LIBRTE_ICE_16BYTE_RX_DESC=n > > # Compile burst-oriented IAVF PMD driver >diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst >index cde3fd620..8af32dabf 100644 >--- a/doc/guides/nics/ice.rst >+++ b/doc/guides/nics/ice.rst >@@ -54,10 +54,6 @@ Please note that enabling debugging options may affect system performance. > > Toggle display of generic debugging messages. > >-- ``CONFIG_RTE_LIBRTE_ICE_RX_ALLOW_BULK_ALLOC`` (default ``y``) >- >- Toggle bulk allocation for RX. >- > - ``CONFIG_RTE_LIBRTE_ICE_16BYTE_RX_DESC`` (default ``n``) > > Toggle to use a 16-byte RX descriptor, by default the RX descriptor is 32 byte. >diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c >index 60c411bfa..c7e5fc484 100644 >--- a/drivers/net/ice/ice_rxtx.c >+++ b/drivers/net/ice/ice_rxtx.c >@@ -236,17 +236,15 @@ _ice_rx_queue_release_mbufs(struct ice_rx_queue *rxq) > rxq->sw_ring[i].mbuf = NULL; > } > } >-#ifdef RTE_LIBRTE_ICE_RX_ALLOW_BULK_ALLOC >- if (rxq->rx_nb_avail == 0) >- return; >- for (i = 0; i < rxq->rx_nb_avail; i++) { >- struct rte_mbuf *mbuf; >- >- mbuf = rxq->rx_stage[rxq->rx_next_avail + i]; >- rte_pktmbuf_free_seg(mbuf); >- } >- rxq->rx_nb_avail = 0; >-#endif /* RTE_LIBRTE_ICE_RX_ALLOW_BULK_ALLOC */ >+ if (rxq->rx_nb_avail == 0) >+ return; >+ for (i = 0; i < rxq->rx_nb_avail; i++) { >+ struct rte_mbuf *mbuf; >+ >+ mbuf = rxq->rx_stage[rxq->rx_next_avail + i]; >+ rte_pktmbuf_free_seg(mbuf); >+ } How about just for (i = 0; i < rxq->rx_nb_avail; i++) rte_pktmbuf_free_seg(rxq->rx_stage[rxq->rx_next_avail + i]); [snip] For the rest, Acked-by: Xiaolong Ye And can this cleanup be applied to i40e as well? I think it's good to have less configurations generally. Thanks, Xiaolong