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

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

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.

Alternatively, the _register() could also be called right from
the rte_flow_restore_info_dynflag() at a slight performance cost.
It shouldn't be important though, since drivers do not seem to
call it from a performance-sensitive code.

Thoughts?

Best regards, Ilya Maximets.