From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 164B75F45 for ; Thu, 10 May 2018 11:25:34 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 37E14400066; Thu, 10 May 2018 09:25:32 +0000 (UTC) Received: from [192.168.38.17] (84.52.114.114) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Thu, 10 May 2018 10:25:26 +0100 To: Wei Dai , , CC: , Qi Zhang References: <1525913371-18178-1-git-send-email-wei.dai@intel.com> <1525913806-41723-1-git-send-email-wei.dai@intel.com> From: Andrew Rybchenko Message-ID: Date: Thu, 10 May 2018 12:25:22 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1525913806-41723-1-git-send-email-wei.dai@intel.com> Content-Language: en-GB X-Originating-IP: [84.52.114.114] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23834.003 X-TM-AS-Result: No--15.698100-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1525944333-yOZlCI2+gXDY Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v10] ethdev: new Rx/Tx offloads API 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: , X-List-Received-Date: Thu, 10 May 2018 09:25:34 -0000 On 05/10/2018 03:56 AM, Wei Dai wrote: > This patch check if a input requested offloading is valid or not. > Any reuqested offloading must be supported in the device capabilities. > Any offloading is disabled by default if it is not set in the parameter > dev_conf->[rt]xmode.offloads to rte_eth_dev_configure( ) and > [rt]x_conf->offloads to rte_eth_[rt]x_queue_setup( ). > If any offloading is enabled in rte_eth_dev_configure( ) by application, > it is enabled on all queues no matter whether it is per-queue or > per-port type and no matter whether it is set or cleared in > [rt]x_conf->offloads to rte_eth_[rt]x_queue_setup( ). > If a per-queue offloading hasn't be enabled in rte_eth_dev_configure( ), > it can be enabled or disabled for individual queue in > ret_eth_[rt]x_queue_setup( ). > A new added offloading is the one which hasn't been enabled in > rte_eth_dev_configure( ) and is reuqested to be enabled in > rte_eth_[rt]x_queue_setup( ), it must be per-queue type, > otherwise triger an error log. > The underlying PMD must be aware that the requested offloadings > to PMD specific queue_setup( ) function only carries those > new added offloadings of per-queue type. > > This patch can make above such checking in a common way in rte_ethdev > layer to avoid same checking in underlying PMD. > > This patch assumes that all PMDs in 18.05-rc2 have already > converted to offload API defined in 17.11 . It also assumes > that all PMDs can return correct offloading capabilities > in rte_eth_dev_infos_get( ). > > In the beginning of [rt]x_queue_setup( ) of underlying PMD, > add offloads = [rt]xconf->offloads | > dev->data->dev_conf.[rt]xmode.offloads; to keep same as offload API > defined in 17.11 to avoid upper application broken due to offload > API change. > PMD can use the info that input [rt]xconf->offloads only carry > the new added per-queue offloads to do some optimization or some > code change on base of this patch. > > Signed-off-by: Wei Dai > Signed-off-by: Ferruh Yigit > Signed-off-by: Qi Zhang > > --- > v10: > sorry, miss the code change, fix the buidling error > > v9: > replace RTE_PMD_DEBUG_TRACE with ethdev_log(ERR, in ethdev > to avoid failure of application which hasn't been completely > converted to new offload API. [...] > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index e560524..5baa2aa 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -1139,6 +1139,28 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, > ETHER_MAX_LEN; > } > > + /* Any requested offloading must be within its device capabilities */ > + if ((local_conf.rxmode.offloads & dev_info.rx_offload_capa) != > + local_conf.rxmode.offloads) { > + ethdev_log(ERR, "ethdev port_id=%d requested Rx offloads " > + "0x%" PRIx64 " doesn't match Rx offloads " > + "capabilities 0x%" PRIx64 " in %s( )\n", > + port_id, > + local_conf.rxmode.offloads, > + dev_info.rx_offload_capa, > + __func__); Why is return -EINVAL removed here? If application is not updated to use offloads, offloads is 0 and everything is OK. If application is updated to use offloads, its behaviour must be consistent. Same below for Tx device offloads. > + } > + if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) != > + local_conf.txmode.offloads) { > + ethdev_log(ERR, "ethdev port_id=%d requested Tx offloads " > + "0x%" PRIx64 " doesn't match Tx offloads " > + "capabilities 0x%" PRIx64 " in %s( )\n", > + port_id, > + local_conf.txmode.offloads, > + dev_info.tx_offload_capa, > + __func__); > + } > + > /* Check that device supports requested rss hash functions. */ > if ((dev_info.flow_type_rss_offloads | > dev_conf->rx_adv_conf.rss_conf.rss_hf) != > @@ -1504,6 +1526,38 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id, > &local_conf.offloads); > } > > + /* > + * If an offloading has already been enabled in > + * rte_eth_dev_configure(), it has been enabled on all queues, > + * so there is no need to enable it in this queue again. > + * The local_conf.offloads input to underlying PMD only carries > + * those offloadings which are only enabled on this queue and > + * not enabled on all queues. > + * The underlying PMD must be aware of this point. > + */ > + local_conf.offloads &= ~dev->data->dev_conf.rxmode.offloads; > + > + /* > + * New added offloadings for this queue are those not enabled in > + * rte_eth_dev_configure( ) and they must be per-queue type. > + * A pure per-port offloading can't be enabled on a queue while > + * disabled on another queue. A pure per-port offloading can't > + * be enabled for any queue as new added one if it hasn't been > + * enabled in rte_eth_dev_configure( ). > + */ > + if ((local_conf.offloads & dev_info.rx_queue_offload_capa) != > + local_conf.offloads) { > + ethdev_log(ERR, "Ethdev port_id=%d rx_queue_id=%d, new " > + "added offloads 0x%" PRIx64 " must be " > + "within pre-queue offload capabilities 0x%" > + PRIx64 " in %s( )\n", > + port_id, > + rx_queue_id, > + local_conf.offloads, > + dev_info.rx_queue_offload_capa, > + __func__); May be it is really a good tradeoff to remove error return here. Ideally it would be nice to see explanation here why. > + } > + > ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc, > socket_id, &local_conf, mp); > if (!ret) { > @@ -1612,6 +1666,38 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id, > &local_conf.offloads); > } > > + /* > + * If an offloading has already been enabled in > + * rte_eth_dev_configure(), it has been enabled on all queues, > + * so there is no need to enable it in this queue again. > + * The local_conf.offloads input to underlying PMD only carries > + * those offloadings which are only enabled on this queue and > + * not enabled on all queues. > + * The underlying PMD must be aware of this point. > + */ > + local_conf.offloads &= ~dev->data->dev_conf.txmode.offloads; > + > + /* > + * New added offloadings for this queue are those not enabled in > + * rte_eth_dev_configure( ) and they must be per-queue type. > + * A pure per-port offloading can't be enabled on a queue while > + * disabled on another queue. A pure per-port offloading can't > + * be enabled for any queue as new added one if it hasn't been > + * enabled in rte_eth_dev_configure( ). > + */ > + if ((local_conf.offloads & dev_info.tx_queue_offload_capa) != > + local_conf.offloads) { > + ethdev_log(ERR, "Ethdev port_id=%d tx_queue_id=%d, new " > + "added offloads 0x%" PRIx64 " must be " > + "within pre-queue offload capabilities 0x%" > + PRIx64 " in %s( )\n", > + port_id, > + tx_queue_id, > + local_conf.offloads, > + dev_info.tx_queue_offload_capa, > + __func__); > + } > + > return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev, > tx_queue_id, nb_tx_desc, socket_id, &local_conf)); > }