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

[David Marchand <david.marchand@redhat.com>]

On Wed, Jun 14, 2023 at 6:46 PM Slava Ovsiienko <viacheslavo@nvidia.com> wrote:
>
> Hi, David
>
> It looks like a good application datapath optimization, as for me.
> But I see some concerns:
>
> 1. Are we sure the PMD should register the flag, not application?
> IIRC, usually application registers needed flags/fields and PMDs just follow.

I agree, this should come from the application.
Maybe this flag should be resolved when the application calls
rte_eth_rx_metadata_negotiate().
For an unknown reason, mlx5 does not implement this ops atm, but I
guess this would be a good place to check for the dv_xmeta_en stuff,
too.


>
> +       if (!sh->tunnel_hub && sh->config.dv_miss_info) {
> +               err = mlx5_flow_restore_info_register();
> +               if (err) {
> +                       DRV_LOG(ERR, "Could not register mbuf dynflag for rte_flow_get_restore_info");
> +                       goto error;
> +               }
>
> 2. This might have some general mlx5 Rx datapath performance impact (very minor though)
> It introduces extra memory access (instead of immed value). We should test thoroughly.
>  @@ -857,7 +857,7 @@ rxq_cq_to_mbuf(struct mlx5_rxq_data *rxq, struct rte_mbuf *pkt,
>                 if (MLX5_FLOW_MARK_IS_VALID(mark)) {
>                         pkt->ol_flags |= RTE_MBUF_F_RX_FDIR;
>                         if (mark != RTE_BE32(MLX5_FLOW_MARK_DEFAULT)) {
> -                               pkt->ol_flags |= RTE_MBUF_F_RX_FDIR_ID;
> +                               pkt->ol_flags |= rxq->mark_flag;
>                                 pkt->hash.fdir.hi = mlx5_flow_mark_get(mark);
>                         }
>                 }

I am curious about the impact of such a change too.


>
> 3. RTE_MBUF_F_RX_FDIR_ID is also handled in vectorized rx_burst() routines.
> Please, see:
>  - mlx5_rxtx_vec_altivec.h
>  - mlx5_rxtx_vec_neon.h
>  - mlx5_rxtx_vec_sse.h
> This code must be updated, and it also might have some general (regardless of using tunnel offload
> - for all cases) performance impact.

Indeed, I forgot to squash some later changes I was experimenting with.
Well, numbers will tell us.

I'll send a RFC v2.


-- 
David Marchand