Re: [PATCH v4] ethdev: advertise flow restore in mbuf

[Ilya Maximets <i.maximets@ovn.org>]

On 9/26/23 11:17, David Marchand wrote:
> Hello Ilya,
> 
> On Mon, Jul 31, 2023 at 10:40 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>> On 6/21/23 16:43, David Marchand wrote:
>>> As reported by Ilya [1], unconditionally calling
>>> rte_flow_get_restore_info() impacts an application performance for drivers
>>> that do not provide this ops.
>>> It could also impact processing of packets that require no call to
>>> rte_flow_get_restore_info() at all.
>>>
>>> Register a dynamic mbuf flag when an application negotiates tunnel
>>> metadata delivery (calling rte_eth_rx_metadata_negotiate() with
>>> RTE_ETH_RX_METADATA_TUNNEL_ID).
>>>
>>> Drivers then advertise that metadata can be extracted by setting this
>>> dynamic flag in each mbuf.
>>>
>>> The application then calls rte_flow_get_restore_info() only when required.
>>>
>>> Link: http://inbox.dpdk.org/dev/5248c2ca-f2a6-3fb0-38b8-7f659bfa40de@ovn.org/
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>> Tested-by: Ali Alnubani <alialnu@nvidia.com>
>>> Acked-by: Ori Kam <orika@nvidia.com>
>>> ---
>>> Changes since RFC v3:
>>> - rebased on next-net,
>>> - sending as non RFC for CIs that skip RFC patches,
>>>
>>> Changes since RFC v2:
>>> - fixed crash introduced in v2 and removed unneeded argument to
>>>   rte_flow_restore_info_dynflag_register(),
>>>
>>> Changes since RFC v1:
>>> - rebased,
>>> - updated vectorized datapath functions for net/mlx5,
>>> - moved dynamic flag register to rte_eth_rx_metadata_negotiate() and
>>>   hid rte_flow_restore_info_dynflag_register() into ethdev internals,
>>>
>>> ---
>>>  app/test-pmd/util.c                      |  9 +++--
>>>  drivers/net/mlx5/mlx5.c                  |  2 +
>>>  drivers/net/mlx5/mlx5.h                  |  5 ++-
>>>  drivers/net/mlx5/mlx5_flow.c             | 47 +++++++++++++++++++++---
>>>  drivers/net/mlx5/mlx5_rx.c               |  2 +-
>>>  drivers/net/mlx5/mlx5_rx.h               |  1 +
>>>  drivers/net/mlx5/mlx5_rxtx_vec_altivec.h | 16 ++++----
>>>  drivers/net/mlx5/mlx5_rxtx_vec_neon.h    |  6 +--
>>>  drivers/net/mlx5/mlx5_rxtx_vec_sse.h     |  6 +--
>>>  drivers/net/mlx5/mlx5_trigger.c          |  4 +-
>>>  drivers/net/sfc/sfc_dp.c                 | 14 +------
>>>  lib/ethdev/rte_ethdev.c                  |  5 +++
>>>  lib/ethdev/rte_flow.c                    | 27 ++++++++++++++
>>>  lib/ethdev/rte_flow.h                    | 18 ++++++++-
>>>  lib/ethdev/rte_flow_driver.h             |  6 +++
>>>  lib/ethdev/version.map                   |  1 +
>>>  16 files changed, 128 insertions(+), 41 deletions(-)
>>
>> <snip>
>>
>>> diff --git a/lib/ethdev/rte_flow_driver.h b/lib/ethdev/rte_flow_driver.h
>>> index 356b60f523..f9fb01b8a2 100644
>>> --- a/lib/ethdev/rte_flow_driver.h
>>> +++ b/lib/ethdev/rte_flow_driver.h
>>> @@ -376,6 +376,12 @@ struct rte_flow_ops {
>>>  const struct rte_flow_ops *
>>>  rte_flow_ops_get(uint16_t port_id, struct rte_flow_error *error);
>>>
>>> +/**
>>> + * Register mbuf dynamic flag for rte_flow_get_restore_info.
>>> + */
>>> +int
>>> +rte_flow_restore_info_dynflag_register(void);
>>> +
>>
>> Hi, David, others.
>>
>> Is there a reason to not expose this function to the application?
>>
>> The point is that application will likely want to know the value
>> of the flag before creating any devices.  I.e. request it once
>> and use for all devices later without performing a call to an
>> external library (DPDK).  In current implementation, application
>> will need to open some device first, and only then the result of
>> rte_flow_restore_info_dynflag() will become meaningful.
>>
>> There is no need to require application to call this function,
>> it can still be called from the rx negotiation API, but it would
>> be nice if application could know it beforehand, i.e. had control
>> over when the flag is actually becomes visible.
> 
> DPDK tries to register flags only when needed, as there is not a lot
> of space for dyn flags.
> Some drivers take some space and applications want some share too.
> 
> DPDK can export the _register function for applications to call it
> regardless of what driver will be used later.
> 
> Yet, I want to be sure why it matters in OVS context.
> Is it not enough resolving the flag (by calling
> rte_flow_restore_info_dynflag()) once rte_eth_rx_metadata_negotiate
> for tunnel metadata is called?
> Do you want to avoid an atomic store/load between OVS main thread and
> PMD threads?

Yeas, something like this.  I do not have a solid implementation idea
for that though.  I replied to your OVS patch.

Best regards, Ilya Maximets.