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 85D2FA046B for ; Fri, 23 Aug 2019 11:45:16 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6AD5D1BFA9; Fri, 23 Aug 2019 11:45:16 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 5DBCC1BFA1 for ; Fri, 23 Aug 2019 11:45:15 +0200 (CEST) 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-us2.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 8B6A2A4005F; Fri, 23 Aug 2019 09:45:12 +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; Fri, 23 Aug 2019 10:45:05 +0100 To: , , John McNamara , Marko Kovacevic , Thomas Monjalon , Ferruh Yigit CC: References: <20190821204755.1990-1-pbhagavatula@marvell.com> <20190821204755.1990-2-pbhagavatula@marvell.com> From: Andrew Rybchenko Message-ID: <65845828-2bca-c9c9-c542-56c65ab332a2@solarflare.com> Date: Fri, 23 Aug 2019 12:44:16 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190821204755.1990-2-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-24864.003 X-TM-AS-Result: No-12.577100-8.000000-10 X-TMASE-MatchedRID: 0+daXaNUWRXmLzc6AOD8DfHkpkyUphL9NV9S7O+u3KYRHgO3hlQeWerO SEJRBEaXSMM+7USIvyxMTh0cRVMQ694V9K8RueK0A9lly13c/gGNkCJU7Ne1c8K0HkBcZFjb2OS V0vdl9lfYZnvq7ZkJR8f6aQTAn4fDwrMHbpAUmLnD0ZWEZr/ntpyqUJ2uHKFAq2RzHFToRUjcru +PSMhej3xs7qFZ9FbOcuFD4pHUjrhdwKvR2QLJ9jIHIyLCTr7e+ma2kEuhRFogUEQTkIWiYt3NN JQDizxebwH3BifLn0XE1QEMPJcqu/dlftnKvkeUWcC0gYMYIWj+paX6bXuNYeZMicrOlIVJsx8U MJr4pncY2ikX1Snq/H3sb3WCJD2UyXrQ52kRxfQylU6xjA3vwzal66Y6k2O0DMTHsGw8KHem2AO 8c0o94qnctLHiR9ZHegVfIaZBNk0UwVRCL6lqYczSKGx9g8xhyeUl7aCTy8jojlgCmUQFYSi6Eg RvqYvi034XK+JcgUkyMJ4ANEGmEMlcp2KsixOJ6GyDR2ZB+cZXLrDwmXJ6b4lMNXlxO/FToKqda gw8NC70lKD+/w5gT/KaVf9i/sZPZhbYS3J8SRZfYa9W9Ojitam9/6ObPjnDVptCZRwLvQHlHzYT kjdrvJswYa3m4JlusAEbWSzut2MYOEQHAQk2uZ4CIKY/Hg3AWQy9YC5qGvzcyHXbbFd0K3g3WOF FVAUtIAcCikR3vq80LxjKo64I2qDWFL8WEwHbynSqrBxUDnLHGxWm4w/zpSd5LWc4vAN4 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--12.577100-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24864.003 X-MDID: 1566553513-rEmqKxQMx3Ob Subject: Re: [dpdk-dev] [PATCH v2 1/7] ethdev: add set ptype function 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 8/21/19 11:47 PM, pbhagavatula@marvell.com wrote: > From: Pavan Nikhilesh > > Add `rte_eth_dev_set_supported_ptypes` function that will allow the > application to inform the PMD the packet types it is interested in. > Based on the ptypes set PMDs can optimize their Rx path. > > -If application doesn’t want any ptype information it can call > `rte_eth_dev_set_supported_ptypes(ethdev_id, RTE_PTYPE_UNKNOWN)` and PMD > will set rte_mbuf::packet_type to 0. I would say that "... and PMD may skip packet type processing and just set rte_mbuf::packet_type to RTE_PTYPE_UNKNOWN." I.e. PMD is not obligated to skip packet type processing, it just has permission to do it. > -If application doesn’t call `rte_eth_dev_set_supported_ptypes` PMD can > return `rte_mbuf::packet_type` with `rte_eth_dev_get_supported_ptypes`. > > -If application is interested only in L2/L3 layer, it can inform the PMD > to update `rte_mbuf::packet_type` with L2/L3 ptype by calling > `rte_eth_dev_set_supported_ptypes(ethdev_id, > RTE_PTYPE_L2_MASK | RTE_PTYPE_L3_MASK)`. > > Suggested-by: Konstantin Ananyev > Signed-off-by: Pavan Nikhilesh > --- > doc/guides/nics/features.rst | 12 ++++++--- > doc/guides/rel_notes/release_19_11.rst | 7 ++++++ > lib/librte_ethdev/rte_ethdev.c | 32 ++++++++++++++++++++++++ > lib/librte_ethdev/rte_ethdev.h | 16 ++++++++++++ > lib/librte_ethdev/rte_ethdev_core.h | 6 +++++ > lib/librte_ethdev/rte_ethdev_version.map | 3 +++ > 6 files changed, 72 insertions(+), 4 deletions(-) > > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst > index c4e128d2f..d4d55f721 100644 > --- a/doc/guides/nics/features.rst > +++ b/doc/guides/nics/features.rst > @@ -582,10 +582,14 @@ Supports inner packet L4 checksum. > Packet type parsing > ------------------- > > -Supports packet type parsing and returns a list of supported types. > - > -* **[implements] eth_dev_ops**: ``dev_supported_ptypes_get``. > -* **[related] API**: ``rte_eth_dev_get_supported_ptypes()``. > +Supports packet type parsing and returns a list of supported types. Allows > +application to set ptypes it is interested in. It is better to start new sentence from new line. It will keep the first line untouched. > + > +* **[implements] eth_dev_ops**: ``dev_supported_ptypes_get``, > + ``dev_supported_ptypes_set``. > +* **[related] API**: ``rte_eth_dev_get_supported_ptypes()``, > + ``rte_eth_dev_set_supported_ptypes()``. > +* **[provides] mbuf**: ``mbuf.packet_type``. > > > .. _nic_features_timesync: > diff --git a/doc/guides/rel_notes/release_19_11.rst b/doc/guides/rel_notes/release_19_11.rst > index 8490d897c..a7cec1fe8 100644 > --- a/doc/guides/rel_notes/release_19_11.rst > +++ b/doc/guides/rel_notes/release_19_11.rst > @@ -56,6 +56,13 @@ New Features > Also, make sure to start the actual text at the margin. > ========================================================= > > +* **Added API in ethdev to set supported packet types** May I suggest: Added ethdev API to set supported packet types. > + > + * Added new API ``rte_eth_dev_set_supported_ptypes`` that allows an > + application to request PMD to set specific ptypes defined > + through ``rte_eth_dev_set_supported_ptypes`` in ``rte_mbuf::packet_type``. Consider: Added new API ``rte_eth_dev_set_supported_ptypes`` that allows an application to inform PMD about packet types classification the application is interested in. > + * This scheme will allow PMDs to avoid lookup to internal ptype table on Rx > + and thereby improve Rx performance if application wishes do so. > > Removed Items > ------------- > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 17d183e1f..f529cbe9f 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -2602,6 +2602,38 @@ rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask, > return j; > } > > +uint32_t > +rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t ptype_mask) > +{ > + int i; > + struct rte_eth_dev *dev; > + const uint32_t *all_ptypes; > + uint32_t all_ptype_mask = 0; > + uint32_t supp_ptype_mask = 0; > + > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); We should either return 0 or change prototype to return successfully set ptypes in optional (may be NULL) output parameter. I would change the prototype. > + dev = &rte_eth_devices[port_id]; > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0); Since API is not mandatory and provides just hits for optimizations, I think it should return error in really unexpected cases only. It looks like invalid port_id is the only case. > + if (ptype_mask == 0) { > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_set, > + 0); > + return (*dev->dev_ops->dev_supported_ptypes_set)(dev, > + ptype_mask); > + } > + > + all_ptypes = (*dev->dev_ops->dev_supported_ptypes_get)(dev); > + if (all_ptypes == NULL) > + return 0; > + > + for (i = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; ++i) > + all_ptype_mask |= all_ptypes[i]; > + > + supp_ptype_mask = all_ptype_mask & ptype_mask; I think it is not that useful to do here. Moreover, it is invalid in a way how it is implemented right now. E.g. if get returns: RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_TCP RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_UDP RTE_PTYPE_L2_ARP and mask is RTE_PTYPE_L2_MASK | RTE_PTYPE_L3_MASK | RTE_PTYPE_L4_MASK the result will be: RTE_PTYPE_L2_ARP | RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_FRAG > + return (*dev->dev_ops->dev_supported_ptypes_set)(dev, supp_ptype_mask); It is not checked that  dev_supported_ptypes_set is not NULL. > +} > + > void > rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr) > { > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h > index dc6596bc9..1ab0af4d8 100644 > --- a/lib/librte_ethdev/rte_ethdev.h > +++ b/lib/librte_ethdev/rte_ethdev.h > @@ -2431,6 +2431,22 @@ int rte_eth_dev_fw_version_get(uint16_t port_id, > */ > int rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask, > uint32_t *ptypes, int num); > +/** Missing:  * @warning  * @b EXPERIMENTAL: this API may change without prior notice. > + * Request Ethernet device to set only specific packet types in the packet. Request sounds mandatory. Consider: Inform Ethernet device of the packet types classification in which the recipient is interested. > + * > + * Application can use this function to set only specific ptypes that it's > + * interested. This information can be used by the PMD to optimize Rx path. > + * > + * @param port_id > + * The port identifier of the Ethernet device. > + * @param ptype_mask > + * The ptype family that application is interested in. It should be highligted that for each layer it should be either full mask (e.g. RTE_PTYPE_L4_MASK) or only one specific type (e.g. RTE_PTYPE_L4_TCP) since otherwise caller may try RTE_PTYPE_L4_TCP|RTE_PTYPE_L4_UDP which is RTE_PTYPE_L4_FRAG in fact. > + * @return > + * - (>=0) Ptype mask that has been set successfully. uint32_t is always >=0, so I think (>=0) should be removed. However, in fact I think it should be: Negative errno value on error, 0 on success. I think that "Ptype mask that has been set successfully" could be misleading since for each layer it is not a bit mark, but enum. The output should be supported_ptype_get-slyle. I.e. an array which is terminated by RTE_PTYPE_UNKNOWN entry. > + */ > +__rte_experimental > +uint32_t rte_eth_dev_set_supported_ptypes(uint16_t port_id, > + uint32_t ptype_mask); > > /** > * Retrieve the MTU of an Ethernet device. > diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h > index 2922d5b7c..e65ae983d 100644 > --- a/lib/librte_ethdev/rte_ethdev_core.h > +++ b/lib/librte_ethdev/rte_ethdev_core.h > @@ -110,6 +110,10 @@ typedef void (*eth_dev_infos_get_t)(struct rte_eth_dev *dev, > typedef const uint32_t *(*eth_dev_supported_ptypes_get_t)(struct rte_eth_dev *dev); > /**< @internal Get supported ptypes of an Ethernet device. */ > > +typedef uint32_t (*eth_dev_supported_ptypes_set_t)(struct rte_eth_dev *dev, > + uint32_t ptype_mask); > +/**< @internal Set required ptypes of an Ethernet device. */ Consider: Inform device about packet types in which the recipient is interested. > + > typedef int (*eth_queue_start_t)(struct rte_eth_dev *dev, > uint16_t queue_id); > /**< @internal Start rx and tx of a queue of an Ethernet device. */ > @@ -421,6 +425,8 @@ struct eth_dev_ops { > eth_fw_version_get_t fw_version_get; /**< Get firmware version. */ > eth_dev_supported_ptypes_get_t dev_supported_ptypes_get; > /**< Get packet types supported and identified by device. */ > + eth_dev_supported_ptypes_set_t dev_supported_ptypes_set; > + /**< Inform device about the interested ptypes. */ Consider Inform device about packet types in which the recipient is interested. > > vlan_filter_set_t vlan_filter_set; /**< Filter VLAN Setup. */ > vlan_tpid_set_t vlan_tpid_set; /**< Outer/Inner VLAN TPID Setup. */