From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 68EBB2E41 for ; Mon, 7 May 2018 09:15:28 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 May 2018 00:15:06 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,373,1520924400"; d="scan'208";a="43724238" Received: from pgsmsx112.gar.corp.intel.com ([10.108.55.201]) by fmsmga002.fm.intel.com with ESMTP; 07 May 2018 00:15:05 -0700 Received: from pgsmsx111.gar.corp.intel.com ([169.254.2.194]) by PGSMSX112.gar.corp.intel.com ([169.254.3.57]) with mapi id 14.03.0319.002; Mon, 7 May 2018 15:15:03 +0800 From: "Dai, Wei" To: Shahaf Shuler , Thomas Monjalon , "Yigit, Ferruh" CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v7] ethdev: check Rx/Tx offloads Thread-Index: AQHT47M4a64xfQHtSkOL1zAaWgpFzqQg+PAAgALNGwA= Date: Mon, 7 May 2018 07:15:02 +0000 Message-ID: <49759EB36A64CF4892C1AFEC9231E8D66CF7E7DC@PGSMSX111.gar.corp.intel.com> References: <1525311040-26694-1-git-send-email-wei.dai@intel.com> <1525442529-12723-1-git-send-email-wei.dai@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMDY5MTIzZDctNmI2Zi00MTFmLTg1NzEtZDc0MmY0ZTBiMWU5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IjNGODRNUGFQK2xacXZoRzM2TVJJcm1OZXpwVmZBK3JZNkRscUdkWkY5c0E9In0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [172.30.20.206] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v7] ethdev: check Rx/Tx offloads 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: Mon, 07 May 2018 07:15:29 -0000 Thanks to Shuler and Ferruh for your feedback and guidance. PMD at least has these 2 options with this patch: a). If PMD doesn't want to make much more changes, it still can do "[rt]x_c= onf->offloads |=3D dev->data->dev_conf.rxmode.offloads;" in the beginning of its specific queue_setup( ) and just remove offload = checking (although the checking always pass now) and all others keep same. In this way, PMDs still comply with the offload APIs d= efined in 17.11. b). PMD also can use the info that only new added queue-level offloads in t= he input argument [rt]x_conf->offloads to make some optimization or other code changes. It may be more efficient than a). As Ferruh said, only this patch and without relevant change in PMD will cau= se application broken, I will submit v8 patch which will include this patch for ethdev and code ch= anges in PMDs with above option a and document update. I'd like include all these changes in only one patch to avoid application f= ailure if some patches are not applied and some are applied. PMD maintainers call go on with option b)=20 Shuler's suggestion to simplify the new added offloads in queue_setup( ) is= better. I will adopt it in my v8 patch. > -----Original Message----- > From: Shahaf Shuler [mailto:shahafs@mellanox.com] > Sent: Sunday, May 6, 2018 3:00 AM > To: Dai, Wei ; Thomas Monjalon > ; Yigit, Ferruh > Cc: dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v7] ethdev: check Rx/Tx offloads >=20 > Hi Ferruh, Dai, > > Subject: [dpdk-dev] [PATCH v7] ethdev: check Rx/Tx offloads > > > > 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( ). > > From application, a pure per-port offloading can't be enabled on any > > queue if it hasn't been enabled in rte_eth_dev_configure( ). > > 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( ). > > The underlying PMD must be aware that the requested offloadings to PMD > > specific queue_setup( ) function only carries those offloadings only > > enabled for the queue but not enabled in rte_eth_dev_configure( ) and > > they are certain 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. > > > > Signed-off-by: Wei Dai > > Signed-off-by: Ferruh Yigit > > > > --- > > v7: > > Give the maximum freedom for upper application, only minimal checking > > is performed in ethdev layer. > > Only requested specific pure per-queue offloadings are input to > > underlying PMD. > > > > v6: > > No need enable an offload in queue_setup( ) if it has already been > > enabled in dev_configure( ) > > > > v5: > > keep offload settings sent to PMD same as those from application > > > > v4: > > fix a wrong description in git log message. > > > > v3: > > rework according to dicision of offloading API in community > > > > v2: > > add offloads checking in rte_eth_dev_configure( ). > > check if a requested offloading is supported. > > --- > > lib/librte_ethdev/rte_ethdev.c | 150 > > +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 150 insertions(+) > > > > diff --git a/lib/librte_ethdev/rte_ethdev.c > > b/lib/librte_ethdev/rte_ethdev.c index e560524..0ad05eb 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) !=3D > > + local_conf.rxmode.offloads) { > > + RTE_PMD_DEBUG_TRACE("ethdev port_id=3D%d requested Rx > > offloads " > > + "0x%" PRIx64 " doesn't match Rx offloads " > > + "capabilities 0x%" PRIx64 "\n", > > + port_id, > > + local_conf.rxmode.offloads, > > + dev_info.rx_offload_capa); > > + return -EINVAL; >=20 > While I am OK with such behavior, we should be more careful not to get in= to > the same issue as in [1]. > There are PMD which don't report the capabilities correctly however do > expect to have the offload configured. >=20 > All I am saying it is worth a check and cautious decision if it is right = to include > this one w/o prior application notice and at such late RC of the release. >=20 > > + } > > + if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=3D > > + local_conf.txmode.offloads) { > > + RTE_PMD_DEBUG_TRACE("ethdev port_id=3D%d requested Tx > > offloads " > > + "0x%" PRIx64 " doesn't match Tx offloads " > > + "capabilities 0x%" PRIx64 "\n", > > + port_id, > > + local_conf.txmode.offloads, > > + dev_info.tx_offload_capa); > > + return -EINVAL; > > + } > > + > > /* Check that device supports requested rss hash functions. */ > > if ((dev_info.flow_type_rss_offloads | > > dev_conf->rx_adv_conf.rss_conf.rss_hf) !=3D @@ -1414,6 +1436,8 > @@ > > rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id, > > struct rte_eth_dev_info dev_info; > > struct rte_eth_rxconf local_conf; > > void **rxq; > > + uint64_t pure_port_offload_capa; > > + uint64_t only_enabled_for_queue; > > > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); > > > > @@ -1504,6 +1528,68 @@ rte_eth_rx_queue_setup(uint16_t port_id, > > uint16_t rx_queue_id, > > &local_conf.offloads); > > } > > > > + /* > > + * The requested offloadings by application for this queue > > + * can be per-queue type or per-port type. and > > + * they must be within the device offloading capabilities. > > + */ > > + if ((local_conf.offloads & dev_info.rx_offload_capa) !=3D > > + local_conf.offloads) { > > + RTE_PMD_DEBUG_TRACE("Ethdev port_id=3D%d > > rx_queue_id=3D%d " > > + "Requested offload 0x%" PRIx64 "doesn't " > > + "match per-queue capability 0x%" PRIx64 > > + " in %s\n", > > + port_id, > > + rx_queue_id, > > + local_conf.offloads, > > + dev_info.rx_queue_offload_capa, > > + __func__); > > + return -EINVAL; > > + } > > + > > + /* > > + * A pure per-port offloading can't be enabled for any queue > > + * if it hasn't been enabled in rte_eth_dev_configure( ). > > + * > > + * Following pure_port_offload_capa is the capabilities which > > + * can't be enabled on some queue while disabled on other queue. > > + * pure_port_offload_capa must be enabled or disabled on all > > + * queues at same time. > > + * > > + * Following only_enabled_for_queue is the offloadings which > > + * are enabled for this queue but hasn't been enabled in > > + * rte_eth_dev_configure( ). > > + */ > > + pure_port_offload_capa =3D dev_info.rx_offload_capa ^ > > + dev_info.rx_queue_offload_capa; > > + only_enabled_for_queue =3D (local_conf.offloads ^ > > + dev->data->dev_conf.rxmode.offloads) & > > local_conf.offloads; >=20 > It looks like above logic could be a lot simpler. >=20 > How about: > local_conf.offloads &=3D ~dev->data->dev_conf.rxmode.offloads; // keep > only the added offloads on top of the port ones if ((local_conf.offloads = & > dev_info.rx_queue_offload_capa) !=3D > local_conf.offloads) { //check if added offloads are part of the queu= e > offload capa > ERROR... >=20 >=20 > > + if (only_enabled_for_queue & pure_port_offload_capa) { > > + RTE_PMD_DEBUG_TRACE("Ethdev port_id=3D%d > > rx_queue_id=3D%d, only " > > + "enabled offload 0x%" PRIx64 "for this " > > + "queue haven't been enabled in " > > + "dev_configure( ), they are within " > > + "pure per-port capabilities 0x%" PRIx64 >=20 > Need to re-work this error message. The user doesn't know what are "pure > per-port capabilities" >=20 > > + " in %s\n", > > + port_id, > > + rx_queue_id, > > + only_enabled_for_queue, > > + pure_port_offload_capa, > > + __func__); > > + return -EINVAL; > > + } > > + > > + /* > > + * 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 &=3D ~dev->data->dev_conf.rxmode.offloads; > > + > > ret =3D (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc, > > socket_id, &local_conf, mp); > > if (!ret) { > > @@ -1549,6 +1635,8 @@ rte_eth_tx_queue_setup(uint16_t port_id, > > uint16_t tx_queue_id, > > struct rte_eth_dev_info dev_info; > > struct rte_eth_txconf local_conf; > > void **txq; > > + uint64_t pure_port_offload_capa; > > + uint64_t only_enabled_for_queue; > > > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); > > > > @@ -1612,6 +1700,68 @@ rte_eth_tx_queue_setup(uint16_t port_id, > > uint16_t tx_queue_id, > > &local_conf.offloads); > > } > > > > + /* > > + * The requested offloadings by application for this queue > > + * can be per-queue type or per-port type. and > > + * they must be within the device offloading capabilities. > > + */ > > + if ((local_conf.offloads & dev_info.tx_offload_capa) !=3D > > + local_conf.offloads) { > > + RTE_PMD_DEBUG_TRACE("Ethdev port_id=3D%d > > tx_queue_id=3D%d " > > + "Requested offload 0x%" PRIx64 "doesn't " > > + "match per-queue capability 0x%" PRIx64 > > + " in %s\n", > > + port_id, > > + tx_queue_id, > > + local_conf.offloads, > > + dev_info.tx_queue_offload_capa, > > + __func__); > > + return -EINVAL; > > + } > > + > > + /* > > + * A pure per-port offloading can't be enabled for any queue > > + * if it hasn't been enabled in rte_eth_dev_configure( ). > > + * > > + * Following pure_port_offload_capa is the capabilities which > > + * can't be enabled on some queue while disabled on other queue. > > + * pure_port_offload_capa must be enabled or disabled on all > > + * queues at same time. > > + * > > + * Following only_enabled_for_queue is the offloadings which > > + * are enabled for this queue but hasn't been enabled in > > + * rte_eth_dev_configure( ). > > + */ > > + pure_port_offload_capa =3D dev_info.tx_offload_capa ^ > > + dev_info.tx_queue_offload_capa; > > + only_enabled_for_queue =3D (local_conf.offloads ^ > > + dev->data->dev_conf.txmode.offloads) & > > local_conf.offloads; >=20 > Same comments as in the Rx part. >=20 > > + if (only_enabled_for_queue & pure_port_offload_capa) { > > + RTE_PMD_DEBUG_TRACE("Ethdev port_id=3D%d > > tx_queue_id=3D%d, only " > > + "enabled offload 0x%" PRIx64 "for this " > > + "queue haven't been enabled in " > > + "dev_configure( ), they are within " > > + "pure per-port capabilities 0x%" PRIx64 > > + " in %s\n", > > + port_id, > > + tx_queue_id, > > + only_enabled_for_queue, > > + pure_port_offload_capa, > > + __func__); > > + return -EINVAL; > > + } > > + > > + /* > > + * 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 &=3D ~dev->data->dev_conf.txmode.offloads; > > + > > return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev, > > tx_queue_id, nb_tx_desc, socket_id, &local_conf)); } > > -- > > 2.7.5 >=20 >=20 > As for Ferruh's comment > > > > PMDs needs to be updated for: > > 1- Remove existing offload verify checks > > 2- Update offload configure logic based on new values > > > > (1) can be part of this patch. But PMD maintainers should send update > > for (2) if a change required. > > > >cc'ed Shahaf, specially for (2) one. >=20 > I think PMD maintainers can help with that. If it will be integrated enou= gh > time before the release Mellanox PMDs can be converted by us. >=20 >=20 >=20 >=20 > [1] > http://dpdk.org/dev/patchwork/patch/38645/ >=20