diff mbox series

[v2,1/2] ethdev: add new tunnel type for ecpri

Message ID 20201224065940.76857-2-jia.guo@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers show
Series add new UDP tunnel port for ecpri | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Guo, Jia Dec. 24, 2020, 6:59 a.m. UTC
Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev tunnel type.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
---
 lib/librte_ethdev/rte_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Thomas Monjalon Jan. 6, 2021, 10:12 p.m. UTC | #1
24/12/2020 07:59, Jeff Guo:
> Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev tunnel type.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
[...]
> --- 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.

Sorry, it is a nack.
BTW, it is probably breaking the ABI because of RTE_TUNNEL_TYPE_MAX.

PS: please Cc ethdev maintainers for such patch, thanks.
tip: use --cc-cmd devtools/get-maintainer.sh
Guo, Jia Jan. 7, 2021, 9:32 a.m. UTC | #2
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, January 7, 2021 6:12 AM
> To: Guo, Jia <jia.guo@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Wang,
> Haiyue <haiyue.wang@intel.com>; dev@dpdk.org; Yigit, Ferruh
> <ferruh.yigit@intel.com>; andrew.rybchenko@oktetlabs.ru
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for
> ecpri
> 
> 24/12/2020 07:59, Jeff Guo:
> > Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev tunnel
> type.
> >
> > Signed-off-by: Jeff Guo <jia.guo@intel.com>
> > Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
> [...]
> > --- 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.

> Sorry, it is a nack.
> BTW, it is probably breaking the ABI because of RTE_TUNNEL_TYPE_MAX.
> 

Oh, the ABI break should be a problem.

> PS: please Cc ethdev maintainers for such patch, thanks.
> tip: use --cc-cmd devtools/get-maintainer.sh
> 

Thanks for your helpful tip.
Andrew Rybchenko Jan. 7, 2021, 10:09 a.m. UTC | #3
On 1/7/21 12:32 PM, Guo, Jia wrote:
> 
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Thursday, January 7, 2021 6:12 AM
>> To: Guo, Jia <jia.guo@intel.com>
>> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
>> <jingjing.wu@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Wang,
>> Haiyue <haiyue.wang@intel.com>; dev@dpdk.org; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; andrew.rybchenko@oktetlabs.ru
>> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for
>> ecpri
>>
>> 24/12/2020 07:59, Jeff Guo:
>>> Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev tunnel
>> type.
>>>
>>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>>> Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
>> [...]
>>> --- 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.

Which legacy API is kept? (I've tried to cleanup drivers, but it is not
always easy to do and I relied on driver maintainers)

If you are talking about an API to set UDP port for UDP-based tunnels,
yes it is kept right now since there is no replacement yet.

It looks like eCRPI could be encapsulated in UDP, but I don't know
if UDP port is fixed for the tunnel type or requires configuring.
Thomas Monjalon Jan. 7, 2021, 10:11 a.m. UTC | #4
07/01/2021 10:32, Guo, Jia:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 24/12/2020 07:59, Jeff Guo:
> > > Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev tunnel
> > type.
> > >
> > > Signed-off-by: Jeff Guo <jia.guo@intel.com>
> > > Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
> > [...]
> > > --- 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.
You can match, encap, decap.
There is even a new API to get tunnel infos after decap.
What is missing?


> > Sorry, it is a nack.
> > BTW, it is probably breaking the ABI because of RTE_TUNNEL_TYPE_MAX.
> 
> Oh, the ABI break should be a problem.
> 
> > PS: please Cc ethdev maintainers for such patch, thanks.
> > tip: use --cc-cmd devtools/get-maintainer.sh
> 
> Thanks for your helpful tip.
Zhang, Qi Z Jan. 7, 2021, 12:47 p.m. UTC | #5
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, January 7, 2021 6:12 PM
> To: Guo, Jia <jia.guo@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Yang, Qiming <qiming.yang@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>; dev@dpdk.org; Yigit, Ferruh
> <ferruh.yigit@intel.com>; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
> getelson@nvidia.com
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
> 
> 07/01/2021 10:32, Guo, Jia:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 24/12/2020 07:59, Jeff Guo:
> > > > Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev tunnel
> > > type.
> > > >
> > > > Signed-off-by: Jeff Guo <jia.guo@intel.com>
> > > > Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
> > > [...]
> > > > --- 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...
So I think it's not a good idea to replace the rte_eth_dev_udp_tunnel_port_add with rte_flow config
and also there is no existing rte_flow_action can cover this requirement unless we introduce some new one.

> You can match, encap, decap.
> There is even a new API to get tunnel infos after decap.
> What is missing?
> 
> 
> > > Sorry, it is a nack.
> > > BTW, it is probably breaking the ABI because of RTE_TUNNEL_TYPE_MAX.

Yes that may break the ABI but fortunately the checking-abi-compatibility tool shows negative :) , thanks Ferruh' s guide.
https://github.com/ferruhy/dpdk/actions/runs/468859673

Thanks
Qi

> >
> > Oh, the ABI break should be a problem.
> >
> > > PS: please Cc ethdev maintainers for such patch, thanks.
> > > tip: use --cc-cmd devtools/get-maintainer.sh
> >
> > Thanks for your helpful tip.
> 
>
Thomas Monjalon Jan. 7, 2021, 1:33 p.m. UTC | #6
07/01/2021 13:47, Zhang, Qi Z:
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Thursday, January 7, 2021 6:12 PM
> > To: Guo, Jia <jia.guo@intel.com>
> > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> > Yang, Qiming <qiming.yang@intel.com>; Wang, Haiyue
> > <haiyue.wang@intel.com>; dev@dpdk.org; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
> > getelson@nvidia.com
> > Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
> > 
> > 07/01/2021 10:32, Guo, Jia:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 24/12/2020 07:59, Jeff Guo:
> > > > > Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev tunnel
> > > > type.
> > > > >
> > > > > Signed-off-by: Jeff Guo <jia.guo@intel.com>
> > > > > Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > [...]
> > > > > --- 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?

> So I think it's not a good idea to replace
> the rte_eth_dev_udp_tunnel_port_add with rte_flow config
> and also there is no existing rte_flow_action
> can cover this requirement unless we introduce some new one.
> 
> > You can match, encap, decap.
> > There is even a new API to get tunnel infos after decap.
> > What is missing?

I still don't see which use case is missing.


> > > > Sorry, it is a nack.
> > > > BTW, it is probably breaking the ABI because of RTE_TUNNEL_TYPE_MAX.
> 
> Yes that may break the ABI but fortunately the checking-abi-compatibility tool shows negative :) , thanks Ferruh' s guide.
> https://github.com/ferruhy/dpdk/actions/runs/468859673

That's very strange. An enum value is changed.
Why it is not flagged by libabigail?


> > > Oh, the ABI break should be a problem.
> > >
> > > > PS: please Cc ethdev maintainers for such patch, thanks.
> > > > tip: use --cc-cmd devtools/get-maintainer.sh
> > >
> > > Thanks for your helpful tip.
David Marchand Jan. 7, 2021, 1:45 p.m. UTC | #7
On Thu, Jan 7, 2021 at 2:33 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > Yes that may break the ABI but fortunately the checking-abi-compatibility tool shows negative :) , thanks Ferruh' s guide.
> > https://github.com/ferruhy/dpdk/actions/runs/468859673
>
> That's very strange. An enum value is changed.
> Why it is not flagged by libabigail?

I suspect this is because the enum is not referenced in any object...
all I see is an integer with a comment that it should be filled with
values from the enum.
Dodji Seketeli Jan. 7, 2021, 2:27 p.m. UTC | #8
David Marchand <david.marchand@redhat.com> writes:

> On Thu, Jan 7, 2021 at 2:33 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>> > Yes that may break the ABI but fortunately the checking-abi-compatibility tool shows negative :) , thanks Ferruh' s guide.
>> > https://github.com/ferruhy/dpdk/actions/runs/468859673
>>
>> That's very strange. An enum value is changed.
>> Why it is not flagged by libabigail?
>
> I suspect this is because the enum is not referenced in any object...
> all I see is an integer with a comment that it should be filled with
> values from the enum.

I am not sure about the full context but David is right in theory.

If the enum is not reachable from a publically exported interface
(function or global variable) then it won't we considered as being part
of the ABI and changes to that enum won't be reported.

I am not sure if that is what is happening in this particular case,
though.

Cheers,
Zhang, Qi Z Jan. 7, 2021, 3:24 p.m. UTC | #9
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, January 7, 2021 9:34 PM
> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
> andrew.rybchenko@oktetlabs.ru; orika@nvidia.com; getelson@nvidia.com;
> Dodji Seketeli <dodji@redhat.com>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
> 
> 07/01/2021 13:47, Zhang, Qi Z:
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > Sent: Thursday, January 7, 2021 6:12 PM
> > > To: Guo, Jia <jia.guo@intel.com>
> > > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Wang,
> > > Haiyue <haiyue.wang@intel.com>; dev@dpdk.org; Yigit, Ferruh
> > > <ferruh.yigit@intel.com>; andrew.rybchenko@oktetlabs.ru;
> > > orika@nvidia.com; getelson@nvidia.com
> > > Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel
> > > type for ecpri
> > >
> > > 07/01/2021 10:32, Guo, Jia:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 24/12/2020 07:59, Jeff Guo:
> > > > > > Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev
> > > > > > tunnel
> > > > > type.
> > > > > >
> > > > > > Signed-off-by: Jeff Guo <jia.guo@intel.com>
> > > > > > Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > > [...]
> > > > > > --- 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."

Thanks
Qi

> 
> > So I think it's not a good idea to replace the
> > rte_eth_dev_udp_tunnel_port_add with rte_flow config and also there is
> > no existing rte_flow_action can cover this requirement unless we
> > introduce some new one.
> >
> > > You can match, encap, decap.
> > > There is even a new API to get tunnel infos after decap.
> > > What is missing?
> 
> I still don't see which use case is missing.
> 
> 
> > > > > Sorry, it is a nack.
> > > > > BTW, it is probably breaking the ABI because of
> RTE_TUNNEL_TYPE_MAX.
> >
> > Yes that may break the ABI but fortunately the checking-abi-compatibility tool
> shows negative :) , thanks Ferruh' s guide.
> > https://github.com/ferruhy/dpdk/actions/runs/468859673
> 
> That's very strange. An enum value is changed.
> Why it is not flagged by libabigail?
> 
> 
> > > > Oh, the ABI break should be a problem.
> > > >
> > > > > PS: please Cc ethdev maintainers for such patch, thanks.
> > > > > tip: use --cc-cmd devtools/get-maintainer.sh
> > > >
> > > > Thanks for your helpful tip.
> 
>
Thomas Monjalon Jan. 7, 2021, 4:58 p.m. UTC | #10
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?
Zhang, Qi Z Jan. 8, 2021, 1:41 a.m. UTC | #11
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, January 8, 2021 12:59 AM
> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
> andrew.rybchenko@oktetlabs.ru; orika@nvidia.com; getelson@nvidia.com;
> Dodji Seketeli <dodji@redhat.com>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
> 
> 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.

BTW, are we going to move all other filter like mac , VLAN filter/strip/insert into rte_flow finally?
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.


> 
> 
>
Ferruh Yigit Jan. 8, 2021, 8:57 a.m. UTC | #12
On 1/8/2021 1:41 AM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Friday, January 8, 2021 12:59 AM
>> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
>> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
>> dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
>> andrew.rybchenko@oktetlabs.ru; orika@nvidia.com; getelson@nvidia.com;
>> Dodji Seketeli <dodji@redhat.com>
>> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
>>
>> 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.
> 

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

> BTW, are we going to move all other filter like mac , VLAN filter/strip/insert into rte_flow finally?
> 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.
> 
> 
>>
>>
>>
>
Ferruh Yigit Jan. 8, 2021, 9:22 a.m. UTC | #13
On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
> 07/01/2021 13:47, Zhang, Qi Z:
>>
>>> -----Original Message-----
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>> Sent: Thursday, January 7, 2021 6:12 PM
>>> To: Guo, Jia <jia.guo@intel.com>
>>> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
>>> Yang, Qiming <qiming.yang@intel.com>; Wang, Haiyue
>>> <haiyue.wang@intel.com>; dev@dpdk.org; Yigit, Ferruh
>>> <ferruh.yigit@intel.com>; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
>>> getelson@nvidia.com
>>> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
>>>
>>> 07/01/2021 10:32, Guo, Jia:
>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>> 24/12/2020 07:59, Jeff Guo:
>>>>>> Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev tunnel
>>>>> type.
>>>>>>
>>>>>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>>>>>> Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
>>>>> [...]
>>>>>> --- 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?
> 
>> So I think it's not a good idea to replace
>> the rte_eth_dev_udp_tunnel_port_add with rte_flow config
>> and also there is no existing rte_flow_action
>> can cover this requirement unless we introduce some new one.
>>
>>> You can match, encap, decap.
>>> There is even a new API to get tunnel infos after decap.
>>> What is missing?
> 
> I still don't see which use case is missing.
> 
> 
>>>>> Sorry, it is a nack.
>>>>> BTW, it is probably breaking the ABI because of RTE_TUNNEL_TYPE_MAX.
>>
>> Yes that may break the ABI but fortunately the checking-abi-compatibility tool shows negative :) , thanks Ferruh' s guide.
>> https://github.com/ferruhy/dpdk/actions/runs/468859673
> 
> That's very strange. An enum value is changed.
> Why it is not flagged by libabigail?
> 

As long as the enum values not sent to the application and kept within the 
library, changing their values shouldn't be problem.

> 
>>>> Oh, the ABI break should be a problem.
>>>>
>>>>> PS: please Cc ethdev maintainers for such patch, thanks.
>>>>> tip: use --cc-cmd devtools/get-maintainer.sh
>>>>
>>>> Thanks for your helpful tip.
> 
> 
>
Andrew Rybchenko Jan. 8, 2021, 9:29 a.m. UTC | #14
On 1/8/21 11:57 AM, Ferruh Yigit wrote:
> On 1/8/2021 1:41 AM, Zhang, Qi Z wrote:
>>
>>
>>> -----Original Message-----
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>> Sent: Friday, January 8, 2021 12:59 AM
>>> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
>>> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
>>> dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
>>> andrew.rybchenko@oktetlabs.ru; orika@nvidia.com; getelson@nvidia.com;
>>> Dodji Seketeli <dodji@redhat.com>
>>> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel 
>>> type for ecpri
>>>
>>> 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.
>>
>
> Isn't this more a device configuration than filtering, not sure about 
> using rte_flow for this.

+1

>> BTW, are we going to move all other filter like mac , VLAN 
>> filter/strip/insert into rte_flow finally?
>> 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.
Thomas Monjalon Jan. 8, 2021, 10:23 a.m. UTC | #15
08/01/2021 10:22, Ferruh Yigit:
> On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
> > 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>
> >>>>> Sorry, it is a nack.
> >>>>> BTW, it is probably breaking the ABI because of RTE_TUNNEL_TYPE_MAX.
> >>
> >> Yes that may break the ABI but fortunately the checking-abi-compatibility tool shows negative :) , thanks Ferruh' s guide.
> >> https://github.com/ferruhy/dpdk/actions/runs/468859673
> > 
> > That's very strange. An enum value is changed.
> > Why it is not flagged by libabigail?
> 
> As long as the enum values not sent to the application and kept within the 
> library, changing their values shouldn't be problem.

But RTE_TUNNEL_TYPE_MAX is part of lib/librte_ethdev/rte_ethdev.h
so it is exposed to the application.
I think it is a case of ABI breakage.
Thomas Monjalon Jan. 8, 2021, 10:36 a.m. UTC | #16
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.
Ferruh Yigit Jan. 8, 2021, 10:43 a.m. UTC | #17
On 1/8/2021 10:23 AM, Thomas Monjalon wrote:
> 08/01/2021 10:22, Ferruh Yigit:
>> On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
>>> 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>
>>>>>>> Sorry, it is a nack.
>>>>>>> BTW, it is probably breaking the ABI because of RTE_TUNNEL_TYPE_MAX.
>>>>
>>>> Yes that may break the ABI but fortunately the checking-abi-compatibility tool shows negative :) , thanks Ferruh' s guide.
>>>> https://github.com/ferruhy/dpdk/actions/runs/468859673
>>>
>>> That's very strange. An enum value is changed.
>>> Why it is not flagged by libabigail?
>>
>> As long as the enum values not sent to the application and kept within the
>> library, changing their values shouldn't be problem.
> 
> But RTE_TUNNEL_TYPE_MAX is part of lib/librte_ethdev/rte_ethdev.h
> so it is exposed to the application.
> I think it is a case of ABI breakage.
> 

Yes it is exposed to the application. But in runtime does it exchanged between 
library and application is the issue I think.
For this case it seems it is not, so not an ABI break.
Kinsella, Ray Jan. 8, 2021, 12:38 p.m. UTC | #18
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday 8 January 2021 10:24
> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
> getelson@nvidia.com; Dodji Seketeli <dodji@redhat.com>; Kinsella, Ray
> <ray.kinsella@intel.com>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type
> for ecpri
> 
> 08/01/2021 10:22, Ferruh Yigit:
> > On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
> > > 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>
> > >>>>> Sorry, it is a nack.
> > >>>>> BTW, it is probably breaking the ABI because of
> RTE_TUNNEL_TYPE_MAX.
> > >>
> > >> Yes that may break the ABI but fortunately the checking-abi-
> compatibility tool shows negative :) , thanks Ferruh' s guide.
> > >> https://github.com/ferruhy/dpdk/actions/runs/468859673
> > >
> > > That's very strange. An enum value is changed.
> > > Why it is not flagged by libabigail?
> >
> > As long as the enum values not sent to the application and kept
> within
> > the library, changing their values shouldn't be problem.
> 
> But RTE_TUNNEL_TYPE_MAX is part of lib/librte_ethdev/rte_ethdev.h so it
> is exposed to the application.
> I think it is a case of ABI breakage.
> 

Really a lot depends on context, Thomas is right it is hard to predict how these _MAX values are used.

We have seen cases in the past where _MAX enumeration values have been used to size arrays the like - I don't immediately see that issue here. My understanding is that the only consumer of this enumeration is rte_eth_dev_udp_tunnel_port_add and rte_eth_dev_udp_tunnel_port_delete, right? On face value, impact looks negligible. 

I will take a look at why libabigail doesn't complain.
Thomas Monjalon Jan. 8, 2021, 2:06 p.m. UTC | #19
08/01/2021 11:43, Ferruh Yigit:
> On 1/8/2021 10:23 AM, Thomas Monjalon wrote:
> > 08/01/2021 10:22, Ferruh Yigit:
> >> On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
> >>> 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>
> >>>>>>> Sorry, it is a nack.
> >>>>>>> BTW, it is probably breaking the ABI because of RTE_TUNNEL_TYPE_MAX.
> >>>>
> >>>> Yes that may break the ABI but fortunately the checking-abi-compatibility tool shows negative :) , thanks Ferruh' s guide.
> >>>> https://github.com/ferruhy/dpdk/actions/runs/468859673
> >>>
> >>> That's very strange. An enum value is changed.
> >>> Why it is not flagged by libabigail?
> >>
> >> As long as the enum values not sent to the application and kept within the
> >> library, changing their values shouldn't be problem.
> > 
> > But RTE_TUNNEL_TYPE_MAX is part of lib/librte_ethdev/rte_ethdev.h
> > so it is exposed to the application.
> > I think it is a case of ABI breakage.
> 
> Yes it is exposed to the application. But in runtime does it exchanged between 
> library and application is the issue I think.
> For this case it seems it is not, so not an ABI break.

If I create a table of size RTE_TUNNEL_TYPE_MAX with DPDK 20.11,
I will get an overflow when writing to the new ECPRI index.
The question is: can I receive the ECPRI value dynamically from ethdev?
If yes, it is an ABI breakage. But I cannot think of such case now.
Kinsella, Ray Jan. 8, 2021, 2:07 p.m. UTC | #20
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday 8 January 2021 14:06
> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
> getelson@nvidia.com; Dodji Seketeli <dodji@redhat.com>; Kinsella, Ray
> <ray.kinsella@intel.com>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type
> for ecpri
> 
> 08/01/2021 11:43, Ferruh Yigit:
> > On 1/8/2021 10:23 AM, Thomas Monjalon wrote:
> > > 08/01/2021 10:22, Ferruh Yigit:
> > >> On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
> > >>> 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>
> > >>>>>>> Sorry, it is a nack.
> > >>>>>>> BTW, it is probably breaking the ABI because of
> RTE_TUNNEL_TYPE_MAX.
> > >>>>
> > >>>> Yes that may break the ABI but fortunately the checking-abi-
> compatibility tool shows negative :) , thanks Ferruh' s guide.
> > >>>> https://github.com/ferruhy/dpdk/actions/runs/468859673
> > >>>
> > >>> That's very strange. An enum value is changed.
> > >>> Why it is not flagged by libabigail?
> > >>
> > >> As long as the enum values not sent to the application and kept
> > >> within the library, changing their values shouldn't be problem.
> > >
> > > But RTE_TUNNEL_TYPE_MAX is part of lib/librte_ethdev/rte_ethdev.h
> so
> > > it is exposed to the application.
> > > I think it is a case of ABI breakage.
> >
> > Yes it is exposed to the application. But in runtime does it
> exchanged
> > between library and application is the issue I think.
> > For this case it seems it is not, so not an ABI break.
> 
> If I create a table of size RTE_TUNNEL_TYPE_MAX with DPDK 20.11, I will
> get an overflow when writing to the new ECPRI index.

I guess the question is - are you likely to do this?

> The question is: can I receive the ECPRI value dynamically from ethdev?
> If yes, it is an ABI breakage. But I cannot think of such case now.

Ray K
Thomas Monjalon Jan. 8, 2021, 2:10 p.m. UTC | #21
08/01/2021 15:07, Kinsella, Ray:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 08/01/2021 11:43, Ferruh Yigit:
> > > On 1/8/2021 10:23 AM, Thomas Monjalon wrote:
> > > > 08/01/2021 10:22, Ferruh Yigit:
> > > >> On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
> > > >>> 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>
> > > >>>>>>> Sorry, it is a nack.
> > > >>>>>>> BTW, it is probably breaking the ABI because of
> > RTE_TUNNEL_TYPE_MAX.
> > > >>>>
> > > >>>> Yes that may break the ABI but fortunately the checking-abi-
> > compatibility tool shows negative :) , thanks Ferruh' s guide.
> > > >>>> https://github.com/ferruhy/dpdk/actions/runs/468859673
> > > >>>
> > > >>> That's very strange. An enum value is changed.
> > > >>> Why it is not flagged by libabigail?
> > > >>
> > > >> As long as the enum values not sent to the application and kept
> > > >> within the library, changing their values shouldn't be problem.
> > > >
> > > > But RTE_TUNNEL_TYPE_MAX is part of lib/librte_ethdev/rte_ethdev.h
> > so
> > > > it is exposed to the application.
> > > > I think it is a case of ABI breakage.
> > >
> > > Yes it is exposed to the application. But in runtime does it
> > exchanged
> > > between library and application is the issue I think.
> > > For this case it seems it is not, so not an ABI break.
> > 
> > If I create a table of size RTE_TUNNEL_TYPE_MAX with DPDK 20.11, I will
> > get an overflow when writing to the new ECPRI index.
> 
> I guess the question is - are you likely to do this?

As said below, no I cannot think about such a case myself.

> > The question is: can I receive the ECPRI value dynamically from ethdev?
> > If yes, it is an ABI breakage. But I cannot think of such case now.
Ferruh Yigit Jan. 8, 2021, 2:27 p.m. UTC | #22
On 1/8/2021 12:38 PM, Kinsella, Ray wrote:
> 
> 
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Friday 8 January 2021 10:24
>> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
>> Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
>> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
>> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
>> getelson@nvidia.com; Dodji Seketeli <dodji@redhat.com>; Kinsella, Ray
>> <ray.kinsella@intel.com>
>> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type
>> for ecpri
>>
>> 08/01/2021 10:22, Ferruh Yigit:
>>> On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
>>>> 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>
>>>>>>>> Sorry, it is a nack.
>>>>>>>> BTW, it is probably breaking the ABI because of
>> RTE_TUNNEL_TYPE_MAX.
>>>>>
>>>>> Yes that may break the ABI but fortunately the checking-abi-
>> compatibility tool shows negative :) , thanks Ferruh' s guide.
>>>>> https://github.com/ferruhy/dpdk/actions/runs/468859673
>>>>
>>>> That's very strange. An enum value is changed.
>>>> Why it is not flagged by libabigail?
>>>
>>> As long as the enum values not sent to the application and kept
>> within
>>> the library, changing their values shouldn't be problem.
>>
>> But RTE_TUNNEL_TYPE_MAX is part of lib/librte_ethdev/rte_ethdev.h so it
>> is exposed to the application.
>> I think it is a case of ABI breakage.
>>
> 
> Really a lot depends on context, Thomas is right it is hard to predict how these _MAX values are used.
> 
> We have seen cases in the past where _MAX enumeration values have been used to size arrays the like - I don't immediately see that issue here. My understanding is that the only consumer of this enumeration is rte_eth_dev_udp_tunnel_port_add and rte_eth_dev_udp_tunnel_port_delete, right? On face value, impact looks negligible.
> 
> I will take a look at why libabigail doesn't complain.
> 

Application can use the enum, including MAX as they desire, we can't really 
assume anything there.

In previous case, library was providing an enum value back to application. And 
the problem was application can use those values blindly and new unexpected 
values may cause trouble.

For this case, even the application create a table with RTE_TUNNEL_TYPE_MAX 
size, library is not sending any type of this enum to application to cause any 
problem, at least abigail seems not able to finding any instance of it.
Kinsella, Ray Jan. 8, 2021, 2:31 p.m. UTC | #23
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday 8 January 2021 14:27
> To: Kinsella, Ray <ray.kinsella@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
> getelson@nvidia.com; Dodji Seketeli <dodji@redhat.com>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type
> for ecpri
> 
> On 1/8/2021 12:38 PM, Kinsella, Ray wrote:
> >
> >
> >> -----Original Message-----
> >> From: Thomas Monjalon <thomas@monjalon.net>
> >> Sent: Friday 8 January 2021 10:24
> >> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>;
> >> Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> >> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> >> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
> >> getelson@nvidia.com; Dodji Seketeli <dodji@redhat.com>; Kinsella,
> Ray
> >> <ray.kinsella@intel.com>
> >> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel
> type
> >> for ecpri
> >>
> >> 08/01/2021 10:22, Ferruh Yigit:
> >>> On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
> >>>> 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>
> >>>>>>>> Sorry, it is a nack.
> >>>>>>>> BTW, it is probably breaking the ABI because of
> >> RTE_TUNNEL_TYPE_MAX.
> >>>>>
> >>>>> Yes that may break the ABI but fortunately the checking-abi-
> >> compatibility tool shows negative :) , thanks Ferruh' s guide.
> >>>>> https://github.com/ferruhy/dpdk/actions/runs/468859673
> >>>>
> >>>> That's very strange. An enum value is changed.
> >>>> Why it is not flagged by libabigail?
> >>>
> >>> As long as the enum values not sent to the application and kept
> >> within
> >>> the library, changing their values shouldn't be problem.
> >>
> >> But RTE_TUNNEL_TYPE_MAX is part of lib/librte_ethdev/rte_ethdev.h so
> >> it is exposed to the application.
> >> I think it is a case of ABI breakage.
> >>
> >
> > Really a lot depends on context, Thomas is right it is hard to
> predict how these _MAX values are used.
> >
> > We have seen cases in the past where _MAX enumeration values have
> been used to size arrays the like - I don't immediately see that issue
> here. My understanding is that the only consumer of this enumeration is
> rte_eth_dev_udp_tunnel_port_add and rte_eth_dev_udp_tunnel_port_delete,
> right? On face value, impact looks negligible.
> >
> > I will take a look at why libabigail doesn't complain.
> >
> 
> Application can use the enum, including MAX as they desire, we can't
> really assume anything there.
> 
> In previous case, library was providing an enum value back to
> application. And the problem was application can use those values
> blindly and new unexpected values may cause trouble.
> 
> For this case, even the application create a table with
> RTE_TUNNEL_TYPE_MAX size, library is not sending any type of this enum
> to application to cause any problem, at least abigail seems not able to
> finding any instance of it.

Yes - it makes any problem associated with unlikely then.

Ray K
Kinsella, Ray Jan. 8, 2021, 5:34 p.m. UTC | #24
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday 8 January 2021 14:27
> To: Kinsella, Ray <ray.kinsella@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
> getelson@nvidia.com; Dodji Seketeli <dodji@redhat.com>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type
> for ecpri
> 
> On 1/8/2021 12:38 PM, Kinsella, Ray wrote:
> >
> >
> >> -----Original Message-----
> >> From: Thomas Monjalon <thomas@monjalon.net>
> >> Sent: Friday 8 January 2021 10:24
> >> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>;
> >> Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> >> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> >> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
> >> getelson@nvidia.com; Dodji Seketeli <dodji@redhat.com>; Kinsella,
> Ray
> >> <ray.kinsella@intel.com>
> >> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel
> type
> >> for ecpri
> >>
> >> 08/01/2021 10:22, Ferruh Yigit:
> >>> On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
> >>>> 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>
> >>>>>>>> Sorry, it is a nack.
> >>>>>>>> BTW, it is probably breaking the ABI because of
> >> RTE_TUNNEL_TYPE_MAX.
> >>>>>
> >>>>> Yes that may break the ABI but fortunately the checking-abi-
> >> compatibility tool shows negative :) , thanks Ferruh' s guide.
> >>>>> https://github.com/ferruhy/dpdk/actions/runs/468859673
> >>>>
> >>>> That's very strange. An enum value is changed.
> >>>> Why it is not flagged by libabigail?
> >>>
> >>> As long as the enum values not sent to the application and kept
> >> within
> >>> the library, changing their values shouldn't be problem.
> >>
> >> But RTE_TUNNEL_TYPE_MAX is part of lib/librte_ethdev/rte_ethdev.h so
> >> it is exposed to the application.
> >> I think it is a case of ABI breakage.
> >>
> >
> > Really a lot depends on context, Thomas is right it is hard to
> predict how these _MAX values are used.
> >
> > We have seen cases in the past where _MAX enumeration values have
> been used to size arrays the like - I don't immediately see that issue
> here. My understanding is that the only consumer of this enumeration is
> rte_eth_dev_udp_tunnel_port_add and rte_eth_dev_udp_tunnel_port_delete,
> right? On face value, impact looks negligible.
> >
> > I will take a look at why libabigail doesn't complain.

So I spent some time looking a bit closer at why libabigail didn't complain,
I am summarizing it is because there is no symbol that obviously uses enum rte_eth_tunnel_type.
The prot_type field in rte_eth_udp_tunnel is declared as a uint8_t, not as enum rte_eth_tunnel_type.

Is there a particular reason an enumerated field would be declared as unsigned int instead?

/**
 * UDP tunneling configuration.
 * Used to config the UDP port for a type of tunnel.
 * NICs need the UDP port to identify the tunnel type.
 * Normally a type of tunnel has a default UDP port, this structure can be used
 * in case if the users want to change or support more UDP port.
 */
struct rte_eth_udp_tunnel {
        uint16_t udp_port; /**< UDP port used for the tunnel. */
        uint8_t prot_type; /**< Tunnel type. Defined in rte_eth_tunnel_type. */
};

> 
> Application can use the enum, including MAX as they desire, we can't
> really assume anything there.
> 
> In previous case, library was providing an enum value back to
> application. And the problem was application can use those values
> blindly and new unexpected values may cause trouble.
> 
> For this case, even the application create a table with
> RTE_TUNNEL_TYPE_MAX size, library is not sending any type of this enum
> to application to cause any problem, at least abigail seems not able to
> finding any instance of it.

I agree - I think this has marginal risk.
Zhang, Qi Z Jan. 9, 2021, 1:01 a.m. UTC | #25
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, January 8, 2021 6:36 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Guo, Jia <jia.guo@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; orika@nvidia.com; getelson@nvidia.com
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
> 
> 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.

Its not about the pattern match, it's about the action, what we need is a rte_flow action to "define a packet's tunnel type", but we don't have.

#flow create eth/ipv4/udp port is 4789/... action set_tunnel_type VxLAN

I think rte_eth_dev_udp_tunnel_port_add does this job well already, if we plan to move it to rte_flow, at least we need a replacement solution.

> 
> 
> > > 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.
>
Ori Kam Jan. 10, 2021, 10:46 a.m. UTC | #26
Hi,

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Saturday, January 9, 2021 3:01 AM
> Subject: RE: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for
> ecpri
> >
> > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftools.ietf
> .org%2Fhtml%2Frfc7348&amp;data=04%7C01%7Corika%40nvidia.com%7C46b2
> d8f48944422f0d9008d8b43a2293%7C43083d15727340c1b7db39efd9ccc17a%7
> C0%7C0%7C637457509081543237%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&a
> mp;sdata=RYWFMjuxkcUZ982kK2s44tBAjf%2FTkDyaa7VEybCtxOo%3D&amp;res
> erved=0).
> > > >>>>
> > > >>>> "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.
> 
> Its not about the pattern match, it's about the action, what we need is a
> rte_flow action to "define a packet's tunnel type", but we don't have.
> 
> #flow create eth/ipv4/udp port is 4789/... action set_tunnel_type VxLAN
> 
> I think rte_eth_dev_udp_tunnel_port_add does this job well already, if we plan
> to move it to rte_flow, at least we need a replacement solution.
> 

Let me see if I understand it correctly.
In your case, the issue is that you need to configure the HW to parse the packet correctly right?
It is not about the matching it is about the configuration of the HW, you wish to tell
the HW that the packet should be parsed by different means correct?

If this is the case it sounds to me that you should use rte_flow and if the 
user adds the following rule:
#flow create pattern eth / ivp4 / udp port is 4789 / .. action .....
You simply need to configure your HW to support the ecpri configuration.


> >
> >
> > > > 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.
> >
Thomas Monjalon Jan. 11, 2021, 9:23 a.m. UTC | #27
10/01/2021 11:46, Ori Kam:
> Hi,
> 
> > -----Original Message-----
> > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Sent: Saturday, January 9, 2021 3:01 AM
> > Subject: RE: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for
> > ecpri
> > >
> > > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftools.ietf
> > .org%2Fhtml%2Frfc7348&amp;data=04%7C01%7Corika%40nvidia.com%7C46b2
> > d8f48944422f0d9008d8b43a2293%7C43083d15727340c1b7db39efd9ccc17a%7
> > C0%7C0%7C637457509081543237%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&a
> > mp;sdata=RYWFMjuxkcUZ982kK2s44tBAjf%2FTkDyaa7VEybCtxOo%3D&amp;res
> > erved=0).
> > > > >>>>
> > > > >>>> "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.
> > 
> > Its not about the pattern match, it's about the action, what we need is a
> > rte_flow action to "define a packet's tunnel type", but we don't have.

A packet type alone is meaningless.
It is always associated to an action, this is what rte_flow does.


> > #flow create eth/ipv4/udp port is 4789/... action set_tunnel_type VxLAN
> > 
> > I think rte_eth_dev_udp_tunnel_port_add does this job well already, if we plan
> > to move it to rte_flow, at least we need a replacement solution.

The documentation does not say why it is useful.
With rte_flow you don't need it because a flow is specified with its action.


> Let me see if I understand it correctly.
> In your case, the issue is that you need to configure the HW to parse the packet correctly right?
> It is not about the matching it is about the configuration of the HW, you wish to tell
> the HW that the packet should be parsed by different means correct?
> 
> If this is the case it sounds to me that you should use rte_flow and if the 
> user adds the following rule:
> #flow create pattern eth / ivp4 / udp port is 4789 / .. action .....
> You simply need to configure your HW to support the ecpri configuration.
> 
> 
> > >
> > >
> > > > > 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.
> > >
> 
>
Zhang, Qi Z Jan. 11, 2021, 11:26 a.m. UTC | #28
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, January 11, 2021 5:24 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Guo, Jia <jia.guo@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ori Kam <orika@nvidia.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; Gregory Etelson <getelson@nvidia.com>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
> 
> 10/01/2021 11:46, Ori Kam:
> > Hi,
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Sent: Saturday, January 9, 2021 3:01 AM
> > > Subject: RE: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel
> > > type for ecpri
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel
> > > > type for
> > > ecpri
> > > >
> > > > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fto
> > > ols.ietf
> > > .org%2Fhtml%2Frfc7348&amp;data=04%7C01%7Corika%40nvidia.com%7C
> 46b2
> > >
> d8f48944422f0d9008d8b43a2293%7C43083d15727340c1b7db39efd9ccc17a
> %7
> > >
> C0%7C0%7C637457509081543237%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC
> > >
> 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&a
> > >
> mp;sdata=RYWFMjuxkcUZ982kK2s44tBAjf%2FTkDyaa7VEybCtxOo%3D&amp;r
> es
> > > erved=0).
> > > > > >>>>
> > > > > >>>> "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.
> > >
> > > Its not about the pattern match, it's about the action, what we need
> > > is a rte_flow action to "define a packet's tunnel type", but we don't have.
> 
> A packet type alone is meaningless.
> It is always associated to an action, this is what rte_flow does.

As I mentioned in previous, this is a device (port) level configuration, so it can only be configured by a PF driver or a privileged VF base on our security model.
A typical usage in a NFV environment could be:

1. A privileged VF (e.g. ice_dcf PMD) use rte_eth_dev_udp_tunnel_port_add to create tunnel port for eCPRI, them this will impact on all VFs in the same PF.
2. A normal VF driver can create rte_flow rule that match specific patch for queue steering or apply RSS for eCPRI packets, but it DON'T have the permission to define the tunnel port.

So it does not help to have a rte_flow that match dst udp port as tunnel while have an action in the same rule.


> 
> 
> > > #flow create eth/ipv4/udp port is 4789/... action set_tunnel_type
> > > VxLAN
> > >
> > > I think rte_eth_dev_udp_tunnel_port_add does this job well already,
> > > if we plan to move it to rte_flow, at least we need a replacement solution.
> 
> The documentation does not say why it is useful.
> With rte_flow you don't need it because a flow is specified with its action.
> 
> 
> > Let me see if I understand it correctly.
> > In your case, the issue is that you need to configure the HW to parse the
> packet correctly right?
> > It is not about the matching it is about the configuration of the HW,
> > you wish to tell the HW that the packet should be parsed by different means
> correct?
> >
> > If this is the case it sounds to me that you should use rte_flow and
> > if the user adds the following rule:
> > #flow create pattern eth / ivp4 / udp port is 4789 / .. action .....
> > You simply need to configure your HW to support the ecpri configuration.
> >
> >
> > > >
> > > >
> > > > > > 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.
> > > >
> >
> >
> 
> 
> 
>
Thomas Monjalon Jan. 11, 2021, 11:37 a.m. UTC | #29
11/01/2021 12:26, Zhang, Qi Z:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 10/01/2021 11:46, Ori Kam:
> > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > From: 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fto
> > > > ols.ietf
> > > > .org%2Fhtml%2Frfc7348&amp;data=04%7C01%7Corika%40nvidia.com%7C
> > 46b2
> > > >
> > d8f48944422f0d9008d8b43a2293%7C43083d15727340c1b7db39efd9ccc17a
> > %7
> > > >
> > C0%7C0%7C637457509081543237%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> > MC
> > > >
> > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&a
> > > >
> > mp;sdata=RYWFMjuxkcUZ982kK2s44tBAjf%2FTkDyaa7VEybCtxOo%3D&amp;r
> > es
> > > > erved=0).
> > > > > > >>>>
> > > > > > >>>> "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.
> > > >
> > > > Its not about the pattern match, it's about the action, what we need
> > > > is a rte_flow action to "define a packet's tunnel type", but we don't have.
> > 
> > A packet type alone is meaningless.
> > It is always associated to an action, this is what rte_flow does.
> 
> As I mentioned in previous, this is a device (port) level configuration, so it can only be configured by a PF driver or a privileged VF base on our security model.
> A typical usage in a NFV environment could be:
> 
> 1. A privileged VF (e.g. ice_dcf PMD) use rte_eth_dev_udp_tunnel_port_add to create tunnel port for eCPRI, them this will impact on all VFs in the same PF.
> 2. A normal VF driver can create rte_flow rule that match specific patch for queue steering or apply RSS for eCPRI packets, but it DON'T have the permission to define the tunnel port.

Whaooh! A normal Intel VF is not allowed to match the tunnel it wants
if not enabled by a priviledged VF?
I would say it is a HW design flaw, but that's not the question.

> So it does not help to have a rte_flow that match dst udp port
> as tunnel while have an action in the same rule.

Now I understand you are truly looking for a device configuration.
But as it looks more as a HW limitation,
I don't think it should be part of ethdev.
The fact is that it is already part of ethdev...

I don't know what to say. I need to digest how Intel limitation
is "still" trying to drive some parts of the "generic" API.


> > > > #flow create eth/ipv4/udp port is 4789/... action set_tunnel_type
> > > > VxLAN
> > > >
> > > > I think rte_eth_dev_udp_tunnel_port_add does this job well already,
> > > > if we plan to move it to rte_flow, at least we need a replacement solution.
> > 
> > The documentation does not say why it is useful.
> > With rte_flow you don't need it because a flow is specified with its action.
> > 
> > 
> > > Let me see if I understand it correctly.
> > > In your case, the issue is that you need to configure the HW to parse the
> > packet correctly right?
> > > It is not about the matching it is about the configuration of the HW,
> > > you wish to tell the HW that the packet should be parsed by different means
> > correct?
> > >
> > > If this is the case it sounds to me that you should use rte_flow and
> > > if the user adds the following rule:
> > > #flow create pattern eth / ivp4 / udp port is 4789 / .. action .....
> > > You simply need to configure your HW to support the ecpri configuration.
> > >
> > >
> > > > >
> > > > >
> > > > > > > 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.
Zhang, Qi Z Jan. 11, 2021, 2:02 p.m. UTC | #30
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, January 11, 2021 7:38 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Guo, Jia <jia.guo@intel.com>; Zhang,
> Qi Z <qi.z.zhang@intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Ori Kam
> <orika@nvidia.com>; Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; Gregory Etelson <getelson@nvidia.com>;
> maxime.coquelin@redhat.com; jerinj@marvell.com;
> ajit.khaparde@broadcom.com
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
> 
> 11/01/2021 12:26, Zhang, Qi Z:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 10/01/2021 11:46, Ori Kam:
> > > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > From: 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%
> > > > > 2Fto
> > > > > ols.ietf
> > > > > .org%2Fhtml%2Frfc7348&amp;data=04%7C01%7Corika%40nvidia.com
> %7C
> > > 46b2
> > > > >
> > >
> d8f48944422f0d9008d8b43a2293%7C43083d15727340c1b7db39efd9ccc17a
> > > %7
> > > > >
> > >
> C0%7C0%7C637457509081543237%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> > > MC
> > > > >
> > >
> 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&a
> > > > >
> > >
> mp;sdata=RYWFMjuxkcUZ982kK2s44tBAjf%2FTkDyaa7VEybCtxOo%3D&amp;r
> > > es
> > > > > erved=0).
> > > > > > > >>>>
> > > > > > > >>>> "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.
> > > > >
> > > > > Its not about the pattern match, it's about the action, what we
> > > > > need is a rte_flow action to "define a packet's tunnel type", but we don't
> have.
> > >
> > > A packet type alone is meaningless.
> > > It is always associated to an action, this is what rte_flow does.
> >
> > As I mentioned in previous, this is a device (port) level configuration, so it can
> only be configured by a PF driver or a privileged VF base on our security model.
> > A typical usage in a NFV environment could be:
> >
> > 1. A privileged VF (e.g. ice_dcf PMD) use rte_eth_dev_udp_tunnel_port_add
> to create tunnel port for eCPRI, them this will impact on all VFs in the same PF.
> > 2. A normal VF driver can create rte_flow rule that match specific patch for
> queue steering or apply RSS for eCPRI packets, but it DON'T have the
> permission to define the tunnel port.
> 
> Whaooh! A normal Intel VF is not allowed to match the tunnel it wants if not
> enabled by a priviledged VF?

> I would say it is a HW design flaw, but that's not the question.	

Why you think this is a design flaw? in real case, is it a typical requirement that different VF need different tunnel port for eCPRI (or VxLan) on the same PF? 
I believe it's not necessary to make it as a per VF resource in most cases, and I will be surprise if a driver that allow any VF to change the share resource without any privilege control.

Btw I guess mlx NIC has more flexible way to handle ecpri tunnel, just curious how it works, what's the expected result of below rules?

1. create flow eth / ipv4 / udp dst is 1234 / ecpri msgtype is 0 / ... to queue 0
2. create flow eth / ipv4 / udp dst is 5678 / ecrpi msgtype is 1 / ... to queue 1.

So both 1234 and 5678 will be regarded as an ECPRI packet? Or only the first one will work?
does dst udp port is always needed if an ecpri pattern is involved?


> 
> > So it does not help to have a rte_flow that match dst udp port as
> > tunnel while have an action in the same rule.
> 
> Now I understand you are truly looking for a device configuration.
> But as it looks more as a HW limitation, I don't think it should be part of ethdev.
> The fact is that it is already part of ethdev...
> 
> I don't know what to say. I need to digest how Intel limitation is "still" trying to
> drive some parts of the "generic" API.
> 
> 
> > > > > #flow create eth/ipv4/udp port is 4789/... action
> > > > > set_tunnel_type VxLAN
> > > > >
> > > > > I think rte_eth_dev_udp_tunnel_port_add does this job well
> > > > > already, if we plan to move it to rte_flow, at least we need a replacement
> solution.
> > >
> > > The documentation does not say why it is useful.
> > > With rte_flow you don't need it because a flow is specified with its action.
> > >
> > >
> > > > Let me see if I understand it correctly.
> > > > In your case, the issue is that you need to configure the HW to
> > > > parse the
> > > packet correctly right?
> > > > It is not about the matching it is about the configuration of the
> > > > HW, you wish to tell the HW that the packet should be parsed by
> > > > different means
> > > correct?
> > > >
> > > > If this is the case it sounds to me that you should use rte_flow
> > > > and if the user adds the following rule:
> > > > #flow create pattern eth / ivp4 / udp port is 4789 / .. action .....
> > > > You simply need to configure your HW to support the ecpri configuration.
> > > >
> > > >
> > > > > >
> > > > > >
> > > > > > > > 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.
> 
>
Thomas Monjalon Jan. 11, 2021, 2:53 p.m. UTC | #31
11/01/2021 15:02, Zhang, Qi Z:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 11/01/2021 12:26, Zhang, Qi Z:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 10/01/2021 11:46, Ori Kam:
> > > > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > > From: 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>
> > > > > > > > >>> 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.
> > > > > >
> > > > > > Its not about the pattern match, it's about the action, what we
> > > > > > need is a rte_flow action to "define a packet's tunnel type", but we don't
> > have.
> > > >
> > > > A packet type alone is meaningless.
> > > > It is always associated to an action, this is what rte_flow does.
> > >
> > > As I mentioned in previous, this is a device (port) level configuration, so it can
> > only be configured by a PF driver or a privileged VF base on our security model.
> > > A typical usage in a NFV environment could be:
> > >
> > > 1. A privileged VF (e.g. ice_dcf PMD) use rte_eth_dev_udp_tunnel_port_add
> > to create tunnel port for eCPRI, them this will impact on all VFs in the same PF.
> > > 2. A normal VF driver can create rte_flow rule that match specific patch for
> > queue steering or apply RSS for eCPRI packets, but it DON'T have the
> > permission to define the tunnel port.
> > 
> > Whaooh! A normal Intel VF is not allowed to match the tunnel it wants if not
> > enabled by a priviledged VF?
> 
> > I would say it is a HW design flaw, but that's not the question.	
> 
> Why you think this is a design flaw? in real case,
> is it a typical requirement that different VF
> need different tunnel port for eCPRI (or VxLan) on the same PF?

They are different VFs, so why should they use the same UDP port?
Anyway it doesn't need to be typical to be allowed.

> I believe it's not necessary to make it as a per VF resource
> in most cases, and I will be surprise if a driver that
> allow any VF to change the share resource without any privilege control.

The thing is that a flow rule should not be a shared resource.
In Intel devices, it seems the UDP port of a protocol is supposed
to be shared with all VFs, but it looks a very specific assumption,
or limitation.
I wonder how we can document this and ask the user to call
rte_eth_dev_udp_tunnel_port_add(), because of some devices.
Anyway, this is currently poorly documented.

> Btw I guess mlx NIC has more flexible way to handle ecpri tunnel,
> just curious how it works, what's the expected result of below rules?
> 
> 1. create flow eth / ipv4 / udp dst is 1234 / ecpri msgtype is 0 / ... to queue 0
> 2. create flow eth / ipv4 / udp dst is 5678 / ecrpi msgtype is 1 / ... to queue 1.

It should move the eCPRI packets to the right queue,
taking into consideration the UDP port and the message type.
Of course there may be some bugs :)

> So both 1234 and 5678 will be regarded as an ECPRI packet?

Yes, both should be considered eCPRI.

> Or only the first one will work?

I am not aware of such limitation.

> does dst udp port is always needed if an ecpri pattern is involved?

No, the UDP part is optional.
Zhang, Qi Z Jan. 12, 2021, 2:14 a.m. UTC | #32
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, January 11, 2021 10:54 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Guo, Jia <jia.guo@intel.com>; Zhang,
> Qi Z <qi.z.zhang@intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Ori Kam
> <orika@nvidia.com>; Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; Gregory Etelson <getelson@nvidia.com>;
> maxime.coquelin@redhat.com; jerinj@marvell.com;
> ajit.khaparde@broadcom.com; Bing Zhao <bingz@nvidia.com>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri
> 
> 11/01/2021 15:02, Zhang, Qi Z:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 11/01/2021 12:26, Zhang, Qi Z:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 10/01/2021 11:46, Ori Kam:
> > > > > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > > > From: 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>
> > > > > > > > > >>> 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.
> > > > > > >
> > > > > > > Its not about the pattern match, it's about the action, what
> > > > > > > we need is a rte_flow action to "define a packet's tunnel
> > > > > > > type", but we don't
> > > have.
> > > > >
> > > > > A packet type alone is meaningless.
> > > > > It is always associated to an action, this is what rte_flow does.
> > > >
> > > > As I mentioned in previous, this is a device (port) level
> > > > configuration, so it can
> > > only be configured by a PF driver or a privileged VF base on our security
> model.
> > > > A typical usage in a NFV environment could be:
> > > >
> > > > 1. A privileged VF (e.g. ice_dcf PMD) use
> > > > rte_eth_dev_udp_tunnel_port_add
> > > to create tunnel port for eCPRI, them this will impact on all VFs in the same
> PF.
> > > > 2. A normal VF driver can create rte_flow rule that match specific
> > > > patch for
> > > queue steering or apply RSS for eCPRI packets, but it DON'T have the
> > > permission to define the tunnel port.
> > >
> > > Whaooh! A normal Intel VF is not allowed to match the tunnel it
> > > wants if not enabled by a priviledged VF?
> >
> > > I would say it is a HW design flaw, but that's not the question.
> >
> > Why you think this is a design flaw? in real case, is it a typical
> > requirement that different VF need different tunnel port for eCPRI (or
> > VxLan) on the same PF?
> 
> They are different VFs, so why should they use the same UDP port?
> Anyway it doesn't need to be typical to be allowed.

Yes, of cause, your can support different UDP tunnel port for different VF, but there are lots of alternative ways to isolate VFs, its just not a big deal for most real use case.
The typical requirement is some customer want eCPRI with UDP port A, while another one want UDP port B, and our NIC is good enough to support both cases separately.
There are seldom cases that different eCPRI tunnel port need to be deployed on the same NIC or same port.
so from my view, it's a reasonable design compromise that lose minor software flexibility but get a more simplified firmware and save more hardware resource from unnecessary usage.

> 
> > I believe it's not necessary to make it as a per VF resource in most
> > cases, and I will be surprise if a driver that allow any VF to change
> > the share resource without any privilege control.
> 
> The thing is that a flow rule should not be a shared resource.
> In Intel devices, it seems the UDP port of a protocol is supposed to be shared
> with all VFs, but it looks a very specific assumption, or limitation.
> I wonder how we can document this and ask the user to call
> rte_eth_dev_udp_tunnel_port_add(), because of some devices.
> Anyway, this is currently poorly documented.

OK, let me check the document to see if anything we can improve.

> 
> > Btw I guess mlx NIC has more flexible way to handle ecpri tunnel, just
> > curious how it works, what's the expected result of below rules?
> >
> > 1. create flow eth / ipv4 / udp dst is 1234 / ecpri msgtype is 0 / ...
> > to queue 0 2. create flow eth / ipv4 / udp dst is 5678 / ecrpi msgtype is 1 / ...
> to queue 1.
> 
> It should move the eCPRI packets to the right queue, taking into consideration
> the UDP port and the message type.
> Of course there may be some bugs :)

I guess it is not just some bugs, I saw below note in Mellanox latest MLX5 driver.
"eCPRI over UDP layer is not yet supported right now",  
but this is not the question, I believe your answers are all fit for the VxLan case :)

For VxLAN offload I note below statement from your user manual

*If you configure multiple UDP ports for offload and exceed the total number of ports supported by hardware, then those additional ports will
still function properly, but will not benefit from any of the stateless offloads. 

Looks like you have a port limitation, additional port that above this number will not work with offload like RSS/steering ...,that's fine.
So my understanding the port resource is not just a regular rule in your general flow table.
The questions is how many is the limitation ?  does each VF has its own resource pool? 
If they are shared, how do you manage these ports? 
What if one malicious VF used up all the tunnel ports, does another VF still get chance to create its own?

> 
> > So both 1234 and 5678 will be regarded as an ECPRI packet?
> 
> Yes, both should be considered eCPRI.
> 
> > Or only the first one will work?
> 
> I am not aware of such limitation.
> 
> > does dst udp port is always needed if an ecpri pattern is involved?
> 
> No, the UDP part is optional.
>
Ferruh Yigit Jan. 14, 2021, 2:34 p.m. UTC | #33
On 12/24/2020 6:59 AM, Jeff Guo wrote:
> Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev tunnel type.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

But since you will be sending a new version of the set, can you please include a 
short release notes update?
Guo, Jia Jan. 15, 2021, 2:51 a.m. UTC | #34
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, January 14, 2021 10:35 PM
> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for
> ecpri
> 
> On 12/24/2020 6:59 AM, Jeff Guo wrote:
> > Add type of RTE_TUNNEL_TYPE_ECPRI into the enum of ethdev tunnel
> type.
> >
> > Signed-off-by: Jeff Guo <jia.guo@intel.com>
> > Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> But since you will be sending a new version of the set, can you please include
> a short release notes update?

Sure, please check v3 again, thanks.
Thomas Monjalon Jan. 15, 2021, 3:15 p.m. UTC | #35
12/01/2021 03:14, Zhang, Qi Z:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 11/01/2021 15:02, Zhang, Qi Z:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 11/01/2021 12:26, Zhang, Qi Z:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > 10/01/2021 11:46, Ori Kam:
> > > > > > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > > > > From: 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>
> > > > > > > > > > >>> 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.
> > > > > > > >
> > > > > > > > Its not about the pattern match, it's about the action, what
> > > > > > > > we need is a rte_flow action to "define a packet's tunnel
> > > > > > > > type", but we don't
> > > > have.
> > > > > >
> > > > > > A packet type alone is meaningless.
> > > > > > It is always associated to an action, this is what rte_flow does.
> > > > >
> > > > > As I mentioned in previous, this is a device (port) level
> > > > > configuration, so it can
> > > > only be configured by a PF driver or a privileged VF base on our security
> > model.
> > > > > A typical usage in a NFV environment could be:
> > > > >
> > > > > 1. A privileged VF (e.g. ice_dcf PMD) use
> > > > > rte_eth_dev_udp_tunnel_port_add
> > > > to create tunnel port for eCPRI, them this will impact on all VFs in the same
> > PF.
> > > > > 2. A normal VF driver can create rte_flow rule that match specific
> > > > > patch for
> > > > queue steering or apply RSS for eCPRI packets, but it DON'T have the
> > > > permission to define the tunnel port.
> > > >
> > > > Whaooh! A normal Intel VF is not allowed to match the tunnel it
> > > > wants if not enabled by a priviledged VF?
> > >
> > > > I would say it is a HW design flaw, but that's not the question.
> > >
> > > Why you think this is a design flaw? in real case, is it a typical
> > > requirement that different VF need different tunnel port for eCPRI (or
> > > VxLan) on the same PF?
> > 
> > They are different VFs, so why should they use the same UDP port?
> > Anyway it doesn't need to be typical to be allowed.
> 
> Yes, of cause, your can support different UDP tunnel port for different VF, but there are lots of alternative ways to isolate VFs, its just not a big deal for most real use case.
> The typical requirement is some customer want eCPRI with UDP port A, while another one want UDP port B, and our NIC is good enough to support both cases separately.
> There are seldom cases that different eCPRI tunnel port need to be deployed on the same NIC or same port.
> so from my view, it's a reasonable design compromise that lose minor software flexibility but get a more simplified firmware and save more hardware resource from unnecessary usage.
> 
> > 
> > > I believe it's not necessary to make it as a per VF resource in most
> > > cases, and I will be surprise if a driver that allow any VF to change
> > > the share resource without any privilege control.
> > 
> > The thing is that a flow rule should not be a shared resource.
> > In Intel devices, it seems the UDP port of a protocol is supposed to be shared
> > with all VFs, but it looks a very specific assumption, or limitation.
> > I wonder how we can document this and ask the user to call
> > rte_eth_dev_udp_tunnel_port_add(), because of some devices.
> > Anyway, this is currently poorly documented.
> 
> OK, let me check the document to see if anything we can improve.

Thank you for trying to improve the doc.


> > > Btw I guess mlx NIC has more flexible way to handle ecpri tunnel, just
> > > curious how it works, what's the expected result of below rules?
> > >
> > > 1. create flow eth / ipv4 / udp dst is 1234 / ecpri msgtype is 0 / ...
> > > to queue 0 2. create flow eth / ipv4 / udp dst is 5678 / ecrpi msgtype is 1 / ...
> > to queue 1.
> > 
> > It should move the eCPRI packets to the right queue, taking into consideration
> > the UDP port and the message type.
> > Of course there may be some bugs :)
> 
> I guess it is not just some bugs, I saw below note in Mellanox latest MLX5 driver.
> "eCPRI over UDP layer is not yet supported right now",  
> but this is not the question, I believe your answers are all fit for the VxLan case :)
> 
> For VxLAN offload I note below statement from your user manual
> 
> *If you configure multiple UDP ports for offload and exceed the total number of ports supported by hardware, then those additional ports will
> still function properly, but will not benefit from any of the stateless offloads. 
> 
> Looks like you have a port limitation, additional port that above this number will not work with offload like RSS/steering ...,that's fine.
> So my understanding the port resource is not just a regular rule in your general flow table.
> The questions is how many is the limitation ?  does each VF has its own resource pool? 
> If they are shared, how do you manage these ports? 
> What if one malicious VF used up all the tunnel ports, does another VF still get chance to create its own?

Sorry I don't know exactly what are the limitations.
From DPDK point of view, when a flow rule cannot be created,
it returns an error and the app must handle.
Yes the app must handle limitations because there is no magic
with hardware offloads: hardware are all more or less limited,
that's a sad truth of our finite world ;)
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index f5f8919186..2cbce958cf 100644
--- 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,
 };