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, pbhagavat...@marvell.com wrote:
>>>>>> From: Pavan Nikhilesh <pbhagavat...@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.anan...@intel.com>
>>>>>> Signed-off-by: Pavan Nikhilesh <pbhagavat...@marvell.com>
>>>>>> Reviewed-by: Andrew Rybchenko <arybche...@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;
>>>>>> };
>>>>>>
>>>>
>>
>
