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

[Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>]

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