Re: [dpdk-dev] [PATCH v2 1/7] ethdev: introduce sample action for rte flow

[Matan Azrad <matan@mellanox.com>]

From: Jerin Jacob:
> On Sun, Jul 5, 2020 at 12:56 AM Matan Azrad <matan@mellanox.com> wrote:
> >
> > Hi all
> >
> > From: Jerin Jacob:
> > > On Fri, Jul 3, 2020 at 8:57 PM Thomas Monjalon <thomas@monjalon.net>
> > > wrote:
> > > >
> > > > 03/07/2020 17:08, Jerin Jacob:
> > > > > On Fri, Jul 3, 2020 at 8:25 PM Matan Azrad <matan@mellanox.com>
> > > wrote:
> > > > > > From: Jerin Jacob:
> > > > > > > When adding overlapping API(rte_eth_mirror_rule_set()) in
> > > > > > > the same library(ethdev).
> > > > > > > Please depreciate the old API.
> > > > > > > We should not have two separate paths for the same function
> > > > > > > in the same ethdev library. It is pain for app and driver developers.
> > > > > >
> > > > > > What are about all the other rte_flow parallel configuration
> > > > > > APIs in
> > > ethdev:
> > > > > >  promiscuous_enable;
> > > > > > promiscuous_disable;
> > > > > > allmulticast_enable;
> > > > > > allmulticast_disable;
> > > > > > mac_addr_remove;
> > > > > > mac_addr_add;
> > > > > > mac_addr_set;
> > > > > > set_mc_addr_list;
> > > > > > vlan_filter_set;
> > > > > > vlan_tpid_set;
> > > > > > vlan_strip_queue_set;
> > > > > > vlan_offload_set;
> > > > > > vlan_pvid_set;
> > > > > > udp_tunnel_port_add;
> > > > > > udp_tunnel_port_del;
> > > > > > ...
> > > > > >
> > > > > > These APIs can be replaced easily by rte_flow API.
> > > > > > Do you think we need to deprecate all?
> > > > >
> > > > > I think, basic stuff like below can have separate API.
> > > > > 1)  promiscuous_enable;
> > > > > 2) promiscuous_disable;
> > > > > 3) allmulticast_enable;
> > > > > 4) allmulticast_disable;
> > > > > 5) mac_addr_remove;
> > > > > 6) mac_addr_add;
> > > > > 7) mac_addr_set;
> > > > > 8) set_mc_addr_list;
> > > >
> > > > "Basic" is not a precise definition :)
> > >
> > > Yep.
> > >
> > > > I would say port-level configuration should remain out of rte_flow
> > > > API.
> >
> > Thomas, Can you explain what is port-level?
> > Everything in rte_flow is per port...
> >
> > Also, can you give reasons for your claim?
> >
> > > +1.
> > > In addition that, I would say anything needs to configured at "per-flow"
> > > granularity use rte_flow.
> >
> > Jerin, What do you mean "per-flow" ?
> 
> In Terms of  NIC HW features, Typical HW will have
> a) Basic "port" level configuration like
> - enable/disable promiscuous

What is "port level", everything in rte_flow is also per port...

> b) Advance HW's will have CAM based flow filtering. IMO, CAM related stuff
> should go to rte_flow.

It is HW internal, I don't think all HWs use the same logic here.
Since rte_flow is generic for all filtering methods, It is good candidate API for all HWs. 

> This is to enable,  The very basic PMD(without advanced features) should
> work with port level basic APIs(i.e without rte_flow)

What is "basic"? Do you mean simple match and simple action?
As I said, Also rte_flow is port level API - so "port level" is not good term here.

As you said " When adding overlapping API(rte_eth_mirror_rule_set()) in the same library(ethdev). Please depreciate the old API."

Different APIs to do the same thing is not good, especially in packet filtering:
What should we do if we have conflicts?
For example: legacy filtering APIs say to receive packet A and rte_flow says to drop it.

Don't you think it complicates more the user API understanding, also the PMD implementations?


> I have seen promiscuous, mac address handling is part of basic NIC HW(i.e
> NICs without advanced CAM filters).
> That's my reasoning for the split.

As I said, the nic HW specific implementation should not affect the API.
I don't think we need to split API and to complicate the user because of it.

IMO, It is better to have one generic API for packet filtering:
It is clearer, simpler, generic and classic.
The user and PMD need to understand only one filtering API and that’s it (no need to combine between multiple filtering APIs). 

I know this is big change but we can do it in modular way.
It reminds me the big change that was done in Rx\Tx offload configurations:
So, when offload became more popular we modularly forced users to replace the offload API.
Also here, flow filtering becomes popular so maybe this is the time(20.08-20.11) to force changes in the old APIs.   

> > Everything in traffic filtering\actions is per flow, for example:
> > Promiscuous: flow create 0 ingress pattern eth / end actions queue
> > index 0 / end
> 
> IMO, it is not an accurate representation of promiscuous enable. It needs to
> send the traffic to all queues and patterns is not just eth.

Yes, if legacy RSS is configured we need to replace the above queue action by rss action as I wrote before.(did you read it just below?)

So, we can add legacy RSS APIs to the list above...

> > Multicast\mac related: flow create 0 ingress pattern eth dst is X /end
> > actions queue 0/ end (in case of legacy RSS queue action will be replaced by
> rss).
> > ....
> >
> > Everything here are flows.
> >
> > > >
> > > > > But The VLAN and UDP related should be rte_flow candidates.(IMO)
> > > >
> > > > Yes definitely, tunneling is better managed with rte_flow.
> >
> > Can you give reasons for your claim?
> > Why should Vlan\Tunnel be in rte_flow and promiscuous\Multicast\mac
> not?
> >
> > > > This is an important discussion, I Cc other ethdev maintainers.
> >
> > Agree, this is a good trigger to open this important discussion.
> >
> > > > Note: this is an ethdev patch, all ethdev maintainers should be Cc'ed.
> > > > Aren't you using --cc-cmd devtools/get-maintainer.sh ?
> >