Re: [dpdk-stable] [dpdk-dev] [PATCH v3] net/mlx5: fix tunnel offload private items location

[Gregory Etelson <getelson@nvidia.com>]

Hello Ferruh,

> >> The were some questions around testpmd part of this patch in previous
> >> version, they are not answered and this version is dropping the
> >> testpmd part.
> >>
> >
> > The tunnel offload API allows application to place tunnel offload elements
> at any valid location in a flow rule.
> 
> How PMD should detect the tunnel offload elements?
> 

Flow elements in tunnel offload rule can be logically divided into two parts:
- application elements that reflect application logic
- tunnel offload related elements. These flow elements are selected by PMD 
to implement tunnel offload with respect to underlying hardware.
Application obtains these actions and items from PMD with 
rte_flow_tunnel_decap_and_set() and rte_flow_tunnel_match().
Application combines both parts into a flow rule and sends it to PMD.

> And which API are we talking about, is there enough documentation in the
> API about the location of the tunnel offload elements, or should we clarify it
> more?
> 

The tunnel offload API was introduced here:
commit 9ec0f97e02e1 ("ethdev: add tunnel offload model").
There is a detailed explanation with examples how the API works.
 
> > Current testpmd code places tunnel offload items at the beginning of
> > pattern array and tunnel offload actions at the beginning of actions array.
> 
> Got it, so this patch is removing false expectation (about the location of the
> tunnel offload elements in flow rule) in the PMD, right?
> 

Correct. Location of flow elements supplied by PMD in a rule is not important. 

> As far as I understand testpmd already satisfies this false expectation, so
> the problem should not be visible with testpmd.
> Was there a visible user impact, like any application failing because of this
> false expectation, or is this theoretical fix to be correct with API contract?
> 

There is no issue with the testpmd code. 
The bug was discovered by OVS.
 
> btw, I can see 'MLX5_RTE_FLOW_ITEM_TYPE_TUNNEL' still checked if it in
> the first location of the items (items[0].type), in 'flow_dv_validate()',
> 'mlx5_flow_dv.c'
> https://git.dpdk.org/dpdk/tree/drivers/net/mlx5/mlx5_flow_dv.c?h=v21.0
> 5-rc1#n6298
> Can you please confirm that this is expected?
>

The `items` variable in that function iterates on pattern array:

for (; items->type != RTE_FLOW_ITEM_TYPE_END; items++) {

In this scenario,

case MLX5_RTE_FLOW_ITEM_TYPE_TUNNEL:
  if (items[0].type != 
        (typeof(items[0].type))MLX5_RTE_FLOW_ITEM_TYPE_TUNNEL)

compares items with itself.

> > The testpmd patch in v1 & v2 changed location of tunnel offload elements
> in a flow rule.
> >
> >> Is it safe to remove the testpmd part? If so why was the changes for
> >> at first place?
> >>
> >
> > That patch was not a fix - it emphasized general usage of the tunnel
> offload API.
> > Current testpmd code works fine.
> >
> >> And can you please reply to the questions on the testpmd patch for
> >> the record?
> >>
> >
> > I'll add my replies.
> >
> >> Thanks,
> >> Ferruh
> >
> > Regards,
> > Gregory
> >