Re: [dpdk-dev] [PATCH v3 1/5] ethdev: add API to negotiate delivery of Rx meta data

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 9/30/21 10:07 PM, Ivan Malov wrote:
> Hi Ori,
> 
> On 30/09/2021 17:59, Ori Kam wrote:
>> Hi Ivan,
>> Sorry for jumping in late.
> 
> No worries. That's OK.
> 
>> I have a concern that this patch breaks other PMDs.
> 
> It does no such thing.
> 
>>> From the rst file " One should negotiate flag delivery beforehand"
>> since you only added this function for your PMD all other PMD will fail.
>> I see that you added exception in the  examples, but it doesn't make
>> sense
>> that applications will also need to add this exception which is not
>> documented.
> 
> Say, you have an application, and you use it with some specific PMD.
> Say, that PMD doesn't run into the problem as ours does. In other words,
> the user can insert a flow with action MARK at any point and get mark
> delivery working starting from that moment without any problem. Say,
> this is exactly the way how it works for you at the moment.
> 
> Now. This new API kicks in. We update the application to invoke it as
> early as possible. But your PMD in question still doesn't support this
> API. The comment in the patch says that if the method returns ENOTSUP,
> the application should ignore that without batting an eyelid. It should
> just keep on working as it did before the introduction of this API.
> 
> More specific example:
> Say, the application doesn't mind using either "RSS + MARK" or tunnel
> offload. What it does right now is attempt to insert tunnel flows first
> and, if this fails, fall back to "RSS + MARK". With this API, the
> application will try to invoke this API with "USER_MARK | TUNNEL_ID" in
> adapter initialised state. If the PMD says that it can only enable the
> tunnel offload, then the application will get the knowledge that it
> doesn't make sense to even try inserting "RSS + MARK" flows. It just can
> skip useless actions. But if the PMD doesn't support the method, the
> application will see ENOTSUP and handle this gracefully: it will make no
> assumptions about what's guaranteed to be supported and what's not and
> will just keep on its old behaviour: try to insert a flow, fail, fall
> back to another type of flow.
> 
> So no breakages with this API.
> 
>>
>> Please see more comments inline.
>>
>> Thanks,
>> Ori
>>
>>> -----Original Message-----
>>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>> Sent: Thursday, September 23, 2021 2:20 PM
>>> Subject: [PATCH v3 1/5] ethdev: add API to negotiate delivery of Rx meta
>>> data
>>>
>>> Delivery of mark, flag and the likes might affect small packet
>>> performance.
>>> If these features are disabled by default, enabling them in started
>>> state
>>> without causing traffic disruption may not always be possible.
>>>
>>> Let applications negotiate delivery of Rx meta data beforehand.
>>>
>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>> Acked-by: Ray Kinsella <mdr@ashroe.eu>
>>> Acked-by: Jerin Jacob <jerinj@marvell.com>
>>> ---
>>>   app/test-flow-perf/main.c              | 21 ++++++++++++
>>>   app/test-pmd/testpmd.c                 | 26 +++++++++++++++
>>>   doc/guides/rel_notes/release_21_11.rst |  9 ++++++
>>>   lib/ethdev/ethdev_driver.h             | 19 +++++++++++
>>>   lib/ethdev/rte_ethdev.c                | 25 ++++++++++++++
>>>   lib/ethdev/rte_ethdev.h                | 45 ++++++++++++++++++++++++++
>>>   lib/ethdev/rte_flow.h                  | 12 +++++++
>>>   lib/ethdev/version.map                 |  3 ++
>>>   8 files changed, 160 insertions(+)
>>>
>>> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c index
>>> 9be8edc31d..48eafffb1d 100644
>>> --- a/app/test-flow-perf/main.c
>>> +++ b/app/test-flow-perf/main.c
>>> @@ -1760,6 +1760,27 @@ init_port(void)
>>>           rte_exit(EXIT_FAILURE, "Error: can't init mbuf pool\n");
>>>
>>>       for (port_id = 0; port_id < nr_ports; port_id++) {
>>> +        uint64_t rx_meta_features = 0;
>>> +
>>> +        rx_meta_features |= RTE_ETH_RX_META_USER_FLAG;
>>> +        rx_meta_features |= RTE_ETH_RX_META_USER_MARK;
>>> +
>>> +        ret = rte_eth_rx_meta_negotiate(port_id,
>>> &rx_meta_features);
>>> +        if (ret == 0) {
>>> +            if (!(rx_meta_features &
>>> RTE_ETH_RX_META_USER_FLAG)) {
>>> +                printf(":: flow action FLAG will not affect Rx
>>> mbufs on port=%u\n",
>>> +                       port_id);
>>> +            }
>>> +
>>> +            if (!(rx_meta_features &
>>> RTE_ETH_RX_META_USER_MARK)) {
>>> +                printf(":: flow action MARK will not affect Rx
>>> mbufs on port=%u\n",
>>> +                       port_id);
>>> +            }
>>> +        } else if (ret != -ENOTSUP) {
>>> +            rte_exit(EXIT_FAILURE, "Error when negotiating Rx
>>> meta features on port=%u: %s\n",
>>> +                 port_id, rte_strerror(-ret));
>>> +        }
>>> +
>>>           ret = rte_eth_dev_info_get(port_id, &dev_info);
>>>           if (ret != 0)
>>>               rte_exit(EXIT_FAILURE,
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 97ae52e17e..7a8da3d7ab 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -1485,10 +1485,36 @@ static void
>>>   init_config_port_offloads(portid_t pid, uint32_t socket_id)  {
>>>       struct rte_port *port = &ports[pid];
>>> +    uint64_t rx_meta_features = 0;
>>>       uint16_t data_size;
>>>       int ret;
>>>       int i;
>>>
>>> +    rx_meta_features |= RTE_ETH_RX_META_USER_FLAG;
>>> +    rx_meta_features |= RTE_ETH_RX_META_USER_MARK;
>>> +    rx_meta_features |= RTE_ETH_RX_META_TUNNEL_ID;
>>> +
>>> +    ret = rte_eth_rx_meta_negotiate(pid, &rx_meta_features);
>>> +    if (ret == 0) {
>>> +        if (!(rx_meta_features & RTE_ETH_RX_META_USER_FLAG)) {
>>> +            TESTPMD_LOG(INFO, "Flow action FLAG will not
>>> affect Rx mbufs on port %u\n",
>>> +                    pid);
>>> +        }
>>> +
>>> +        if (!(rx_meta_features & RTE_ETH_RX_META_USER_MARK))
>>> {
>>> +            TESTPMD_LOG(INFO, "Flow action MARK will not
>>> affect Rx mbufs on port %u\n",
>>> +                    pid);
>>> +        }
>>> +
>>> +        if (!(rx_meta_features & RTE_ETH_RX_META_TUNNEL_ID)) {
>>> +            TESTPMD_LOG(INFO, "Flow tunnel offload support
>>> might be limited or unavailable on port %u\n",
>>> +                    pid);
>>> +        }
>>> +    } else if (ret != -ENOTSUP) {
>>> +        rte_exit(EXIT_FAILURE, "Error when negotiating Rx meta
>>> features on port %u: %s\n",
>>> +             pid, rte_strerror(-ret));
>>> +    }
>>> +
>>>       port->dev_conf.txmode = tx_mode;
>>>       port->dev_conf.rxmode = rx_mode;
>>>
>>> diff --git a/doc/guides/rel_notes/release_21_11.rst
>>> b/doc/guides/rel_notes/release_21_11.rst
>>> index 19356ac53c..6674d4474c 100644
>>> --- a/doc/guides/rel_notes/release_21_11.rst
>>> +++ b/doc/guides/rel_notes/release_21_11.rst
>>> @@ -106,6 +106,15 @@ New Features
>>>     Added command-line options to specify total number of processes and
>>>     current process ID. Each process owns subset of Rx and Tx queues.
>>>
>>> +* **Added an API to negotiate delivery of specific parts of Rx meta
>>> +data**
>>> +
>>> +  A new API, ``rte_eth_rx_meta_negotiate()``, was added.
>>> +  The following parts of Rx meta data were defined:
>>> +
>>> +  * ``RTE_ETH_RX_META_USER_FLAG``
>>> +  * ``RTE_ETH_RX_META_USER_MARK``
>>> +  * ``RTE_ETH_RX_META_TUNNEL_ID``
>>> +
>>>
>>>   Removed Items
>>>   -------------
>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>> index
>>> 40e474aa7e..96e0c60cae 100644
>>> --- a/lib/ethdev/ethdev_driver.h
>>> +++ b/lib/ethdev/ethdev_driver.h
>>> @@ -789,6 +789,22 @@ typedef int (*eth_get_monitor_addr_t)(void *rxq,
>>> typedef int (*eth_representor_info_get_t)(struct rte_eth_dev *dev,
>>>       struct rte_eth_representor_info *info);
>>>
>>> +/**
>>> + * @internal
>>> + * Negotiate delivery of specific parts of Rx meta data.
>>> + *
>>> + * @param dev
>>> + *   Port (ethdev) handle
>>> + *
>>> + * @param[inout] features
>>> + *   Feature selection buffer
>>> + *
>>> + * @return
>>> + *   Negative errno value on error, zero otherwise
>>> + */
>>> +typedef int (*eth_rx_meta_negotiate_t)(struct rte_eth_dev *dev,
>>> +                       uint64_t *features);
>>> +
>>>   /**
>>>    * @internal A structure containing the functions exported by an
>>> Ethernet
>>> driver.
>>>    */
>>> @@ -949,6 +965,9 @@ struct eth_dev_ops {
>>>
>>>       eth_representor_info_get_t representor_info_get;
>>>       /**< Get representor info. */
>>> +
>>> +    eth_rx_meta_negotiate_t rx_meta_negotiate;
>>> +    /**< Negotiate delivery of specific parts of Rx meta data. */
>>>   };
>>>
>>>   /**
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index
>>> daf5ca9242..49cb84d64c 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -6310,6 +6310,31 @@ rte_eth_representor_info_get(uint16_t port_id,
>>>       return eth_err(port_id, (*dev->dev_ops-
>>>> representor_info_get)(dev, info));  }
>>>
>>> +int
>>> +rte_eth_rx_meta_negotiate(uint16_t port_id, uint64_t *features) {
>>> +    struct rte_eth_dev *dev;
>>> +
>>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>> +    dev = &rte_eth_devices[port_id];
>>> +
>>> +    if (dev->data->dev_configured != 0) {
>>> +        RTE_ETHDEV_LOG(ERR,
>>> +            "The port (id=%"PRIu16") is already configured\n",
>>> +            port_id);
>>> +        return -EBUSY;
>>> +    }
>>> +
>>> +    if (features == NULL) {
>>> +        RTE_ETHDEV_LOG(ERR, "Invalid features (NULL)\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_meta_negotiate,
>>> -ENOTSUP);
>>> +    return eth_err(port_id,
>>> +               (*dev->dev_ops->rx_meta_negotiate)(dev, features)); }
>>> +
>>>   RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
>>>
>>>   RTE_INIT(ethdev_init_telemetry)
>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
>>> 1da37896d8..8467a7a362 100644
>>> --- a/lib/ethdev/rte_ethdev.h
>>> +++ b/lib/ethdev/rte_ethdev.h
>>> @@ -4888,6 +4888,51 @@ __rte_experimental  int
>>> rte_eth_representor_info_get(uint16_t port_id,
>>>                    struct rte_eth_representor_info *info);
>>>
>>> +/** The ethdev sees flagged packets if there are flows with action
>>> +FLAG. */ #define RTE_ETH_RX_META_USER_FLAG (UINT64_C(1) << 0)
>>> +
>>> +/** The ethdev sees mark IDs in packets if there are flows with action
>>> +MARK. */ #define RTE_ETH_RX_META_USER_MARK (UINT64_C(1) << 1)
>>> +
>>> +/** The ethdev detects missed packets if there are "tunnel_set" flows
>>> +in use. */ #define RTE_ETH_RX_META_TUNNEL_ID (UINT64_C(1) << 2)
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Negotiate delivery of specific parts of Rx meta data.
>>> + *
>>> + * Invoke this API before the first rte_eth_dev_configure() invocation
>>> + * to let the PMD make preparations that are inconvenient to do later.
>>> + *
>>> + * The negotiation process is as follows:
>>> + *
>>> + * - the application requests features intending to use at least some
>>> +of them;
>>> + * - the PMD responds with the guaranteed subset of the requested
>>> +feature set;
>>> + * - the application can retry negotiation with another set of
>>> +features;
>>> + * - the application can pass zero to clear the negotiation result;
>>> + * - the last negotiated result takes effect upon the ethdev start.
>>> + *
>>> + * If this API is unsupported, the application should gracefully
>>> ignore that.
>>> + *
>>> + * @param port_id
>>> + *   Port (ethdev) identifier
>>> + *
>>> + * @param[inout] features
>>> + *   Feature selection buffer
>>> + *
>>> + * @return
>>> + *   - (-EBUSY) if the port can't handle this in its current state;
>>> + *   - (-ENOTSUP) if the method itself is not supported by the PMD;
>>> + *   - (-ENODEV) if *port_id* is invalid;
>>> + *   - (-EINVAL) if *features* is NULL;
>>> + *   - (-EIO) if the device is removed;
>>> + *   - (0) on success
>>> + */
>>> +__rte_experimental
>>> +int rte_eth_rx_meta_negotiate(uint16_t port_id, uint64_t *features);
>>
>> I don't think meta is the best name since we also have meta item and
>> the word meta can be used
>> in other cases.
> 
> I'm no expert in naming. What could be a better term for this?
> Personally, I'd rather not perceive "meta" the way you describe. It's
> not just "meta". It's "rx_meta", and the flags supplied with this API
> provide enough context to explain what it's all about.

Thinking overnight about it I'd suggest full "metadata".
Yes, it will name a bit longer, but less confusing versus
term META already used in flow API.

Andrew.