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 D7824A00BE; Tue, 29 Oct 2019 09:42:39 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id AB9731BED2; Tue, 29 Oct 2019 09:42:39 +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 C93081BED5 for ; Tue, 29 Oct 2019 09:42:30 +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-us3.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 56E26980067; Tue, 29 Oct 2019 08:42:29 +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 08:42:23 +0000 To: Pavan Nikhilesh Bhagavatula , "ferruh.yigit@intel.com" , Jerin Jacob Kollanukkaran , Thomas Monjalon CC: "dev@dpdk.org" References: <20191025143314.11162-1-pbhagavatula@marvell.com> <20191029050312.2715-1-pbhagavatula@marvell.com> <20191029050312.2715-4-pbhagavatula@marvell.com> <0775aa6b-cb16-0efe-bace-9a080ecfcabe@solarflare.com> From: Andrew Rybchenko Message-ID: <5c5d1908-93f8-5393-f6aa-9cbf57232608@solarflare.com> Date: Tue, 29 Oct 2019 11:42:19 +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: 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-17.446400-8.000000-10 X-TMASE-MatchedRID: dL10VBB8yoex4Q5iE/G4VfZvT2zYoYOwt3aeg7g/usAutoY2UtFqGK/B 5+2WRWsIZnH9wlvrd9uYZfjORODtZTM9BBRuZZ1v+CjwEqX1p7n1+9bO3CCbk2vxMifzTnhCsmc +HzD5HmjdvO9TrNr2cNH26ep1B6hXeoT8ghp9T7+qNnzrkU+2mg/o5bNHEsCTV7MQTbTl029KQ+ EC+8ID2T35UX3NN0TkQc0lWOKsR2ViWV0DQ85LUkf49ONH0RaSIrMoP5XxqGcwCbMdSs+QHl+Vs 3hc+s8hfJ9bnWp9qVYdcBoGeO1lYuVCna7Y4RCFSHCU59h5KrGmvNzKvPYvXcOtrkhuZC9WN17X wn/rN3PCyfYnAuBEjtZQ8unXOAVhoOeRcT+IlrFSW7+rBmR2KlWiBZGwRpH+K6BnL+9AZm34eYE /JF5KDDtEP3T6petR4jnwuXf7k75CRB4ladU5szqf0DtPln4nZIT8qopMUXs6ievrBsJv2ygzmF kIPxkzNsE5/VLWW+PpatQevkymW4G0LbypqyKqxVtvemNbkyfdXhRKGhNdp7mGRIy/zvxFNiLP5 F13qP7nzlXMYw4XMAGLeSok4rrZC24oEZ6SpSkj80Za3RRg8LtjOMjwnBxaeCF/4VwhzsQbKgvk 2SAd3GuqLjvcva3cmqJ0Ri/9iBA= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--17.446400-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-25008.003 X-MDID: 1572338550-zbHW79MpN12g 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" On 10/29/19 11:33 AM, Pavan Nikhilesh Bhagavatula wrote: > >> -----Original Message----- >> From: dev On Behalf Of Andrew Rybchenko >> Sent: Tuesday, October 29, 2019 12:36 PM >> To: Pavan Nikhilesh Bhagavatula ; >> ferruh.yigit@intel.com; Jerin Jacob Kollanukkaran >> ; Thomas Monjalon >> Cc: dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH v14 3/7] ethdev: log offloads that can't >> be disabled by PMD >> >> 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) > Isn't the above already taken care through > " > /* Any requested offloading must be within its device capabilities */ > if ((dev_conf->rxmode.offloads & dev_info.rx_offload_capa) != > dev_conf->rxmode.offloads) { > RTE_ETHDEV_LOG(ERR, > "Ethdev port_id=%u requested Rx offloads 0x%"PRIx64" doesn't match Rx offloads " > "capabilities 0x%"PRIx64" in %s()\n", > port_id, dev_conf->rxmode.offloads, > dev_info.rx_offload_capa, > __func__); > ret = -EINVAL; > goto rollback; > } > if ((dev_conf->txmode.offloads & dev_info.tx_offload_capa) != > dev_conf->txmode.offloads) { > RTE_ETHDEV_LOG(ERR, > "Ethdev port_id=%u requested Tx offloads 0x%"PRIx64" doesn't match Tx offloads " > "capabilities 0x%"PRIx64" in %s()\n", > port_id, dev_conf->txmode.offloads, > dev_info.tx_offload_capa, > __func__); > ret = -EINVAL; > goto rollback; > } > " > > Do you think PMDs will advertise but not enable? Above we check the request for correctness. Here I'd like to check that the result is correct. The problem here that we make it 100% legal for PMD to adjust dev->data->dev_conf.rxmode.offloads so it is better to check the result as well. >> 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. > Yes, we can skip using __builtin_popcount. > >>> + 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. >> > I will move it to static function in next iteration. > >>> + 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: