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 9D5BAA00BE; Tue, 29 Oct 2019 08:06:13 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 336021BEE0; Tue, 29 Oct 2019 08:06:12 +0100 (CET) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id D5ADA2B8E for ; Tue, 29 Oct 2019 08:06:09 +0100 (CET) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 89963140066; Tue, 29 Oct 2019 07:06:08 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 29 Oct 2019 07:06:03 +0000 To: , , , Thomas Monjalon CC: References: <20191025143314.11162-1-pbhagavatula@marvell.com> <20191029050312.2715-1-pbhagavatula@marvell.com> <20191029050312.2715-4-pbhagavatula@marvell.com> From: Andrew Rybchenko Message-ID: <0775aa6b-cb16-0efe-bace-9a080ecfcabe@solarflare.com> Date: Tue, 29 Oct 2019 10:05:58 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <20191029050312.2715-4-pbhagavatula@marvell.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-25008.003 X-TM-AS-Result: No-9.622400-8.000000-10 X-TMASE-MatchedRID: eVEkOcJu0F6x4Q5iE/G4VfVY7U3NX8Jg6QiT3SsBfyEM74Nf6tTB9tbX wyjvXpaT8XXKCAKJjbuPWx38Q1qImx+zr4ZrJWTdUPktDdOX0fsGchEhVwJY3wl4w4lfxz2c1KG 8hyHCM65V4AbFJy45HWDZOM9/Dzfj3JJWPTAGiVLBjbyj5wYDmktc8DbogbSE1VNlojpO42hx1B wqk2Gx7fmOqTud8R8ozp3ieRI93umczisXkLghg19hr1b06OK1IiTd2l7lf6GFQklSA7AuqNnoq uRwHY3BQeWNJmt20hJCuwkyMeXgVbrlpBICl5Bqxd9CXnue78YML/BvUnxaM7gzdtxt9wymtdA9 AfZ7l9OCRs05n+R8u7xJlxKtuN6Ev1l2Uvx6idpWdFebWIc3VsRB0bsfrpPIx1FPlNAAmcDpPxd IzCV3sTLhEDfQEJfefUvDjwUrj1B27HV5mTXOBJ6oP1a0mRIj X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--9.622400-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-25008.003 X-MDID: 1572332769-BPAnWth0UF5l Subject: Re: [dpdk-dev] [PATCH v14 3/7] ethdev: log offloads that can't be disabled by PMD 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 Pavan, thanks for the patch. Please, see my review notes below. On 10/29/19 8:03 AM, pbhagavatula@marvell.com wrote: > From: Pavan Nikhilesh > > Some PMD can't work when certain offloads are disabled, to work around > this the PMD auto enable the offloads internally and expose it through > dev->data->dev_conf.rxmode.offloads. > After dev_configure is called compare the requested offloads to the > enabled offloads and log any offloads that have been enabled by the PMD. > > Suggested-by: Andrew Rybchenko > Signed-off-by: Pavan Nikhilesh > --- > lib/librte_ethdev/rte_ethdev.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index fef1dbb61..7dfc2f691 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -1142,6 +1142,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, > struct rte_eth_dev *dev; > struct rte_eth_dev_info dev_info; > struct rte_eth_conf orig_conf; > + uint64_t offloads_force_ena; > + uint64_t offload; > int diag; > int ret; > > @@ -1357,6 +1359,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, > goto rollback; > } > > + /* Extract Rx offload bits that can't be disabled and log them */ > + offloads_force_ena = dev_conf->rxmode.offloads ^ > + dev->data->dev_conf.rxmode.offloads; Strictly speaking XOR returns diff and in theory the diff could catch requested but not enabled offload in fact. So, I think the variable name should be offloads_diff. Yes, it is unexpected and very bad, but it adds even more value to the check. May be requested but not enabled offloads should be checked first: ((dev_conf->rxmode.offloads & ~dev->data->dev_conf.rxmode.offloads) != 0) but I think it would be useful to log these offloads as well to help debugging, so, it should be handled below. > + while (__builtin_popcount(offloads_force_ena)) { If we really need it, __builtin_popcountll() should be used, but I think we don't need it here in fact since all we want to know if offloads_diff is 0 or not. So, comparison to 0 will do the job without any builtins. > + offload = 1ULL << __builtin_ctzll(offloads_force_ena); > + offloads_force_ena &= ~offload; Below we should differentiate if the offload is requested but not enabled (error) and if the offload is not requested but enabled (info or warning as Ferruh suggested). I think ret should be set to some error if we find any requested, but not enabled offload and finally configure should fail (after logging of all violations) since it is a strong violation. Same for Tx below. Also I think that it is better to factor out these checks into a separate function since  rte_eth_dev_configure() is already too long. It looks like that parameters should port ID, requested and result offloads. > + RTE_ETHDEV_LOG(INFO, "Port %u can't disable Rx offload %s\n", > + port_id, rte_eth_dev_rx_offload_name(offload)); > + } > + > + /* Extract Tx offload bits that can't be disabled and log them */ > + offloads_force_ena = dev_conf->txmode.offloads ^ > + dev->data->dev_conf.txmode.offloads; > + while (__builtin_popcount(offloads_force_ena)) { > + offload = 1ULL << __builtin_ctzll(offloads_force_ena); > + offloads_force_ena &= ~offload; > + RTE_ETHDEV_LOG(INFO, "Port %u can't disable Tx offload %s\n", > + port_id, rte_eth_dev_tx_offload_name(offload)); > + } > + > return 0; > > rollback: