From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0081.outbound.protection.outlook.com [104.47.0.81]) by dpdk.org (Postfix) with ESMTP id C48DF2BD5 for ; Sat, 5 May 2018 20:59:44 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=Sny6YQJ4iGpXWwOhKttuWrXT/EEjjMPON8g+jY/+3NI=; b=cwvGAfn49mmpmQepZ21b4PlkEpxzRD3HTUQlysvZPsYsRTNt37xVKfzKmVaoD5srPmGM7gadgG5mVBEpplrfHvjukea6vfI34Vta17DVHmMtHnPnBDBwnFwBfNb74FAHfFM3VlbS5RbWPBDV3xtdNSejh3Ol60E8aHjf+EN4SNw= Received: from DB7PR05MB4426.eurprd05.prod.outlook.com (52.134.109.15) by DB7PR05MB4492.eurprd05.prod.outlook.com (52.134.109.138) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.735.17; Sat, 5 May 2018 18:59:42 +0000 Received: from DB7PR05MB4426.eurprd05.prod.outlook.com ([fe80::f116:5be4:ba29:fed8]) by DB7PR05MB4426.eurprd05.prod.outlook.com ([fe80::f116:5be4:ba29:fed8%13]) with mapi id 15.20.0735.018; Sat, 5 May 2018 18:59:42 +0000 From: Shahaf Shuler To: Wei Dai , Thomas Monjalon , "ferruh.yigit@intel.com" CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v7] ethdev: check Rx/Tx offloads Thread-Index: AQHT47M7iIMrmISYu0mpn2xCT4zf5KQheLdw Date: Sat, 5 May 2018 18:59:42 +0000 Message-ID: References: <1525311040-26694-1-git-send-email-wei.dai@intel.com> <1525442529-12723-1-git-send-email-wei.dai@intel.com> In-Reply-To: <1525442529-12723-1-git-send-email-wei.dai@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=shahafs@mellanox.com; x-originating-ip: [31.154.10.107] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB7PR05MB4492; 7:QVMXY1cIZQLnjKq8MSab4sawM1giysZUIMZKeXGgS9/i8IatfDsAEM5Y+N3U6B+4baQqE+QRXfhJYFeMSeSvdOcoO3OcWYlur1bUsfxpQ41rlm+4/+u/9e6MZC5MuAL9GIDEgIp23WV4DZ2wufKMcErGHePjqqLca4zFqWpEdwLFilim0NyMHbvRUwpJj8xwJyxof4lcD1CNM2FlWTB+txaMWvdcXPfdCblUZta5D1EeFGyGKOd1iRkKcbyJiBQx x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(5600026)(48565401081)(4534165)(4627221)(201703031133081)(201702281549075)(2017052603328)(7153060)(7193020); SRVR:DB7PR05MB4492; x-ms-traffictypediagnostic: DB7PR05MB4492: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(3002001)(10201501046)(3231254)(944501410)(52105095)(93006095)(93001095)(6055026)(6041310)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123564045)(20161123560045)(20161123558120)(6072148)(201708071742011); SRVR:DB7PR05MB4492; BCL:0; PCL:0; RULEID:; SRVR:DB7PR05MB4492; x-forefront-prvs: 0663390E1B x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(396003)(366004)(39380400002)(39860400002)(376002)(346002)(199004)(189003)(105586002)(2906002)(68736007)(33656002)(7736002)(25786009)(229853002)(6246003)(106356001)(81156014)(110136005)(53376002)(81166006)(55016002)(53936002)(5660300001)(316002)(966005)(3280700002)(14454004)(3660700001)(8936002)(6306002)(9686003)(8676002)(478600001)(486006)(26005)(76176011)(5250100002)(86362001)(2900100001)(186003)(446003)(4326008)(11346002)(305945005)(6116002)(6436002)(3846002)(66066001)(59450400001)(2501003)(97736004)(74316002)(6506007)(99286004)(102836004)(7696005)(476003); DIR:OUT; SFP:1101; SCL:1; SRVR:DB7PR05MB4492; H:DB7PR05MB4426.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: nv5P2UclIzp4plQ0jq8lMYfnFgo+jt4OV0d9qxd2lx9Mpr1mv5/1+S65D8RGt4UIYR/GXfbfEHL0EW9Lon+yNhd5JzOV65SOdwL1yjp70E4LLvoSqKIGLkOTiYGG6d/gSYMlMVbBrgqg/VCjwPvnZcU0MDZoUml2ZqYQIBEiaBSfu9AundUwME6hvEE5nRNP spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: 232f8459-a41f-4c57-14f7-08d5b2ba5c77 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 232f8459-a41f-4c57-14f7-08d5b2ba5c77 X-MS-Exchange-CrossTenant-originalarrivaltime: 05 May 2018 18:59:42.6981 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB7PR05MB4492 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: Sat, 05 May 2018 18:59:45 -0000 Hi Ferruh, Dai, > Subject: [dpdk-dev] [PATCH v7] ethdev: check Rx/Tx offloads >=20 > 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 queu= e 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 enab= led > for the queue but not enabled in rte_eth_dev_configure( ) and they are > certain per-queue type. >=20 > This patch can make above such checking in a common way in rte_ethdev > layer to avoid same checking in underlying PMD. >=20 > Signed-off-by: Wei Dai > Signed-off-by: Ferruh Yigit >=20 > --- > 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 underlyin= g > PMD. >=20 > v6: > No need enable an offload in queue_setup( ) if it has already been enable= d > in dev_configure( ) >=20 > v5: > keep offload settings sent to PMD same as those from application >=20 > v4: > fix a wrong description in git log message. >=20 > v3: > rework according to dicision of offloading API in community >=20 > 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(+) >=20 > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethde= v.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; > } >=20 > + /* 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; While I am OK with such behavior, we should be more careful not to get into= the same issue as in [1]. There are PMD which don't report the capabilities correctly however do expe= ct to have the offload configured. 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 r= elease.=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; >=20 > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); >=20 > @@ -1504,6 +1528,68 @@ rte_eth_rx_queue_setup(uint16_t port_id, > uint16_t rx_queue_id, > &local_conf.offloads); > } >=20 > + /* > + * 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; 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 queue = offload capa ERROR... > + 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 Need to re-work this error message. The user doesn't know what are "pure pe= r-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; >=20 > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); >=20 > @@ -1612,6 +1700,68 @@ rte_eth_tx_queue_setup(uint16_t port_id, > uint16_t tx_queue_id, > &local_conf.offloads); > } >=20 > + /* > + * 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; 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 As for Ferruh's comment >=20 > PMDs needs to be updated for: > 1- Remove existing offload verify checks > 2- Update offload configure logic based on new values >=20 > (1) can be part of this patch. But PMD maintainers should send update=20 > for (2) if a change required. > >cc'ed Shahaf, specially for (2) one. I think PMD maintainers can help with that. If it will be integrated enough= time before the release Mellanox PMDs can be converted by us.=20 [1] http://dpdk.org/dev/patchwork/patch/38645/