[dpdk-dev,01/10] ethdev: add a generic flow and new behavior switch to fdir

  Add a new raw packet flow that allows specifying generic flow input.

Add the ability to provide masks for fields in flow to allow range of
values.

Add a new behavior switch.

Add the ability to provide behavior arguments to allow rewriting matched
fields with new values. Ex: allows to provide new ip and port addresses
to rewrite the fields of packets matching a filter rule before NAT'ing.

Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
---
 doc/guides/rel_notes/release_2_3.rst |  3 +++
 lib/librte_ether/rte_eth_ctrl.h      | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)
  

On Wed, Feb 03, 2016 at 02:02:22PM +0530, Rahul Lakkireddy wrote:
> Add a new raw packet flow that allows specifying generic flow input.
> 
> Add the ability to provide masks for fields in flow to allow range of
> values.
> 
> Add a new behavior switch.
> 
> Add the ability to provide behavior arguments to allow rewriting matched
> fields with new values. Ex: allows to provide new ip and port addresses
> to rewrite the fields of packets matching a filter rule before NAT'ing.
> 
> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
> ---
>  doc/guides/rel_notes/release_2_3.rst |  3 +++
>  lib/librte_ether/rte_eth_ctrl.h      | 15 ++++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
Thomas, any comments as ethdev maintainer?

Jingjing, you have been doing some work on flow director for other NICs. Can
you perhaps review this patch as well.

Regards,
/Bruce
  

2016-02-24 14:43, Bruce Richardson:
> On Wed, Feb 03, 2016 at 02:02:22PM +0530, Rahul Lakkireddy wrote:
> > Add a new raw packet flow that allows specifying generic flow input.
> > 
> > Add the ability to provide masks for fields in flow to allow range of
> > values.
> > 
> > Add a new behavior switch.
> > 
> > Add the ability to provide behavior arguments to allow rewriting matched
> > fields with new values. Ex: allows to provide new ip and port addresses
> > to rewrite the fields of packets matching a filter rule before NAT'ing.
> > 
> Thomas, any comments as ethdev maintainer?

Yes, some comments.
First, there are several different changes in the same patch. It must be split.
Then I don't understand at all the raw flow filter. What is a raw flow?
How behavior_arg must be used?
  

Hi Thomas,

On Wednesday, February 02/24/16, 2016 at 07:02:42 -0800, Thomas Monjalon wrote:
> 2016-02-24 14:43, Bruce Richardson:
> > On Wed, Feb 03, 2016 at 02:02:22PM +0530, Rahul Lakkireddy wrote:
> > > Add a new raw packet flow that allows specifying generic flow input.
> > > 
> > > Add the ability to provide masks for fields in flow to allow range of
> > > values.
> > > 
> > > Add a new behavior switch.
> > > 
> > > Add the ability to provide behavior arguments to allow rewriting matched
> > > fields with new values. Ex: allows to provide new ip and port addresses
> > > to rewrite the fields of packets matching a filter rule before NAT'ing.
> > > 
> > Thomas, any comments as ethdev maintainer?
> 
> Yes, some comments.
> First, there are several different changes in the same patch. It must be split.

Should each structure change be split into a separate patch?

> Then I don't understand at all the raw flow filter. What is a raw flow?
> How behavior_arg must be used?
> 

This was discussed with Jingjing at

http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/31471

A raw flow provides a generic way for vendors to add their vendor
specific input flow.  In our case, it is possible to match several flows
in a single rule.  For example, it's possible to set an ethernet, vlan,
ip and tcp/udp flows all in a single rule.  We can specify all of these
flows in a single raw input flow, which can then be passed to cxgbe flow
director to set the corresponding filter.

On similar lines, behavior_arg provides a generic way to pass extra
action arguments for matched flows.  For example, in our case, to
perform NAT, the new src/dst ip and src/dst port addresses to be
re-written for a matched rule can be passed in behavior_arg.

Thanks,
Rahul
  

Caution: I truly respect the work done by Chelsio on DPDK.
And I'm sure you can help to build a good filtering API, which
was mainly designed with Intel needs in mind because it was
difficult to have opinions of other vendors some time ago.
That's why it's a chance to have new needs and it would be a shame
to let it go through a vendor specific backdoor.

2016-02-25 00:10, Rahul Lakkireddy:
> Hi Thomas,
> 
> On Wednesday, February 02/24/16, 2016 at 07:02:42 -0800, Thomas Monjalon wrote:
> > 2016-02-24 14:43, Bruce Richardson:
> > > On Wed, Feb 03, 2016 at 02:02:22PM +0530, Rahul Lakkireddy wrote:
> > > > Add a new raw packet flow that allows specifying generic flow input.
> > > > 
> > > > Add the ability to provide masks for fields in flow to allow range of
> > > > values.
> > > > 
> > > > Add a new behavior switch.
> > > > 
> > > > Add the ability to provide behavior arguments to allow rewriting matched
> > > > fields with new values. Ex: allows to provide new ip and port addresses
> > > > to rewrite the fields of packets matching a filter rule before NAT'ing.
> > > > 
> > > Thomas, any comments as ethdev maintainer?
> > 
> > Yes, some comments.
> > First, there are several different changes in the same patch. It must be split.
> 
> Should each structure change be split into a separate patch?

A patch = a feature.
The switch action and the flow rule are different things.

> > Then I don't understand at all the raw flow filter. What is a raw flow?
> > How behavior_arg must be used?
> 
> This was discussed with Jingjing at
> 
> http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/31471

Thanks, I missed it.

> A raw flow provides a generic way for vendors to add their vendor
> specific input flow.

Please, "generic" and "vendor specific" in the same sentence.
It's obviously wrong.

> In our case, it is possible to match several flows
> in a single rule.  For example, it's possible to set an ethernet, vlan,
> ip and tcp/udp flows all in a single rule.  We can specify all of these
> flows in a single raw input flow, which can then be passed to cxgbe flow
> director to set the corresponding filter.

I feel we need to define what is an API.
If the application wants to call something specific to the NIC, why using
the ethdev API? You just have to include cxgbe.h.

> On similar lines, behavior_arg provides a generic way to pass extra
> action arguments for matched flows.  For example, in our case, to
> perform NAT, the new src/dst ip and src/dst port addresses to be
> re-written for a matched rule can be passed in behavior_arg.

Yes a kind of void* to give what you want to the driver without the
convenience of a documented function.

I know the support of filters among NICs is really heterogeneous.
And the DPDK API are not yet generic enough. But please do not give up!
If the filtering API can be improved to support your cases, please do it.
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Rahul Lakkireddy
> Sent: Wednesday, February 03, 2016 4:32 PM
> To: dev@dpdk.org
> Cc: Kumar Sanghvi; Nirranjan Kirubaharan
> Subject: [dpdk-dev] [PATCH 01/10] ethdev: add a generic flow and new
> behavior switch to fdir
> 
> Add a new raw packet flow that allows specifying generic flow input.
> 
> Add the ability to provide masks for fields in flow to allow range of values.
> 
> Add a new behavior switch.
> 
> Add the ability to provide behavior arguments to allow rewriting matched
> fields with new values. Ex: allows to provide new ip and port addresses to
> rewrite the fields of packets matching a filter rule before NAT'ing.
> 
> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
> ---
>  doc/guides/rel_notes/release_2_3.rst |  3 +++
>  lib/librte_ether/rte_eth_ctrl.h      | 15 ++++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/rel_notes/release_2_3.rst
> b/doc/guides/rel_notes/release_2_3.rst
> index 99de186..19ce954 100644
> --- a/doc/guides/rel_notes/release_2_3.rst
> +++ b/doc/guides/rel_notes/release_2_3.rst
> @@ -39,6 +39,9 @@ API Changes
>  ABI Changes
>  -----------
> 
> +* New flow type ``RTE_ETH_FLOW_RAW_PKT`` had been introduced and
> hence
> +  ``RTE_ETH_FLOW_MAX`` had been increased to 19.
> +
> 
Great to see a raw_pkt_flow type.
And there is already a flow type "RTE_ETH_FLOW_RAW", it's not necessary to add a new one.

>  Shared Library Versions
>  -----------------------
> diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
> index ce224ad..1bc0d03 100644
> --- a/lib/librte_ether/rte_eth_ctrl.h
> +++ b/lib/librte_ether/rte_eth_ctrl.h
> @@ -74,7 +74,8 @@ extern "C" {
>  #define RTE_ETH_FLOW_IPV6_EX            15
>  #define RTE_ETH_FLOW_IPV6_TCP_EX        16
>  #define RTE_ETH_FLOW_IPV6_UDP_EX        17
> -#define RTE_ETH_FLOW_MAX                18
> +#define RTE_ETH_FLOW_RAW_PKT            18
> +#define RTE_ETH_FLOW_MAX                19
> 
>  /**
>   * Feature filter types
> @@ -499,6 +500,9 @@ struct rte_eth_tunnel_flow {
>  	struct ether_addr mac_addr;                /**< Mac address to match. */
>  };
> 
> +/**< Max length of raw packet in bytes. */ #define
> +RTE_ETH_RAW_PKT_FLOW_MAX_LEN 256
> +
>  /**
>   * An union contains the inputs for all types of flow
>   */
> @@ -514,6 +518,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;
> +	uint8_t raw_pkt_flow[RTE_ETH_RAW_PKT_FLOW_MAX_LEN];
>  };
> 
>  /**
> @@ -534,6 +539,8 @@ struct rte_eth_fdir_input {
>  	uint16_t flow_type;
>  	union rte_eth_fdir_flow flow;
>  	/**< Flow fields to match, dependent on flow_type */
> +	union rte_eth_fdir_flow flow_mask;
> +	/**< Mask for the fields matched, dependent on flow */
>  	struct rte_eth_fdir_flow_ext flow_ext;
>  	/**< Additional fields to match */
>  };
> @@ -545,6 +552,7 @@ enum rte_eth_fdir_behavior {
>  	RTE_ETH_FDIR_ACCEPT = 0,
>  	RTE_ETH_FDIR_REJECT,
>  	RTE_ETH_FDIR_PASSTHRU,
> +	RTE_ETH_FDIR_SWITCH,
>  };
> 
>  /**
> @@ -558,6 +566,9 @@ enum rte_eth_fdir_status {
>  	RTE_ETH_FDIR_REPORT_FLEX_8,        /**< Report 8 flex bytes. */
>  };
> 
> +/**< Max # of behavior arguments */
> +#define RTE_ETH_BEHAVIOR_ARG_MAX_LEN 256
> +
>  /**
>   * A structure used to define an action when match FDIR packet filter.
>   */
> @@ -569,6 +580,8 @@ struct rte_eth_fdir_action {
>  	/**< If report_status is RTE_ETH_FDIR_REPORT_ID_FLEX_4 or
>  	     RTE_ETH_FDIR_REPORT_FLEX_8, flex_off specifies where the
> reported
>  	     flex bytes start from in flexible payload. */
> +	uint8_t behavior_arg[RTE_ETH_BEHAVIOR_ARG_MAX_LEN];
> +	/**< Extra arguments for behavior taken */
>  };
> 
>  /**
> --
> 2.5.3
  

Hi Jingjing,

On Wednesday, February 02/24/16, 2016 at 19:26:54 -0800, Wu, Jingjing wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Rahul Lakkireddy
> > Sent: Wednesday, February 03, 2016 4:32 PM
> > To: dev@dpdk.org
> > Cc: Kumar Sanghvi; Nirranjan Kirubaharan
> > Subject: [dpdk-dev] [PATCH 01/10] ethdev: add a generic flow and new
> > behavior switch to fdir
> > 
> > Add a new raw packet flow that allows specifying generic flow input.
> > 
> > Add the ability to provide masks for fields in flow to allow range of values.
> > 
> > Add a new behavior switch.
> > 
> > Add the ability to provide behavior arguments to allow rewriting matched
> > fields with new values. Ex: allows to provide new ip and port addresses to
> > rewrite the fields of packets matching a filter rule before NAT'ing.
> > 
> > Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> > Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
> > ---
> >  doc/guides/rel_notes/release_2_3.rst |  3 +++
> >  lib/librte_ether/rte_eth_ctrl.h      | 15 ++++++++++++++-
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/doc/guides/rel_notes/release_2_3.rst
> > b/doc/guides/rel_notes/release_2_3.rst
> > index 99de186..19ce954 100644
> > --- a/doc/guides/rel_notes/release_2_3.rst
> > +++ b/doc/guides/rel_notes/release_2_3.rst
> > @@ -39,6 +39,9 @@ API Changes
> >  ABI Changes
> >  -----------
> > 
> > +* New flow type ``RTE_ETH_FLOW_RAW_PKT`` had been introduced and
> > hence
> > +  ``RTE_ETH_FLOW_MAX`` had been increased to 19.
> > +
> > 
> Great to see a raw_pkt_flow type.
> And there is already a flow type "RTE_ETH_FLOW_RAW", it's not necessary to add a new one.

I added a new type based on your feedback only
http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/31386

So, do you want me to revert this change and use RTE_ETH_FLOW_RAW
instead ?


Thanks,
Rahul.
  

Hi Thomas,

On Wednesday, February 02/24/16, 2016 at 14:17:58 -0800, Thomas Monjalon wrote:
> Caution: I truly respect the work done by Chelsio on DPDK.

Thank you. And Chelsio will continue to support DPDK community and will
continue to contribute.

> And I'm sure you can help to build a good filtering API, which
> was mainly designed with Intel needs in mind because it was
> difficult to have opinions of other vendors some time ago.
> That's why it's a chance to have new needs and it would be a shame
> to let it go through a vendor specific backdoor.

I agree that new needs should be raised.

RFC v1 was submitted at:
http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/29986

RFC v2 was submitted at:
http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/30732

I tried to accomodate as many fields as possible to make this as generic as
possible. Also, I followed the review comments given by Jingjing.
I also waited for more review comments before posting this series to
see if there were any objections with the approach.

I have been trying to make this generic for all vendors and not favour
any one over the other.

> 
> 2016-02-25 00:10, Rahul Lakkireddy:
> > Hi Thomas,
> > 
> > On Wednesday, February 02/24/16, 2016 at 07:02:42 -0800, Thomas Monjalon wrote:
> > > 2016-02-24 14:43, Bruce Richardson:
> > > > On Wed, Feb 03, 2016 at 02:02:22PM +0530, Rahul Lakkireddy wrote:
> > > > > Add a new raw packet flow that allows specifying generic flow input.
> > > > > 
> > > > > Add the ability to provide masks for fields in flow to allow range of
> > > > > values.
> > > > > 
> > > > > Add a new behavior switch.
> > > > > 
> > > > > Add the ability to provide behavior arguments to allow rewriting matched
> > > > > fields with new values. Ex: allows to provide new ip and port addresses
> > > > > to rewrite the fields of packets matching a filter rule before NAT'ing.
> > > > > 
> > > > Thomas, any comments as ethdev maintainer?
> > > 
> > > Yes, some comments.
> > > First, there are several different changes in the same patch. It must be split.
> > 
> > Should each structure change be split into a separate patch?
> 
> A patch = a feature.
> The switch action and the flow rule are different things.

Ok. I will split this into separate patches.

> 
> > > Then I don't understand at all the raw flow filter. What is a raw flow?
> > > How behavior_arg must be used?
> > 
> > This was discussed with Jingjing at
> > 
> > http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/31471
> 
> Thanks, I missed it.
> 
> > A raw flow provides a generic way for vendors to add their vendor
> > specific input flow.
> 
> Please, "generic" and "vendor specific" in the same sentence.
> It's obviously wrong.

I think this sentence is being mis-interpreted.
What I intended to say is: the fields are generic so that any vendor can
hook-in. The fields themselves are not vendor specific.


> 
> > In our case, it is possible to match several flows
> > in a single rule.  For example, it's possible to set an ethernet, vlan,
> > ip and tcp/udp flows all in a single rule.  We can specify all of these
> > flows in a single raw input flow, which can then be passed to cxgbe flow
> > director to set the corresponding filter.
> 
> I feel we need to define what is an API.
> If the application wants to call something specific to the NIC, why using
> the ethdev API? You just have to include cxgbe.h.

Well, in that sense, flow-director is also very intel specific, no ?
What we are trying to do is make flow-director generic and, we have been
following the review comments on this. If there are better ideas on how
to achieve this, we are open to suggestions/comments and are ready to
re-do the series and re-submit also.


> 
> > On similar lines, behavior_arg provides a generic way to pass extra
> > action arguments for matched flows.  For example, in our case, to
> > perform NAT, the new src/dst ip and src/dst port addresses to be
> > re-written for a matched rule can be passed in behavior_arg.
> 
> Yes a kind of void* to give what you want to the driver without the
> convenience of a documented function.

void* approach was taken based on review comments from Jingjing.
And we didn't receive any further comments/objections on that thread.

> 
> I know the support of filters among NICs is really heterogeneous.
> And the DPDK API are not yet generic enough. But please do not give up!
> If the filtering API can be improved to support your cases, please do it.

I am not giving up. If there are better suggestions then, I am willing
to re-do and re-submit the series.
If the approach taken in RFC v1 series looks more promising then, I can
re-surrect that also. However, I will need some direction over here so
that it becomes generic and doesn't remain intel specific as it is now.

Thanks,
Rahul.
  

2016-02-25 15:03, Rahul Lakkireddy:
> On Wednesday, February 02/24/16, 2016 at 14:17:58 -0800, Thomas Monjalon wrote:
> > > A raw flow provides a generic way for vendors to add their vendor
> > > specific input flow.
> > 
> > Please, "generic" and "vendor specific" in the same sentence.
> > It's obviously wrong.
> 
> I think this sentence is being mis-interpreted.
> What I intended to say is: the fields are generic so that any vendor can
> hook-in. The fields themselves are not vendor specific.

We are trying to push some features into fields of an API instead of
thinking how to make it simple.

> > > In our case, it is possible to match several flows
> > > in a single rule.  For example, it's possible to set an ethernet, vlan,
> > > ip and tcp/udp flows all in a single rule.  We can specify all of these
> > > flows in a single raw input flow, which can then be passed to cxgbe flow
> > > director to set the corresponding filter.
> > 
> > I feel we need to define what is an API.
> > If the application wants to call something specific to the NIC, why using
> > the ethdev API? You just have to include cxgbe.h.
> 
> Well, in that sense, flow-director is also very intel specific, no ?

Yes. I think the term "flow director" comes from Intel.

> What we are trying to do is make flow-director generic

So let's stop calling it flow director.
We are talking about filtering, right?

> and, we have been
> following the review comments on this. If there are better ideas on how
> to achieve this, we are open to suggestions/comments and are ready to
> re-do the series and re-submit also.

My first question: are you happy with the current API?
Do you understand the difference between RTE_ETH_FILTER_ETHERTYPE and
RTE_ETH_FILTER_FDIR with RTE_ETH_FLOW_L2_PAYLOAD?
Do you understand this structure?
enum rte_eth_fdir_status {
    RTE_ETH_FDIR_NO_REPORT_STATUS = 0, /**< Report nothing. */
    RTE_ETH_FDIR_REPORT_ID,            /**< Only report FD ID. */
    RTE_ETH_FDIR_REPORT_ID_FLEX_4,     /**< Report FD ID and 4 flex bytes. */
    RTE_ETH_FDIR_REPORT_FLEX_8,        /**< Report 8 flex bytes. */
};
These values?
enum rte_fdir_mode {
    RTE_FDIR_MODE_NONE      = 0, /**< Disable FDIR support. */
    RTE_FDIR_MODE_SIGNATURE,     /**< Enable FDIR signature filter mode. */
    RTE_FDIR_MODE_PERFECT,       /**< Enable FDIR perfect filter mode. */
    RTE_FDIR_MODE_PERFECT_MAC_VLAN, /**< Enable FDIR filter mode - MAC VLAN. */
    RTE_FDIR_MODE_PERFECT_TUNNEL,   /**< Enable FDIR filter mode - tunnel. */
};

From my point of view, it is insane.
We have put the hardware complexity in the API.
And now you want to put some vendor specific data in some fields
like some black magic recipes.

Why is it so complex? We are talking about packet filtering, not rocket science!

> > I know the support of filters among NICs is really heterogeneous.
> > And the DPDK API are not yet generic enough. But please do not give up!
> > If the filtering API can be improved to support your cases, please do it.
> 
> I am not giving up. If there are better suggestions then, I am willing
> to re-do and re-submit the series.
> If the approach taken in RFC v1 series looks more promising then, I can
> re-surrect that also. However, I will need some direction over here so
> that it becomes generic and doesn't remain intel specific as it is now.

Yes the approach in the RFC was better in the sense it was describing the
fields. But honestly, I'd prefer thinking of filtering from scratch.

What is a hardware filter? (note there is no such doc yet)
It matches a packet with some criterias and take an action on it.
Simple.
Now details (it can take weeks or months to list every details).

A hardware implements a subset of the infinite capabilities.
So the API must provide a flag to check a rule/action capability without
really configuring it.

A matching rule must match every criterias or only some of them (AND/OR operators).
An action is associated to a matching rule.
There can be several matching rules on the same port (Chelsio case?).
Any packet field can be matched (we currently define some of them).

An action can be of different types:
- drop
- switch
- accept in any queue
- accept in a specific queue

Most of the rules give some values to match the fields.
The hash filtering (RSS) specify only some fields and a key to direct
packets in different queues.

Please, Intel, Chelsio and other vendors, tell what is wrong above
and let's start a sane discussion on hardware filtering.
More background:
The current API was designed by Intel when they were the only NIC vendor.
Now that there are 8 vendors with different capabilities and that FPGA should
bring even more capabilities, we should be able to build something more
generic while being usable.
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, February 26, 2016 2:25 AM
> To: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; dev@dpdk.org; Kumar A S
> <kumaras@chelsio.com>; Nirranjan Kirubaharan <nirranjan@chelsio.com>; Wu, Jingjing
> <jingjing.wu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 01/10] ethdev: add a generic flow and new behavior switch
> to fdir
> 
> 2016-02-25 15:03, Rahul Lakkireddy:
> > On Wednesday, February 02/24/16, 2016 at 14:17:58 -0800, Thomas Monjalon wrote:
> > > > A raw flow provides a generic way for vendors to add their vendor
> > > > specific input flow.
> > >
> > > Please, "generic" and "vendor specific" in the same sentence.
> > > It's obviously wrong.
> >
> > I think this sentence is being mis-interpreted.
> > What I intended to say is: the fields are generic so that any vendor can
> > hook-in. The fields themselves are not vendor specific.
> 
> We are trying to push some features into fields of an API instead of
> thinking how to make it simple.
> 
> > > > In our case, it is possible to match several flows
> > > > in a single rule.  For example, it's possible to set an ethernet, vlan,
> > > > ip and tcp/udp flows all in a single rule.  We can specify all of these
> > > > flows in a single raw input flow, which can then be passed to cxgbe flow
> > > > director to set the corresponding filter.
> > >
> > > I feel we need to define what is an API.
> > > If the application wants to call something specific to the NIC, why using
> > > the ethdev API? You just have to include cxgbe.h.
> >
> > Well, in that sense, flow-director is also very intel specific, no ?
> 
> Yes. I think the term "flow director" comes from Intel.
> 
> > What we are trying to do is make flow-director generic
> 
> So let's stop calling it flow director.
> We are talking about filtering, right?
> 
Hi Thomas

Are you suggesting chelsio to define a new filter type?

> Why is it so complex? We are talking about packet filtering, not rocket science!
>
The complex is due to different NICs different behavior :-)
As I know, it is a common way to use used-define data pass specific infor to driver.

Even flow director is concept from Intel's NIC, but I think it is the generic one comparing
with other kinds of filters. So I think that's why Rahul choose it to add their kind of filters.
As I know enic driver also uses flow director API to support their filters.

No matter chelsio NIC filter uses flow director API or define another new filter type. I vote
the change happened in struct rte_eth_fdir_input, it provide a RAW Flow type,
And there is also a mask field for that, by this way, user can have a flexible way to configure.
And drivers can parse the raw input to define the filter fields.

But for the change happened in struct rte_eth_fdir_action, only SWITCH type is added,
Where to switch? All things is in behavior_arg[RTE_ETH_BEHAVIOR_ARG_MAX_LEN]
which is black to user. Maybe your previous define in RFC makes more sense. It's better to add
user defined field but not for all args.

Any better suggestion?
  

I think what Thomas meant is that we should redesign  Flow Director feature and call it something else , Mellanox is calling  it "Flow Steering"  . I agree that  Filtering may be more generic name.
We have implemented Flow Director API in Mellanox ConnectX-4 PMD (part of the DPDK 16.04 patches) but  we did is in very awkward way that will fit the current API and some Mellanox features are missing with current Flow Director API.
Therefore I disagree with Jingjing's statement that this API is generic. 
Frankly, it is very hard to understand it , as Thomas mentioned  ..., not sure how DPDK users understand what each function/field means .... 

Best Regards,
Olga

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wu, Jingjing
Sent: Friday, February 26, 2016 3:18 AM
To: Thomas Monjalon; Rahul Lakkireddy
Cc: dev@dpdk.org; Kumar A S; Nirranjan Kirubaharan
Subject: Re: [dpdk-dev] [PATCH 01/10] ethdev: add a generic flow and new behavior switch to fdir



> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, February 26, 2016 2:25 AM
> To: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; dev@dpdk.org; 
> Kumar A S <kumaras@chelsio.com>; Nirranjan Kirubaharan 
> <nirranjan@chelsio.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 01/10] ethdev: add a generic flow and 
> new behavior switch to fdir
> 
> 2016-02-25 15:03, Rahul Lakkireddy:
> > On Wednesday, February 02/24/16, 2016 at 14:17:58 -0800, Thomas Monjalon wrote:
> > > > A raw flow provides a generic way for vendors to add their 
> > > > vendor specific input flow.
> > >
> > > Please, "generic" and "vendor specific" in the same sentence.
> > > It's obviously wrong.
> >
> > I think this sentence is being mis-interpreted.
> > What I intended to say is: the fields are generic so that any vendor 
> > can hook-in. The fields themselves are not vendor specific.
> 
> We are trying to push some features into fields of an API instead of 
> thinking how to make it simple.
> 
> > > > In our case, it is possible to match several flows in a single 
> > > > rule.  For example, it's possible to set an ethernet, vlan, ip 
> > > > and tcp/udp flows all in a single rule.  We can specify all of 
> > > > these flows in a single raw input flow, which can then be passed 
> > > > to cxgbe flow director to set the corresponding filter.
> > >
> > > I feel we need to define what is an API.
> > > If the application wants to call something specific to the NIC, 
> > > why using the ethdev API? You just have to include cxgbe.h.
> >
> > Well, in that sense, flow-director is also very intel specific, no ?
> 
> Yes. I think the term "flow director" comes from Intel.
> 
> > What we are trying to do is make flow-director generic
> 
> So let's stop calling it flow director.
> We are talking about filtering, right?
> 
Hi Thomas

Are you suggesting chelsio to define a new filter type?

> Why is it so complex? We are talking about packet filtering, not rocket science!
>
The complex is due to different NICs different behavior :-) As I know, it is a common way to use used-define data pass specific infor to driver.

Even flow director is concept from Intel's NIC, but I think it is the generic one comparing with other kinds of filters. So I think that's why Rahul choose it to add their kind of filters.
As I know enic driver also uses flow director API to support their filters.

No matter chelsio NIC filter uses flow director API or define another new filter type. I vote the change happened in struct rte_eth_fdir_input, it provide a RAW Flow type, And there is also a mask field for that, by this way, user can have a flexible way to configure.
And drivers can parse the raw input to define the filter fields.

But for the change happened in struct rte_eth_fdir_action, only SWITCH type is added, Where to switch? All things is in behavior_arg[RTE_ETH_BEHAVIOR_ARG_MAX_LEN]
which is black to user. Maybe your previous define in RFC makes more sense. It's better to add user defined field but not for all args.

Any better suggestion?
  

2016-02-26 01:17, Wu, Jingjing:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-02-25 15:03, Rahul Lakkireddy:
> > > On Wednesday, February 02/24/16, 2016 at 14:17:58 -0800, Thomas Monjalon wrote:
> > > > > In our case, it is possible to match several flows
> > > > > in a single rule.  For example, it's possible to set an ethernet, vlan,
> > > > > ip and tcp/udp flows all in a single rule.  We can specify all of these
> > > > > flows in a single raw input flow, which can then be passed to cxgbe flow
> > > > > director to set the corresponding filter.
> > > >
> > > > I feel we need to define what is an API.
> > > > If the application wants to call something specific to the NIC, why using
> > > > the ethdev API? You just have to include cxgbe.h.
> > >
> > > Well, in that sense, flow-director is also very intel specific, no ?
> > 
> > Yes. I think the term "flow director" comes from Intel.
> > 
> > > What we are trying to do is make flow-director generic
> > 
> > So let's stop calling it flow director.
> > We are talking about filtering, right?
> 
> Are you suggesting chelsio to define a new filter type?
> 
> > Why is it so complex? We are talking about packet filtering, not rocket science!
> >
> The complex is due to different NICs different behavior :-)
> As I know, it is a common way to use used-define data pass specific infor to driver.
> 
> Even flow director is concept from Intel's NIC, but I think it is the generic one comparing
> with other kinds of filters. So I think that's why Rahul choose it to add their kind of filters.
> As I know enic driver also uses flow director API to support their filters.
[...]
> Any better suggestion?

We have a more generic proposal now:
	http://dpdk.org/ml/archives/dev/2016-July/043365.html
Rahul, does it fit your needs?