Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri

[Thomas Monjalon <thomas@monjalon.net>]

08/01/2021 10:29, Andrew Rybchenko:
> On 1/8/21 11:57 AM, Ferruh Yigit wrote:
> > On 1/8/2021 1:41 AM, Zhang, Qi Z wrote:
> >> From: Thomas Monjalon <thomas@monjalon.net>
> >>> 07/01/2021 16:24, Zhang, Qi Z:
> >>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>> 07/01/2021 13:47, Zhang, Qi Z:
> >>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>> 07/01/2021 10:32, Guo, Jia:
> >>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>>>> 24/12/2020 07:59, Jeff Guo:
> >>>>>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>>>>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>>>>>>>>> @@ -1219,6 +1219,7 @@ enum rte_eth_tunnel_type {
> >>>>>>>>>>       RTE_TUNNEL_TYPE_IP_IN_GRE,
> >>>>>>>>>>       RTE_L2_TUNNEL_TYPE_E_TAG,
> >>>>>>>>>>       RTE_TUNNEL_TYPE_VXLAN_GPE,
> >>>>>>>>>> +    RTE_TUNNEL_TYPE_ECPRI,
> >>>>>>>>>>       RTE_TUNNEL_TYPE_MAX,
> >>>>>>>>>>   };
> >>>>>>>>>
> >>>>>>>>> We tried to remove all these legacy API in DPDK 20.11.
> >>>>>>>>> Andrew decided to not remove this one because it is not yet
> >>>>>>>>> completely replaced by rte_flow in all drivers.
> >>>>>>>>> However, I am against continuing to update this API.
> >>>>>>>>> The opposite work should be done: migrate to rte_flow.
> >>>>>>>>
> >>>>>>>> Agree but seems that the legacy api and driver legacy
> >>>>>>>> implementation still keep in this release, and there is no a
> >>>>>>>> general way to replace the legacy by rte_flow right now.
> >>>>>>>
> >>>>>>> I think rte_flow is a complete replacement with more features.
> >>>>>>
> >>>>>> Thomas, I may not agree with this.
> >>>>>>
> >>>>>> Actually the "enum rte_eth_tunnel_type" is used by
> >>>>>> rte_eth_dev_udp_tunnel_port_add A packet with specific dst udp
> >>>>>> port will be recognized as a specific tunnel packet type (e.g.
> >>>>>> vxlan, vxlan-gpe,
> >>>>> ecpri...) In Intel NIC, the API actually changes the configuration
> >>>>> of the packet parser in HW but not add a filter rule and I guess all
> >>>>> other devices may enable it in a similar way.
> >>>>>> so naturally it should be a device (port) level configuration but
> >>>>>> not a rte_flow
> >>>>> rule for match, encap, decap...
> >>>>>
> >>>>> I don't understand how it helps to identify an UDP port if there is
> >>>>> no rule for this tunnel.
> >>>>> What is the usage?
> >>>>
> >>>> Yes, in general It is a rule, it matches a udp packet's dst port 
> >>>> and the action is
> >>> "now the packet is identified as vxlan packet" then all other 
> >>> rte_flow rules that
> >>> match for a vlxan as pattern will take effect.  but somehow, I think 
> >>> they are
> >>> not rules in the same domain, just like we have dedicate API for 
> >>> mac/vlan filter,
> >>> we'd better have a dedicate API for this also. ( RFC for Vxlan 
> >>> explains why we
> >>> need this. https://tools.ietf.org/html/rfc7348).
> >>>>
> >>>> "Destination Port: IANA has assigned the value 4789 for the VXLAN UDP
> >>>> port, and this value SHOULD be used by default as the destination UDP
> >>>> port.  Some early implementations of VXLAN have used other values for
> >>>> the destination port.  To enable interoperability with these
> >>>> implementations, the destination port SHOULD be configurable."
> >>>
> >>> Yes the port number is free.
> >>> But isn't it more natural to specify this port number as part of the 
> >>> rte_flow
> >>> rule?
> >>
> >> I think if we have a rte_flow action type that can be used to set a 
> >> packet's tunnel type xxx, like below
> >> #flow create eth/ipv4/udp port is 4789/... action set_tunnel_type 
> >> VxLAN / end
> >> then we may replace it with rte_flow, but I'm not sure if it's 
> >> necessary, please share if you have a better idea.

Of course we can specify the UDP port in rte_flow rule.
Please check rte_flow_item_udp.
That's a basic of rte_flow.


> > Isn't this more a device configuration than filtering, not sure about 
> > using rte_flow for this.
> 
> +1

A device configuration? No, setting an UDP port is a stack configuration.

 
> >> BTW, are we going to move all other filter like mac , VLAN 
> >> filter/strip/insert into rte_flow finally?

Yes I think it should be the direction.
All of this can be achieved with rte_flow.


> >> if that's the plan, though I don't have much inputs for this right 
> >> now, but I think we may not need to prevent new features be added 
> >> based on current API if it does not introduce more complexity and not 
> >> break anything.

If we continue updating old API, we are just forking ourself:
having 2 APIs for the same feature is a non-sense.
We must remove APIs which are superseded by rte_flow.