Hello Ferruh,

quoted

quoted

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.
 

quoted

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.

quoted

The testpmd patch in v1 & v2 changed location of tunnel offload elements

in a flow rule.

quoted

quoted

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.

quoted

Current testpmd code works fine.

quoted

And can you please reply to the questions on the testpmd patch for
the record?

I'll add my replies.

quoted

Thanks,
Ferruh

Regards,
Gregory