On Wed, Jun 14, 2023 at 6:46 PM Slava Ovsiienko <viachesl...@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
