[dpdk-dev,RFC,v2,1/2] ethdev: add packet filter flow and new behavior switch to fdir

  Add a new packet filter flow that allows filtering a packet based on
matching ingress port, ethertype, vlan, ip, and tcp/udp fields, i.e.
matching based on any or all fields at the same time.

Add the ability to provide masks for fields in flow to allow range of
values. Allow selection of maskfull vs maskless filter types. Provide
mechanism to set priority to maskfull vs maskless filter types when
packet matches several filter types.

Add a new vlan flow containing inner and outer vlan to match. Add tos,
proto, and ttl fields that can be matched for ipv4 flow.  Add tc,
flow_label, next_header, and hop_limit fields that can be matched for
ipv6 flow.

Add a new behavior switch.

Add the ability to provide behavior arguments to allow insert/deletion/
swapping of matched fields in the flow.  Useful when rewriting matched
fields with new values.  Adds arguments for port, mac, vlan, and nat.
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>
---
v2:
1. Added ttl to rte_eth_ipv4_flow and tc, flow_label, next_header,
   and hop_limit to rte_eth_ipv6_flow.

2. Added new field type to rte_eth_pkt_filter_flow to differentiate
   between maskfull and maskless filter types.

3. Added new field prio to rte_eth_pkt_filter_flow to allow setting
   priority over maskfull or maskless when packet matches multiple
   filter types.

4. Added new behavior sub op RTE_FDIR_BEHAVIOR_SUB_OP_SWAP to allow
   swapping fields in matched flows. Useful when swapping mac addresses
   in hardware before switching.

 lib/librte_ether/rte_eth_ctrl.h | 127 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 1 deletion(-)
  

> 
> diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
> index ce224ad..5cc22a0 100644
> --- a/lib/librte_ether/rte_eth_ctrl.h
> +++ b/lib/librte_ether/rte_eth_ctrl.h
> @@ -74,7 +74,11 @@ 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_PKT_FILTER_IPV4_TCP 18 #define
> +RTE_ETH_FLOW_PKT_FILTER_IPV4_UDP 19 #define
> +RTE_ETH_FLOW_PKT_FILTER_IPV6_TCP 20 #define
> +RTE_ETH_FLOW_PKT_FILTER_IPV6_UDP 21
> +#define RTE_ETH_FLOW_MAX                22
> 
How to distinguish RTE_ETH_FLOW_PKT_FILTER_IPV4_XX with RTE_ETH_FLOW_NONFRAG_IPV4_XX, what is the difference?
>  /**
>   * Feature filter types
> @@ -407,6 +411,9 @@ struct rte_eth_l2_flow {  struct rte_eth_ipv4_flow {
>  	uint32_t src_ip;      /**< IPv4 source address to match. */
>  	uint32_t dst_ip;      /**< IPv4 destination address to match. */
> +	uint8_t tos;          /**< IPV4 type of service to match. */
> +	uint8_t proto;        /**< IPV4 proto to match. */
> +	uint8_t ttl;          /**< IPV4 time to live to match. */
>  };
> 
>  /**
> @@ -443,6 +450,10 @@ struct rte_eth_sctpv4_flow {  struct
> rte_eth_ipv6_flow {
>  	uint32_t src_ip[4];      /**< IPv6 source address to match. */
>  	uint32_t dst_ip[4];      /**< IPv6 destination address to match. */
> +	uint8_t  tc;             /**< IPv6 traffic class to match. */
> +	uint32_t flow_label;     /**< IPv6 flow label to match. */
> +	uint8_t  next_header;    /**< IPv6 next header to match. */
> +	uint8_t  hop_limit;      /**< IPv6 hop limits to match. */
>  };
> 
There is also a patch http://dpdk.org/dev/patchwork/patch/9661/ which added these fields. Maybe we can merge them together.
> +struct rte_eth_pkt_filter_flow {
> +	enum rte_eth_pkt_filter_type type;   /**< Type of filter */
> +	enum rte_eth_pkt_filter_type prio;
> +	/**< Prioritize the filter type when a packet matches several types */
> +	struct rte_eth_pkt_filter pkt;      /**< Packet fields to match. */
> +	struct rte_eth_pkt_filter mask;     /**< Mask for matched fields. */
> +};
> +
> +/**
>   * An union contains the inputs for all types of flow
>   */
>  union rte_eth_fdir_flow {
> @@ -514,6 +570,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_pkt_filter_flow pkt_filter_flow;
>  };
Why not use rte_eth_XX_flow directly but add a new one? Is it because of the mask? If so, how about to add a field in rte_eth_fdir_input like:
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;
    struct rte_eth_fdir_flow_ext flow_ext;
    /**< Additional fields to match */
};

Thanks
Jingjing
  

Hi Jingjing,

On Tuesday, January 01/12/16, 2016 at 17:12:47 -0800, Wu, Jingjing wrote:
> > 
> > diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
> > index ce224ad..5cc22a0 100644
> > --- a/lib/librte_ether/rte_eth_ctrl.h
> > +++ b/lib/librte_ether/rte_eth_ctrl.h
> > @@ -74,7 +74,11 @@ 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_PKT_FILTER_IPV4_TCP 18 #define
> > +RTE_ETH_FLOW_PKT_FILTER_IPV4_UDP 19 #define
> > +RTE_ETH_FLOW_PKT_FILTER_IPV6_TCP 20 #define
> > +RTE_ETH_FLOW_PKT_FILTER_IPV6_UDP 21
> > +#define RTE_ETH_FLOW_MAX                22
> > 
> How to distinguish RTE_ETH_FLOW_PKT_FILTER_IPV4_XX with RTE_ETH_FLOW_NONFRAG_IPV4_XX, what is the difference?

The packet filter flow is basically a superset containing Ethernet,
vlan, ipv4/ipv6 and tcp/udp flows whose fields can all be matched at
the same time, unlike in case of the current flow director which seems
to match only one of the flows at any given time.  Additionally, it also
allows specifying masks.  I separated the two to make this meaning
explicit.  If this is not necessary, then I will merge them.

> >  /**
> >   * Feature filter types
> > @@ -407,6 +411,9 @@ struct rte_eth_l2_flow {  struct rte_eth_ipv4_flow {
> >  	uint32_t src_ip;      /**< IPv4 source address to match. */
> >  	uint32_t dst_ip;      /**< IPv4 destination address to match. */
> > +	uint8_t tos;          /**< IPV4 type of service to match. */
> > +	uint8_t proto;        /**< IPV4 proto to match. */
> > +	uint8_t ttl;          /**< IPV4 time to live to match. */
> >  };
> > 
> >  /**
> > @@ -443,6 +450,10 @@ struct rte_eth_sctpv4_flow {  struct
> > rte_eth_ipv6_flow {
> >  	uint32_t src_ip[4];      /**< IPv6 source address to match. */
> >  	uint32_t dst_ip[4];      /**< IPv6 destination address to match. */
> > +	uint8_t  tc;             /**< IPv6 traffic class to match. */
> > +	uint32_t flow_label;     /**< IPv6 flow label to match. */
> > +	uint8_t  next_header;    /**< IPv6 next header to match. */
> > +	uint8_t  hop_limit;      /**< IPv6 hop limits to match. */
> >  };
> > 
> There is also a patch http://dpdk.org/dev/patchwork/patch/9661/ which added these fields. Maybe we can merge them together.

Yes, we can merge them.  Would you like me to merge your patch here?

> > +struct rte_eth_pkt_filter_flow {
> > +	enum rte_eth_pkt_filter_type type;   /**< Type of filter */
> > +	enum rte_eth_pkt_filter_type prio;
> > +	/**< Prioritize the filter type when a packet matches several types */
> > +	struct rte_eth_pkt_filter pkt;      /**< Packet fields to match. */
> > +	struct rte_eth_pkt_filter mask;     /**< Mask for matched fields. */
> > +};
> > +
> > +/**
> >   * An union contains the inputs for all types of flow
> >   */
> >  union rte_eth_fdir_flow {
> > @@ -514,6 +570,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_pkt_filter_flow pkt_filter_flow;
> >  };
> Why not use rte_eth_XX_flow directly but add a new one? Is it because of the mask? If so, how about to add a field in rte_eth_fdir_input like:
> 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;
>     struct rte_eth_fdir_flow_ext flow_ext;
>     /**< Additional fields to match */
> };
> 
> Thanks
> Jingjing

The current rte_eth_XX_flow only allow matching _one_ of the flows
because of the union.  In contrast, rte_eth_pkt_filter_flow can match
several flows at the same time; i.e. it can match ethernet, vlan, ip,
and tcp/udp all at the same time.  rte_eth_pkt_filter_flow is basically
a superset containing several flows that can be matched at the same
time.

In our Chelsio T5 hardware, it's possible to have several flows that can
be matched in a single rule. This is why I've created a superset that
can match several flows in the same rule.

Thanks,
Rahul
  

Hi, Rahul

> -----Original Message-----
> From: Rahul Lakkireddy [mailto:rahul.lakkireddy@chelsio.com]
> Sent: Wednesday, January 13, 2016 4:49 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org; Felix Marti; Kumar A S; Nirranjan Kirubaharan
> Subject: Re: [dpdk-dev] [RFC v2 1/2] ethdev: add packet filter flow and new behavior
> switch to fdir
> 
> Hi Jingjing,
> 
> On Tuesday, January 01/12/16, 2016 at 17:12:47 -0800, Wu, Jingjing wrote:
> > >
> > > diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
> > > index ce224ad..5cc22a0 100644
> > > --- a/lib/librte_ether/rte_eth_ctrl.h
> > > +++ b/lib/librte_ether/rte_eth_ctrl.h
> > > @@ -74,7 +74,11 @@ 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_PKT_FILTER_IPV4_TCP 18 #define
> > > +RTE_ETH_FLOW_PKT_FILTER_IPV4_UDP 19 #define
> > > +RTE_ETH_FLOW_PKT_FILTER_IPV6_TCP 20 #define
> > > +RTE_ETH_FLOW_PKT_FILTER_IPV6_UDP 21
> > > +#define RTE_ETH_FLOW_MAX                22
> > >
> > How to distinguish RTE_ETH_FLOW_PKT_FILTER_IPV4_XX with
> RTE_ETH_FLOW_NONFRAG_IPV4_XX, what is the difference?
> 
> The packet filter flow is basically a superset containing Ethernet,
> vlan, ipv4/ipv6 and tcp/udp flows whose fields can all be matched at
> the same time, unlike in case of the current flow director which seems
> to match only one of the flows at any given time.  Additionally, it also
> allows specifying masks.  I separated the two to make this meaning
> explicit.  If this is not necessary, then I will merge them.
Thanks for clarification, now I understand. How about just define one to indicate using pkt_filter?
And move the IPV4_XX info to the structure rte_eth_pkt_filter?
> > There is also a patch http://dpdk.org/dev/patchwork/patch/9661/ which added these
> fields. Maybe we can merge them together.
> 
> Yes, we can merge them.  Would you like me to merge your patch here?
The i40e driver implementation is done based on the change. If you'd like to merge,
maybe other patches in the patch set also need to be merged. Anyway, I think
maintainer can deal with it. 

> 
> The current rte_eth_XX_flow only allow matching _one_ of the flows
> because of the union.  In contrast, rte_eth_pkt_filter_flow can match
> several flows at the same time; i.e. it can match ethernet, vlan, ip,
> and tcp/udp all at the same time.  rte_eth_pkt_filter_flow is basically
> a superset containing several flows that can be matched at the same
> time.
> 
> In our Chelsio T5 hardware, it's possible to have several flows that can
> be matched in a single rule. This is why I've created a superset that
> can match several flows in the same rule.
> 

> Thanks,
> Rahul

Thanks for clarification, it's clear. And it's great to have this feature.
Even it is a superset containing several flows, we still can use the
existing structs like
struct rte_eth_ipv4_flow {
	struct rte_eth_l2_flow ether;
	struct rte_eth_vlan_flow vlan;
	uint32_t src_ip;
	uint32_t dst_ip;
};
What do you think?

Thanks
Jingjing
  

Hi, Rahul

Just another thought, please consider about it:

Add a new flow type like

#define RTE_ETH_FLOW_IPV6_UDP_EX        17
+#define RTE_ETH_FLOW_RAW_PKT             18

Then add a new item in rte_eth_fdir_flow
union rte_eth_fdir_flow {
	struct rte_eth_l2_flow     l2_flow;
	struct rte_eth_udpv4_flow  udp4_flow;
	struct rte_eth_tcpv4_flow  tcp4_flow;
	struct rte_eth_sctpv4_flow sctp4_flow;
	struct rte_eth_ipv4_flow   ip4_flow;
	struct rte_eth_udpv6_flow  udp6_flow;
	struct rte_eth_tcpv6_flow  tcp6_flow;
	struct rte_eth_sctpv6_flow sctp6_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[80];
};

Then add mask item in rte_eth_fdir_input:

struct rte_eth_fdir_input {
	uint16_t flow_type;
	union rte_eth_fdir_flow flow;
+	union rte_eth_fdir_flow flow_mask;
	/**< Flow fields to match, dependent on flow_type */
	struct rte_eth_fdir_flow_ext flow_ext;
	/**< Additional fields to match */
};

Then the filter can be added just in a format of raw packet, it looks generic, and even other NIC can use this too.

Thanks
Jingjing
> -----Original Message-----
> From: Wu, Jingjing
> Sent: Wednesday, January 13, 2016 9:17 PM
> To: Rahul Lakkireddy
> Cc: dev@dpdk.org; Felix Marti; Kumar A S; Nirranjan Kirubaharan
> Subject: RE: [dpdk-dev] [RFC v2 1/2] ethdev: add packet filter flow and new
> behavior switch to fdir
>
  

Hi Jingjing,

On Thursday, January 01/14/16, 2016 at 00:48:17 -0800, Wu, Jingjing wrote:
> Hi, Rahul
> 
> Just another thought, please consider about it:
> 
> Add a new flow type like
> 
> #define RTE_ETH_FLOW_IPV6_UDP_EX        17
> +#define RTE_ETH_FLOW_RAW_PKT             18
> 
> Then add a new item in rte_eth_fdir_flow
> union rte_eth_fdir_flow {
> 	struct rte_eth_l2_flow     l2_flow;
> 	struct rte_eth_udpv4_flow  udp4_flow;
> 	struct rte_eth_tcpv4_flow  tcp4_flow;
> 	struct rte_eth_sctpv4_flow sctp4_flow;
> 	struct rte_eth_ipv4_flow   ip4_flow;
> 	struct rte_eth_udpv6_flow  udp6_flow;
> 	struct rte_eth_tcpv6_flow  tcp6_flow;
> 	struct rte_eth_sctpv6_flow sctp6_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[80];
> };
> 
> Then add mask item in rte_eth_fdir_input:
> 
> struct rte_eth_fdir_input {
> 	uint16_t flow_type;
> 	union rte_eth_fdir_flow flow;
> +	union rte_eth_fdir_flow flow_mask;
> 	/**< Flow fields to match, dependent on flow_type */
> 	struct rte_eth_fdir_flow_ext flow_ext;
> 	/**< Additional fields to match */
> };
> 
> Then the filter can be added just in a format of raw packet, it looks generic, and even other NIC can use this too.
> 
> Thanks
> Jingjing

This approach seems generic enough to allow any vendor specific data
to be passed in filter as well.  However, 80 seems to be too low for
multiple flow types that can be combined in the same filter rule.
I think size of 256 seems reasonable.

Could the same thing be done for action arguments as well? Can we add
the same generic info to rte_eth_fdir_action too?

struct rte_eth_fdir_action {
        uint16_t rx_queue;
	enum rte_eth_fdir_behavior behavior;
	enum rte_eth_fdir_status report_status;
	uint8_t flex_off;
+       uint8_t behavior_arg[256];
};

This way, we can pass vendor specific action arguments too. What do
you think?

Also, now if we take this approach then, I am wondering, that all
vendors would need to document their own vendor-specific format of
taking filter match and filter action arguments, right?

And probably, even come up with their own example application showing
how to apply filters via dpdk on their card?

Thanks,
Rahul
  

Hi, Rahul

> This approach seems generic enough to allow any vendor specific data
> to be passed in filter as well.  However, 80 seems to be too low for
> multiple flow types that can be combined in the same filter rule.
> I think size of 256 seems reasonable.
>
Yes, 80 is just an example. 
> Could the same thing be done for action arguments as well? Can we add
> the same generic info to rte_eth_fdir_action too?
> 
> struct rte_eth_fdir_action {
>         uint16_t rx_queue;
> 	enum rte_eth_fdir_behavior behavior;
> 	enum rte_eth_fdir_status report_status;
> 	uint8_t flex_off;
> +       uint8_t behavior_arg[256];
> };
> 
> This way, we can pass vendor specific action arguments too. What do
> you think?
Yes, it also makes sense.
> Also, now if we take this approach then, I am wondering, that all
> vendors would need to document their own vendor-specific format of
> taking filter match and filter action arguments, right?
> 
> And probably, even come up with their own example application showing
> how to apply filters via dpdk on their card?
Yes, I guess it will be better to doc it or example it. Even currently, different kinds of NIC may need different configuration.
Or you can add description (how to configure) in your driver's comment log?
Not sure about the others' opinion?

Thanks
Jingjing
  

Hi Jingjing,

On Thursday, January 01/14/16, 2016 at 17:30:53 -0800, Wu, Jingjing wrote:
> Hi, Rahul
> 
> > This approach seems generic enough to allow any vendor specific data
> > to be passed in filter as well.  However, 80 seems to be too low for
> > multiple flow types that can be combined in the same filter rule.
> > I think size of 256 seems reasonable.
> >
> Yes, 80 is just an example. 
> > Could the same thing be done for action arguments as well? Can we add
> > the same generic info to rte_eth_fdir_action too?
> > 
> > struct rte_eth_fdir_action {
> >         uint16_t rx_queue;
> > 	enum rte_eth_fdir_behavior behavior;
> > 	enum rte_eth_fdir_status report_status;
> > 	uint8_t flex_off;
> > +       uint8_t behavior_arg[256];
> > };
> > 
> > This way, we can pass vendor specific action arguments too. What do
> > you think?
> Yes, it also makes sense.
> > Also, now if we take this approach then, I am wondering, that all
> > vendors would need to document their own vendor-specific format of
> > taking filter match and filter action arguments, right?
> > 
> > And probably, even come up with their own example application showing
> > how to apply filters via dpdk on their card?
> Yes, I guess it will be better to doc it or example it. Even currently, different kinds of NIC may need different configuration.
> Or you can add description (how to configure) in your driver's comment log?
> Not sure about the others' opinion?
> 
> Thanks
> Jingjing

Ok.  We will wait for a couple of days to get more opinions from others.
If the above generic approach is agreeable to everyone, then I will post
the patch series using this new approach.

Thanks,
Rahul