| Message ID | 20191002034716.6842-2-pbhagavatula@marvell.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Ferruh Yigit |
| Headers | show |
| Series | [v7,1/7] ethdev: add set ptype function | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | success | Compilation OK |
| ci/iol-compilation | success | Compile Testing PASS |
| ci/iol-intel-Performance | fail | Performance Testing issues |
| ci/iol-mellanox-Performance | success | Performance Testing PASS |
Hi, looks good, just few comments below. Many thanks for working on it, Andrew. On 10/2/19 6:47 AM, pbhagavatula@marvell.com wrote: > From: Pavan Nikhilesh <pbhagavatula@marvell.com> > > 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, NULL, 0)` > and PMD may skip packet type processing and set rte_mbuf::packet_type to > RTE_PTYPE_UNKNOWN. > > -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, NULL, 0)`. > > Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com> [snip] > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 17d183e1f..b1588fe7a 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -2602,6 +2602,56 @@ rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask, > return j; > } > > +int > +rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t ptype_mask, > + uint32_t *set_ptypes, unsigned int num) > +{ > + unsigned int i, j; > + struct rte_eth_dev *dev; > + const uint32_t *all_ptypes; > + > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > + dev = &rte_eth_devices[port_id]; > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0); > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_set, 0); When 0 is returned above, we should set set_ptypes. So, below check num vs set_types should be done before callbacks check and I think it is OK to do set_ptypes[0] = RTE_PTYPE_UNKNOWN; just after the check. It will allow to simplify code below and will make it set correctly even if 0 returned because of no callbacks. > + > + if (num > 0 && set_ptypes == NULL) > + return -EINVAL; > + > + if (ptype_mask == 0) { > + if (num > 0) > + set_ptypes[0] = RTE_PTYPE_UNKNOWN; > + > + 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) { > + if (num > 0) > + set_ptypes[0] = RTE_PTYPE_UNKNOWN; > + > + return 0; > + } > + > + for (i = 0, j = 0; set_ptypes != NULL && > + (all_ptypes[i] != RTE_PTYPE_UNKNOWN); ++i) { > + if (ptype_mask & all_ptypes[i]) { > + if (j < num - 1) { > + set_ptypes[j] = all_ptypes[i]; > + j++; > + continue; > + } > + break; I'd like to repeat my question about insufficient space to return set_ptypes. Do we need to signal it somehow? If no, it should be explained why in the comments here. > + } > + } > + > + if (set_ptypes != NULL) > + set_ptypes[j] = RTE_PTYPE_UNKNOWN; > + > + return (*dev->dev_ops->dev_supported_ptypes_set)(dev, ptype_mask); > +} > + > 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 d9871782e..c577a9172 100644 > --- a/lib/librte_ethdev/rte_ethdev.h > +++ b/lib/librte_ethdev/rte_ethdev.h > @@ -2431,6 +2431,42 @@ 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); > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * 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. > + * > + * The function accepts an array `set_ptypes` allocated by the caller to > + * store the packet types set by the driver, the last element of the array > + * is set to RTE_PTYPE_UNKNOWN. The size of the `set_ptype` array should be > + * `rte_eth_dev_get_supported_ptypes() + 1` else it might only be filled > + * partially. > + * > + * @param port_id > + * The port identifier of the Ethernet device. > + * @param ptype_mask > + * The ptype family that application is interested in. > + * @param set_ptypes > + * An array pointer to store set packet types, allocated by caller. The > + * function marks the end of array with RTE_PTYPE_UNKNOWN. > + * @param num > + * Size of the array pointed by param ptypes. > + * Should be rte_eth_dev_get_supported_ptypes() + 1 to accommodate the > + * set ptypes. > + * @return > + * - (0) if Success. > + * - (-ENODEV) if *port_id* invalid. > + * - (-EINVAL) if *ptype_mask* is invalid (or) set_ptypes is NULL and > + * num > 0. > + */ > +__rte_experimental > +int rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t ptype_mask, > + uint32_t *set_ptypes, unsigned int num); > > /** > * 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..93bc34480 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 Inform device about packet types in which the recipient is interested. */ > + Please, take a look at promiscuous mode callback and let's put more verbose description here which better document interface to drivers. I think ptyep_mask description should refer to corresponding defines to be used. [snip]
On 10/2/19 4:37 PM, Andrew Rybchenko wrote: > Hi, > > looks good, just few comments below. > > Many thanks for working on it, > Andrew. > > On 10/2/19 6:47 AM, pbhagavatula@marvell.com wrote: >> From: Pavan Nikhilesh <pbhagavatula@marvell.com> >> >> 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, NULL, >> 0)` >> and PMD may skip packet type processing and set rte_mbuf::packet_type to >> RTE_PTYPE_UNKNOWN. >> >> -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, NULL, 0)`. >> >> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com> >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com> > > [snip] > >> diff --git a/lib/librte_ethdev/rte_ethdev.c >> b/lib/librte_ethdev/rte_ethdev.c >> index 17d183e1f..b1588fe7a 100644 >> --- a/lib/librte_ethdev/rte_ethdev.c >> +++ b/lib/librte_ethdev/rte_ethdev.c >> @@ -2602,6 +2602,56 @@ rte_eth_dev_get_supported_ptypes(uint16_t >> port_id, uint32_t ptype_mask, >> return j; >> } >> +int >> +rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t ptype_mask, >> + uint32_t *set_ptypes, unsigned int num) >> +{ >> + unsigned int i, j; >> + struct rte_eth_dev *dev; >> + const uint32_t *all_ptypes; >> + >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> + dev = &rte_eth_devices[port_id]; >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0); >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_set, 0); > > When 0 is returned above, we should set set_ptypes. So, below > check num vs set_types should be done before callbacks check and > I think it is OK to do > set_ptypes[0] = RTE_PTYPE_UNKNOWN; of course if num > 0 > just after the check. It will allow to simplify code below and will > make it set correctly even if 0 returned because of no callbacks. > >> + >> + if (num > 0 && set_ptypes == NULL) >> + return -EINVAL; >> + >> + if (ptype_mask == 0) { >> + if (num > 0) >> + set_ptypes[0] = RTE_PTYPE_UNKNOWN; >> + >> + 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) { >> + if (num > 0) >> + set_ptypes[0] = RTE_PTYPE_UNKNOWN; >> + >> + return 0; >> + } >> + >> + for (i = 0, j = 0; set_ptypes != NULL && >> + (all_ptypes[i] != RTE_PTYPE_UNKNOWN); ++i) { >> + if (ptype_mask & all_ptypes[i]) { >> + if (j < num - 1) { >> + set_ptypes[j] = all_ptypes[i]; >> + j++; >> + continue; >> + } >> + break; > > I'd like to repeat my question about insufficient space to return > set_ptypes. Do we need to signal it somehow? If no, it should be > explained why in the comments here. > >> + } >> + } >> + >> + if (set_ptypes != NULL) >> + set_ptypes[j] = RTE_PTYPE_UNKNOWN; >> + >> + return (*dev->dev_ops->dev_supported_ptypes_set)(dev, ptype_mask); >> +} >> + >> 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 d9871782e..c577a9172 100644 >> --- a/lib/librte_ethdev/rte_ethdev.h >> +++ b/lib/librte_ethdev/rte_ethdev.h >> @@ -2431,6 +2431,42 @@ 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); >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice. >> + * >> + * 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. >> + * >> + * The function accepts an array `set_ptypes` allocated by the >> caller to >> + * store the packet types set by the driver, the last element of the >> array >> + * is set to RTE_PTYPE_UNKNOWN. The size of the `set_ptype` array >> should be >> + * `rte_eth_dev_get_supported_ptypes() + 1` else it might only be >> filled >> + * partially. >> + * >> + * @param port_id >> + * The port identifier of the Ethernet device. >> + * @param ptype_mask >> + * The ptype family that application is interested in. >> + * @param set_ptypes >> + * An array pointer to store set packet types, allocated by >> caller. The >> + * function marks the end of array with RTE_PTYPE_UNKNOWN. >> + * @param num >> + * Size of the array pointed by param ptypes. >> + * Should be rte_eth_dev_get_supported_ptypes() + 1 to accommodate >> the >> + * set ptypes. >> + * @return >> + * - (0) if Success. >> + * - (-ENODEV) if *port_id* invalid. >> + * - (-EINVAL) if *ptype_mask* is invalid (or) set_ptypes is NULL and >> + * num > 0. >> + */ >> +__rte_experimental >> +int rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t >> ptype_mask, >> + uint32_t *set_ptypes, unsigned int num); >> /** >> * 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..93bc34480 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 Inform device about packet types in which the >> recipient is interested. */ >> + > > Please, take a look at promiscuous mode callback and let's put > more verbose description here which better document interface > to drivers. I think ptyep_mask description should refer to corresponding > defines to be used. > > [snip] >
diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst index c4e128d2f..1756fa73a 100644 --- a/doc/guides/nics/features.rst +++ b/doc/guides/nics/features.rst @@ -583,9 +583,13 @@ Packet type parsing ------------------- Supports packet type parsing and returns a list of supported types. +Allows application to set ptypes it is interested in. -* **[implements] eth_dev_ops**: ``dev_supported_ptypes_get``. -* **[related] API**: ``rte_eth_dev_get_supported_ptypes()``. +* **[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 27cfbd9e3..abb7b6529 100644 --- a/doc/guides/rel_notes/release_19_11.rst +++ b/doc/guides/rel_notes/release_19_11.rst @@ -56,6 +56,14 @@ New Features Also, make sure to start the actual text at the margin. ========================================================= +* **Added ethdev API to set supported packet types** + + * 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..b1588fe7a 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -2602,6 +2602,56 @@ rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask, return j; } +int +rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t ptype_mask, + uint32_t *set_ptypes, unsigned int num) +{ + unsigned int i, j; + struct rte_eth_dev *dev; + const uint32_t *all_ptypes; + + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); + dev = &rte_eth_devices[port_id]; + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0); + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_set, 0); + + if (num > 0 && set_ptypes == NULL) + return -EINVAL; + + if (ptype_mask == 0) { + if (num > 0) + set_ptypes[0] = RTE_PTYPE_UNKNOWN; + + 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) { + if (num > 0) + set_ptypes[0] = RTE_PTYPE_UNKNOWN; + + return 0; + } + + for (i = 0, j = 0; set_ptypes != NULL && + (all_ptypes[i] != RTE_PTYPE_UNKNOWN); ++i) { + if (ptype_mask & all_ptypes[i]) { + if (j < num - 1) { + set_ptypes[j] = all_ptypes[i]; + j++; + continue; + } + break; + } + } + + if (set_ptypes != NULL) + set_ptypes[j] = RTE_PTYPE_UNKNOWN; + + return (*dev->dev_ops->dev_supported_ptypes_set)(dev, ptype_mask); +} + 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 d9871782e..c577a9172 100644 --- a/lib/librte_ethdev/rte_ethdev.h +++ b/lib/librte_ethdev/rte_ethdev.h @@ -2431,6 +2431,42 @@ 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); +/** + * @warning + * @b EXPERIMENTAL: this API may change without prior notice. + * + * 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. + * + * The function accepts an array `set_ptypes` allocated by the caller to + * store the packet types set by the driver, the last element of the array + * is set to RTE_PTYPE_UNKNOWN. The size of the `set_ptype` array should be + * `rte_eth_dev_get_supported_ptypes() + 1` else it might only be filled + * partially. + * + * @param port_id + * The port identifier of the Ethernet device. + * @param ptype_mask + * The ptype family that application is interested in. + * @param set_ptypes + * An array pointer to store set packet types, allocated by caller. The + * function marks the end of array with RTE_PTYPE_UNKNOWN. + * @param num + * Size of the array pointed by param ptypes. + * Should be rte_eth_dev_get_supported_ptypes() + 1 to accommodate the + * set ptypes. + * @return + * - (0) if Success. + * - (-ENODEV) if *port_id* invalid. + * - (-EINVAL) if *ptype_mask* is invalid (or) set_ptypes is NULL and + * num > 0. + */ +__rte_experimental +int rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t ptype_mask, + uint32_t *set_ptypes, unsigned int num); /** * 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..93bc34480 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 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 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. */ diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map index 6df42a47b..e14745b9c 100644 --- a/lib/librte_ethdev/rte_ethdev_version.map +++ b/lib/librte_ethdev/rte_ethdev_version.map @@ -283,4 +283,7 @@ EXPERIMENTAL { # added in 19.08 rte_eth_read_clock; + + # added in 19.11 + rte_eth_dev_set_supported_ptypes; };