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.ma...@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.ma...@oktetlabs.ru>
>>> Reviewed-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
>>> Reviewed-by: Andy Moreton <amore...@xilinx.com>
>>> Acked-by: Ray Kinsella <m...@ashroe.eu>
>>> Acked-by: Jerin Jacob <jer...@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.
