[dpdk-dev,v5,1/4] ethdev: add support for raw flow type for flow director
Checks
Commit Message
Add new structure rte_eth_raw_flow to the union rte_eth_fdir_flow
to support filter for raw flow type.
Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
---
lib/librte_ether/rte_eth_ctrl.h | 10 ++++++++++
1 file changed, 10 insertions(+)
Comments
Hi,
10/10/2017 22:28, Kirill Rybalchenko:
> Add new structure rte_eth_raw_flow to the union rte_eth_fdir_flow
> to support filter for raw flow type.
>
> Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
This description does not explain why you add this new flow director type.
It seems you are allowing a new feature to filter custom protocols.
As I replied on v2, you are implementing your new feature with
a deprecated API (there is a deprecation notice without any deadline).
It is dangerous because this new use case will be settled on top
of a fragile foundation. And because of these new users, it will be
harder to drop this API as announced.
It is also dangerous because you are not trying to implement your
feature with the new rte_flow API. So we cannot be sure that it
will fit for every use cases.
If rte_flow is not good enough, we must improve it.
This is my suggestion:
1/ Implement this interesting feature with rte_flow.
2/ Switch every other use cases to rte_flow.
3/ Let's agree on a date to drop the legacy flow director API.
So this is a NACK.
Please let's move forward.
Hi Thomas,
The reason this feature is needed is to be able to program custom flow types using a template packet rather than building up a C struct to define the protocol. This means that users don't have to work on the DPDK internals to support new flow types that they may be using, but can instead add them dynamically as part of their application. There are also several customers who are looking for this feature as part of the 17.11 LTS release.
This patchset has been out since August and these comments are very late, with the first objections last week, which we tried to answer. This short notice doesn't allow us a reasonable amount of time to take them into account.
However, to address your primary concern, we can implement this using a i40e private API, so that we are not tying users to FDIR APIs and thus not blocking the removal of the APIs in time.
Ideally we would like to use rte_flow but it is based around the idea of describing packet headers which is significantly different from the proposed method using template packets. Longer term it may be possible to support this in rte_flow, we could propose this for discussion in the next release, and if there is community interest/agreement we can add it.
We will rework this, in the short term, as a private API, as suggested above, and then propose an rte_flow API in the longer term. Let us know if you have any concerns about this.
Regards,
Kirill.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday 11 October 2017 23:27
> To: Rybalchenko, Kirill <kirill.rybalchenko@intel.com>
> Cc: dev@dpdk.org; Chilikin, Andrey <andrey.chilikin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v5 1/4] ethdev: add support for raw flow
> type for flow director
>
> Hi,
>
> 10/10/2017 22:28, Kirill Rybalchenko:
> > Add new structure rte_eth_raw_flow to the union rte_eth_fdir_flow to
> > support filter for raw flow type.
> >
> > Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
>
> This description does not explain why you add this new flow director type.
> It seems you are allowing a new feature to filter custom protocols.
>
> As I replied on v2, you are implementing your new feature with a deprecated
> API (there is a deprecation notice without any deadline).
> It is dangerous because this new use case will be settled on top of a fragile
> foundation. And because of these new users, it will be harder to drop this
> API as announced.
> It is also dangerous because you are not trying to implement your feature
> with the new rte_flow API. So we cannot be sure that it will fit for every use
> cases.
> If rte_flow is not good enough, we must improve it.
>
> This is my suggestion:
> 1/ Implement this interesting feature with rte_flow.
> 2/ Switch every other use cases to rte_flow.
> 3/ Let's agree on a date to drop the legacy flow director API.
>
> So this is a NACK.
> Please let's move forward.
OK, a private API is a good short term approach.
12/10/2017 13:41, Rybalchenko, Kirill:
> Hi Thomas,
>
> The reason this feature is needed is to be able to program custom flow types using a template packet rather than building up a C struct to define the protocol. This means that users don't have to work on the DPDK internals to support new flow types that they may be using, but can instead add them dynamically as part of their application. There are also several customers who are looking for this feature as part of the 17.11 LTS release.
>
> This patchset has been out since August and these comments are very late, with the first objections last week, which we tried to answer. This short notice doesn't allow us a reasonable amount of time to take them into account.
>
> However, to address your primary concern, we can implement this using a i40e private API, so that we are not tying users to FDIR APIs and thus not blocking the removal of the APIs in time.
>
> Ideally we would like to use rte_flow but it is based around the idea of describing packet headers which is significantly different from the proposed method using template packets. Longer term it may be possible to support this in rte_flow, we could propose this for discussion in the next release, and if there is community interest/agreement we can add it.
>
> We will rework this, in the short term, as a private API, as suggested above, and then propose an rte_flow API in the longer term. Let us know if you have any concerns about this.
>
> Regards,
> Kirill.
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Wednesday 11 October 2017 23:27
> > To: Rybalchenko, Kirill <kirill.rybalchenko@intel.com>
> > Cc: dev@dpdk.org; Chilikin, Andrey <andrey.chilikin@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v5 1/4] ethdev: add support for raw flow
> > type for flow director
> >
> > Hi,
> >
> > 10/10/2017 22:28, Kirill Rybalchenko:
> > > Add new structure rte_eth_raw_flow to the union rte_eth_fdir_flow to
> > > support filter for raw flow type.
> > >
> > > Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
> >
> > This description does not explain why you add this new flow director type.
> > It seems you are allowing a new feature to filter custom protocols.
> >
> > As I replied on v2, you are implementing your new feature with a deprecated
> > API (there is a deprecation notice without any deadline).
> > It is dangerous because this new use case will be settled on top of a fragile
> > foundation. And because of these new users, it will be harder to drop this
> > API as announced.
> > It is also dangerous because you are not trying to implement your feature
> > with the new rte_flow API. So we cannot be sure that it will fit for every use
> > cases.
> > If rte_flow is not good enough, we must improve it.
> >
> > This is my suggestion:
> > 1/ Implement this interesting feature with rte_flow.
> > 2/ Switch every other use cases to rte_flow.
> > 3/ Let's agree on a date to drop the legacy flow director API.
> >
> > So this is a NACK.
> > Please let's move forward.
Hi Kirill,
On Thu, Oct 12, 2017 at 11:41:50AM +0000, Rybalchenko, Kirill wrote:
> Hi Thomas,
>
> The reason this feature is needed is to be able to program custom flow types using a template packet rather than building up a C struct to define the protocol. This means that users don't have to work on the DPDK internals to support new flow types that they may be using, but can instead add them dynamically as part of their application. There are also several customers who are looking for this feature as part of the 17.11 LTS release.
>
> This patchset has been out since August and these comments are very late, with the first objections last week, which we tried to answer. This short notice doesn't allow us a reasonable amount of time to take them into account.
>
> However, to address your primary concern, we can implement this using a i40e private API, so that we are not tying users to FDIR APIs and thus not blocking the removal of the APIs in time.
>
> Ideally we would like to use rte_flow but it is based around the idea of describing packet headers which is significantly different from the proposed method using template packets. Longer term it may be possible to support this in rte_flow, we could propose this for discussion in the next release, and if there is community interest/agreement we can add it.
>
> We will rework this, in the short term, as a private API, as suggested above, and then propose an rte_flow API in the longer term. Let us know if you have any concerns about this.
I am not trying to push for its integration through rte_flow at this stage
and I don't mind the chosen PMD-specific approach, I'm just curious about
the reasons that made it hard to implement as a RTE_FLOW_ITEM_RAW thing?
(e.g. a rule with a pattern that only contains one or several such items)
Please have a look at the rte_flow_item_raw structure in rte_flow.h, tell me
what's missing in there and I'll take it into account during the next
overhaul. Thanks in advance for your feedback.
@@ -525,6 +525,15 @@ struct rte_eth_tunnel_flow {
};
/**
+ * A structure used to define the input for raw flow
+ */
+struct rte_eth_raw_flow {
+ uint16_t flow; /**< flow type. */
+ void *packet; /**< pre-constructed packet buffer. */
+ uint16_t length; /**< buffer length. */
+};
+
+/**
* An union contains the inputs for all types of flow
* Items in flows need to be in big endian
*/
@@ -540,6 +549,7 @@ union rte_eth_fdir_flow {
struct rte_eth_ipv6_flow ipv6_flow;
struct rte_eth_mac_vlan_flow mac_vlan_flow;
struct rte_eth_tunnel_flow tunnel_flow;
+ struct rte_eth_raw_flow raw_flow;
};
/**