Re: [dpdk-dev] [PATCH v16 1/8] ethdev: add set ptype function

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 11/8/19 2:05 PM, Ferruh Yigit wrote:
> On 11/8/2019 10:24 AM, Andrew Rybchenko wrote:
>> On 11/8/19 12:52 PM, Ferruh Yigit wrote:
>>> On 11/8/2019 9:05 AM, Andrew Rybchenko wrote:
>>>> On 11/7/19 9:28 PM, Ferruh Yigit wrote:
>>>>> On 11/6/2019 7:17 PM, pbhagavatula@marvell.com wrote:
>>>>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>>>>
>>>>>> Add `rte_eth_dev_set_ptypes` function that will allow the application
>>>>>> to inform the PMD about reduced range of packet types to handle.
>>>>>> 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_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_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_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>
>>>>>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>> ---
>>>>>>  doc/guides/nics/features.rst             |  7 +-
>>>>>>  doc/guides/rel_notes/release_19_11.rst   |  8 +++
>>>>>>  lib/librte_ethdev/rte_ethdev.c           | 87 +++++++++++++++++++++++-
>>>>>>  lib/librte_ethdev/rte_ethdev.h           | 36 ++++++++++
>>>>>>  lib/librte_ethdev/rte_ethdev_core.h      | 18 +++++
>>>>>>  lib/librte_ethdev/rte_ethdev_version.map |  1 +
>>>>>>  6 files changed, 154 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
>>>>>> index 7a31cf7c8..f33e8e6d4 100644
>>>>>> --- a/doc/guides/nics/features.rst
>>>>>> +++ b/doc/guides/nics/features.rst
>>>>>> @@ -583,9 +583,12 @@ 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``,
>>>>>> +* **[related]    API**: ``rte_eth_dev_get_supported_ptypes()``,
>>>>>> +  ``rte_eth_dev_set_ptypes()``, ``dev_ptypes_set``.
>>>>>> +* **[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 23182d187..2fa54aadc 100644
>>>>>> --- a/doc/guides/rel_notes/release_19_11.rst
>>>>>> +++ b/doc/guides/rel_notes/release_19_11.rst
>>>>>> @@ -86,6 +86,13 @@ New Features
>>>>>>    SET_TAG action and TAG item have been added to support transient flow
>>>>>>    tag.
>>>>>>  
>>>>>> +* **Added ethdev API to set supported packet types**
>>>>>> +
>>>>>> +  * Added new API ``rte_eth_dev_set_ptypes`` that allows an application to
>>>>>> +    inform PMD about about reduced range of packet types to handle.
>>>>>> +  * This scheme will allow PMDs to avoid lookup to internal ptype table on Rx
>>>>>> +    and thereby improve Rx performance if application wishes do so.
>>>>>> +
>>>>>>  * **Updated the enic driver.**
>>>>>>  
>>>>>>    * Added support for Geneve with options offload.
>>>>>> @@ -258,6 +265,7 @@ New Features
>>>>>>    * Rx metadata is delivered to host via a dynamic field of ``rte_mbuf`` with
>>>>>>      PKT_RX_DYNF_METADATA.
>>>>>>  
>>>>>> +
>>>>>>  Removed Items
>>>>>>  -------------
>>>>>>  
>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>>>>> index 652c369e5..13e6daaf4 100644
>>>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>>>> @@ -2879,6 +2879,92 @@ rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
>>>>>>  	return j;
>>>>>>  }
>>>>>>  
>>>>>> +int
>>>>>> +rte_eth_dev_set_ptypes(uint16_t port_id, uint32_t ptype_mask,
>>>>>> +				 uint32_t *set_ptypes, unsigned int num)
>>>>>> +{
>>>>>> +	const uint32_t valid_ptype_masks[] = {
>>>>>> +		RTE_PTYPE_L2_MASK,
>>>>>> +		RTE_PTYPE_L3_MASK,
>>>>>> +		RTE_PTYPE_L4_MASK,
>>>>>> +		RTE_PTYPE_TUNNEL_MASK,
>>>>>> +		RTE_PTYPE_INNER_L2_MASK,
>>>>>> +		RTE_PTYPE_INNER_L3_MASK,
>>>>>> +		RTE_PTYPE_INNER_L4_MASK,
>>>>>> +	};
>>>>>> +	const uint32_t *all_ptypes;
>>>>>> +	struct rte_eth_dev *dev;
>>>>>> +	uint32_t unused_mask;
>>>>>> +	unsigned int i, j;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>>> +	dev = &rte_eth_devices[port_id];
>>>>>> +
>>>>>> +	if (num > 0 && set_ptypes == NULL)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	if (*dev->dev_ops->dev_supported_ptypes_get == NULL ||
>>>>>> +			*dev->dev_ops->dev_ptypes_set == NULL) {
>>>>>> +		ret = 0;
>>>>>> +		goto ptype_unknown;
>>>>>> +	}
>>>>>
>>>>> Hi Pavan, Andrew,
>>>>>
>>>>> Why API is not returning "-ENOTSUP" id dev_ops is not provided as done it other
>>>>> APIs?
>>>>
>>>> As I understand the reason behind it is an optional nature
>>>> of the callback. It is just an optimization and there is no
>>>> point to complicate applications code to check and treat
>>>> -ENOTSUP is OK.
>>>
>>> I was also thinking this is just an optimization but Jerin wants to call it
>>> default in all samples first, now in testpmd by default.
>>
>> Why the optimization should not be applied in all samples?
>> If sample does not use ptype information it may and should
>> be applied since we are all keen for the best possible
>> performance.
>>
>>> Although it makes simple to ignore the error this way, I think it can be good to
>>> differentiate the success, not implemented and failed cases.
>>
>> No, it simplifies error tracking. We can check return value and
>> if it is not zero say that it goes wrong (instead of not zero and
>> not -ENOTSUP).
> 
> Right now it is not clear if API successfully run or not supported at all,

Why is it important? I think it is not important.

> also there is no API to get list of ptypes API reports after this API call,
> so there is no way to know what is current reported ptypes after this API
> return success

Yes, that's true. As I understand it possible to obtain the information
at the type of set_types only.

> For example, for the tespmd call, I suggest printing a log saying that supported
> ptypes has been reduced if the API returns successfully, but with current API we
> can't really say if it was successful or not.

Fair and makes sense. We can dump returned set_ptypes.
Anyway saying if it is *reduced* or not is impossible
without explicit indication and I'm not sure that the
information required (worse efforts to provide it).

>>>>
>>>>>> +
>>>>>> +	if (ptype_mask == 0) {
>>>>>> +		ret = (*dev->dev_ops->dev_ptypes_set)(dev,
>>>>>> +				ptype_mask);
>>>>>> +		goto ptype_unknown;
>>>>>> +	}
>>>>>> +
>>>>>> +	unused_mask = ptype_mask;
>>>>>> +	for (i = 0; i < RTE_DIM(valid_ptype_masks); i++) {
>>>>>> +		uint32_t mask = ptype_mask & valid_ptype_masks[i];
>>>>>> +		if (mask && mask != valid_ptype_masks[i]) {
>>>>>> +			ret = -EINVAL;
>>>>>> +			goto ptype_unknown;
>>>>>> +		}
>>>>>> +		unused_mask &= ~valid_ptype_masks[i];
>>>>>> +	}
>>>>>
>>>>> What does above block does?
>>>>> Does it mean 'ptype_mask' only can be combination of 'valid_ptype_masks' items?
>>>>
>>>> Yes.
>>>>
>>>>> Let's say can't I set only 'RTE_PTYPE_L2_ETHER' as requested ptype?
>>>>
>>>> Yes
>>>>
>>>>> Why this limitation and is it documented anywhere, like the API documentation?
>>>>
>>>> It is simply incorrect to have non-mask bits here.
>>>> RTE_PTYPE_L2_ETHER & RTE_PTYPE_L2_ETHER_ARP != 0
>>>
>>> Right.
>>>
>>>> It is documented in the ptype_mask parameter description.
>>>
>>> OK, I missed it in ptype_mask description.
>>>
>>>>
>>>>>> +
>>>>>> +	if (unused_mask) {
>>>>>> +		ret = -EINVAL;
>>>>>> +		goto ptype_unknown;
>>>>>> +	}
>>>>>> +
>>>>>> +	all_ptypes = (*dev->dev_ops->dev_supported_ptypes_get)(dev);
>>>>>> +	if (all_ptypes == NULL) {
>>>>>> +		ret = 0;
>>>>>> +		goto ptype_unknown;
>>>>>> +	}
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * Accodommodate as many set_ptypes as possible. If the supplied
>>>>>
>>>>> s/Accodommodate/Accommodate
>>>>>
>>>>>> +	 * set_ptypes array is insufficient fill it partially.
>>>>>> +	 */
>>>>>> +	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 && j < num)
>>>>>> +		set_ptypes[j] = RTE_PTYPE_UNKNOWN;
>>>>>> +
>>>>>> +	return (*dev->dev_ops->dev_ptypes_set)(dev, ptype_mask);
>>>>>
>>>>> Only a question, at this stage API knows the supported ptypes by device, does it
>>>>> make sense to check if 'ptype_mask' has anything unsupported instead of passing
>>>>> it to the driver and expect driver to check it?
>>>>
>>>> It is also explained by the nature of the API. Application says:
>>>> I'm interested in the following ptypes only to let PMD know that
>>>> it could drop classification behind it. If PMD does not support
>>>> some layers, it will not provide classification anyway.
>>>
>>> in "eth_dev_ptypes_set_t" documentation, it says:
>>>  "- (-EINVAL) if *ptype_mask* is invalid."
>>> I thought PMD does a check on received 'ptype_mask', if expectation is PMD will
>>> do the check, why not do the check in API commonly instead of each PMD.
>>> What is the documented case "*ptype_mask* is invalid."?
>>
>> Not a combination of RTE_PTYPE_*_MASK.
>> There are few cases above where -EINVAL is returned includes failed
>> validation of ptype_mask.
> 
> OK, let me try to again,
> according API, *PMD* can return error if 'ptype_mask' is not valid,

Ah, it is rather a comment related to eth_dev_ptypes_set_t prototype
description. I'd remove -EINVAL description from the prototype
description.

> a) what is that validity check in PMD?
> b) if it is a common check can we move if from PMD to the API?
>
>>
>>>>
>>>>>> +
>>>>>> +ptype_unknown:
>>>>>> +	if (num > 0)
>>>>>> +		set_ptypes[0] = RTE_PTYPE_UNKNOWN;
>>>>>> +
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>>  int
>>>>>>  rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr)
>>>>>>  {
>>>>>> @@ -2891,7 +2977,6 @@ rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr)
>>>>>>  	return 0;
>>>>>>  }
>>>>>>  
>>>>>> -
>>>>>>  int
>>>>>>  rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu)
>>>>>>  {
>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>>>>> index 44d77b332..a7872819d 100644
>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>>> @@ -2604,6 +2604,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 about reduced range of packet types to handle.
>>>>>> + *
>>>>>> + * 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 should be bitwise OR of
>>>>>> + *   RTE_PTYPE_*_MASK or 0.
>>>>>
>>>>> No strong opinion but is it really a mask? it doesn't used to mask any value,
>>>>> but it is list of ptypes that application requests, in a bitwise ORed form, so I
>>>>> this it can be ptypes.
>>>>
>>>> See above.
>>>
>>> OK, it is a mask really.
>>>
>>>>
>>>>>> + * @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.
>>>>>
>>>>> Not for this patch, but is an API also required to get current supported ptypes
>>>>> from the driver? Even application keeps this list, something can reset in the
>>>>> device/driver, supported pytpe list can be changed and it can differ with what
>>>>> application knows.
>>>>
>>>> I would say that set_ptypes should not affected supported_ptypes.
>>>
>>> I expect same. I was asking should we have an API that returns list of ptypes
>>> returned by PMD after 'rte_eth_dev_set_ptypes()' call? Can we know that list
>>> without a new API?
>>
>> As I understand the only way right now is to get on when ptypes are set.
> 
> That is what I am saying, assuming application storing this ptype list and
> driver did a reset or something can make application list invalid/wrong, so a
> better way to get the list can be required.

Yes, that's true. I have no strong opinion.

>> I would add _get_ptypes() function when it is really required only.
> 
> OK, I don't know if application may need to know it.

I suggest to wait with it. It is an experimental API and
usage will show if _get_ptypes() is required or not.

Many thanks.

>>
>>>>
>>>>>> + * @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_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 f215af7c9..657c39b56 100644
>>>>>> --- a/lib/librte_ethdev/rte_ethdev_core.h
>>>>>> +++ b/lib/librte_ethdev/rte_ethdev_core.h
>>>>>> @@ -234,6 +234,22 @@ typedef int (*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. */
>>>>>>  
>>>>>> +/**
>>>>>> + * @internal
>>>>>> + * Inform Ethernet device about reduced range of packet types to handle.
>>>>>> + *
>>>>>> + * @param dev
>>>>>> + *   The Ethernet device identifier.
>>>>>> + * @param ptype_mask
>>>>>> + *   The ptype family that application is interested in should be bitwise OR of
>>>>>> + *   RTE_PTYPE_*_MASK or 0.
>>>>>> + * @return
>>>>>> + *   - (0) if Success.
>>>>>> + *   - (-EINVAL) if *ptype_mask* is invalid.
>>>>>> + */
>>>>>> +typedef int (*eth_dev_ptypes_set_t)(struct rte_eth_dev *dev,
>>>>>> +				     uint32_t ptype_mask);
>>>>>> +
>>>>>>  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. */
>>>>>> @@ -630,6 +646,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_ptypes_set_t dev_ptypes_set;
>>>>>> +	/**< Inform Ethernet device about reduced range of packet types to handle. */
>>>>>>  
>>>>>>  	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 ee289ef13..ccfbeae23 100644
>>>>>> --- a/lib/librte_ethdev/rte_ethdev_version.map
>>>>>> +++ b/lib/librte_ethdev/rte_ethdev_version.map
>>>>>> @@ -292,4 +292,5 @@ EXPERIMENTAL {
>>>>>>  	rte_flow_dynf_metadata_offs;
>>>>>>  	rte_flow_dynf_metadata_mask;
>>>>>>  	rte_flow_dynf_metadata_register;
>>>>>> +	rte_eth_dev_set_ptypes;
>>>>>>  };
>>>>>>
>>>>
>>
>