[dpdk-dev,v6,5/9] librte_ether:add data structures of VxLAN filter

Message ID 1413881168-20239-6-git-send-email-jijiang.liu@intel.com (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Jijiang Liu Oct. 21, 2014, 8:46 a.m. UTC
  Add definations of the data structures of tunneling packet filter in the rte_eth_ctrl.h file.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>
Acked-by: Jing Chen <jing.d.chen@intel.com>
---
 lib/librte_ether/rte_eth_ctrl.h |   64 +++++++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.h   |   12 -------
 2 files changed, 64 insertions(+), 12 deletions(-)
  

Comments

Thomas Monjalon Oct. 21, 2014, 3:13 p.m. UTC | #1
2014-10-21 16:46, Jijiang Liu:
> +#define RTE_TUNNEL_FILTER_TO_QUEUE 1 /**< point to an queue by filter type */

Sorry, I don't understand what is this value for?

> +#define RTE_TUNNEL_FILTER_IMAC_IVLAN (ETH_TUNNEL_FILTER_IMAC | \
> +					ETH_TUNNEL_FILTER_IVLAN)
> +#define RTE_TUNNEL_FILTER_IMAC_IVLAN_TENID (ETH_TUNNEL_FILTER_IMAC | \
> +					ETH_TUNNEL_FILTER_IVLAN | \
> +					ETH_TUNNEL_FILTER_TENID)
> +#define RTE_TUNNEL_FILTER_IMAC_TENID (ETH_TUNNEL_FILTER_IMAC | \
> +					ETH_TUNNEL_FILTER_TENID)
> +#define RTE_TUNNEL_FILTER_OMAC_TENID_IMAC (ETH_TUNNEL_FILTER_OMAC | \
> +					ETH_TUNNEL_FILTER_TENID | \
> +					ETH_TUNNEL_FILTER_IMAC)

I thought you agree that these definitions are useless?
  
Jijiang Liu Oct. 22, 2014, 2:25 a.m. UTC | #2
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, October 21, 2014 11:13 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 5/9] librte_ether:add data structures of
> VxLAN filter
> 
> 2014-10-21 16:46, Jijiang Liu:
> > +#define RTE_TUNNEL_FILTER_TO_QUEUE 1 /**< point to an queue by filter
> type */
> 
> Sorry, I don't understand what is this value for?
> 
> > +#define RTE_TUNNEL_FILTER_IMAC_IVLAN (ETH_TUNNEL_FILTER_IMAC | \
> > +					ETH_TUNNEL_FILTER_IVLAN)
> > +#define RTE_TUNNEL_FILTER_IMAC_IVLAN_TENID
> (ETH_TUNNEL_FILTER_IMAC | \
> > +					ETH_TUNNEL_FILTER_IVLAN | \
> > +					ETH_TUNNEL_FILTER_TENID)
> > +#define RTE_TUNNEL_FILTER_IMAC_TENID (ETH_TUNNEL_FILTER_IMAC | \
> > +					ETH_TUNNEL_FILTER_TENID)
> > +#define RTE_TUNNEL_FILTER_OMAC_TENID_IMAC
> (ETH_TUNNEL_FILTER_OMAC | \
> > +					ETH_TUNNEL_FILTER_TENID | \
> > +					ETH_TUNNEL_FILTER_IMAC)
> 
> I thought you agree that these definitions are useless?
> 

Sorry, this MAY be  some misunderstanding, I don't think these definition are useless. I just thought change "uint16_t filter_type" is better than define "enum filter_type".

Let me explain here again.
The filter condition are: 
1.  inner MAC + inner VLAN
2. inner MAC + IVLAN + tenant ID
..
5. outer MAC + tenant ID + inner MAC

For each filter condition, we need to check if the mandatory parameters are valid by checking corresponding bit MASK.

An pseudo code example:

       Switch (filter_type)
       Case 1:  //inner MAC + inner VLAN
	If (filter_type & ETH_TUNNEL_FILTER_IMAC )
                            if   (IMAC==NULL)
                                      return -1;

       case 5: // outer MAC + tenant ID + inner MAC
            If (filter_type & ETH_TUNNEL_FILTER_IMAC )
                            if   (IMAC==NULL)
                                      return -1;
          
             If (filter_type & ETH_TUNNEL_FILTER_OMAC )
                            if   (IMAC==NULL)
                                      return -1;
   ......
 









> Thomas
  
Jijiang Liu Oct. 22, 2014, 6:45 a.m. UTC | #3
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, October 21, 2014 11:13 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 5/9] librte_ether:add data structures of
> VxLAN filter
> 
> 2014-10-21 16:46, Jijiang Liu:
> > +#define RTE_TUNNEL_FILTER_TO_QUEUE 1 /**< point to an queue by filter
> type */
> 
> Sorry, I don't understand what is this value for?

This MACRO is used to indicate if user application hope to filter incoming packet(s) to a specific queue(multi-queue configuration is required) using filter type(such as inner MAC + tenant ID).
If the flag is not set, and all incoming packet will always go to queue 0.


> --
> Thomas
  
Thomas Monjalon Oct. 22, 2014, 9:25 a.m. UTC | #4
2014-10-22 06:45, Liu, Jijiang:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2014-10-21 16:46, Jijiang Liu:
> > > +#define RTE_TUNNEL_FILTER_TO_QUEUE 1 /**< point to an queue by filter
> > type */
> > 
> > Sorry, I don't understand what is this value for?
> 
> This MACRO is used to indicate if user application hope to filter
> incoming packet(s) to a specific queue(multi-queue configuration
> is required) using filter type(such as inner MAC + tenant ID).
> If the flag is not set, and all incoming packet will always go
> to queue 0.

1) It's a boolean, so a new constant is not required.
2) You set the variable "to_queue" with this value but the variable is not used
3) Is there a sense to add a filter with this variable to 0?

I think "to_queue" variable and this constant are useless.

PS: it seems I'm the only person worried by the filtering API.
So maybe we shouldn't integrate it at all?
  
Thomas Monjalon Oct. 22, 2014, 9:31 a.m. UTC | #5
2014-10-22 02:25, Liu, Jijiang:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2014-10-21 16:46, Jijiang Liu:
> > > +#define RTE_TUNNEL_FILTER_IMAC_IVLAN (ETH_TUNNEL_FILTER_IMAC | \
> > > +					ETH_TUNNEL_FILTER_IVLAN)
> > > +#define RTE_TUNNEL_FILTER_IMAC_IVLAN_TENID (ETH_TUNNEL_FILTER_IMAC | \
> > > +					ETH_TUNNEL_FILTER_IVLAN | \
> > > +					ETH_TUNNEL_FILTER_TENID)
> > > +#define RTE_TUNNEL_FILTER_IMAC_TENID (ETH_TUNNEL_FILTER_IMAC | \
> > > +					ETH_TUNNEL_FILTER_TENID)
> > > +#define RTE_TUNNEL_FILTER_OMAC_TENID_IMAC (ETH_TUNNEL_FILTER_OMAC | \
> > > +					ETH_TUNNEL_FILTER_TENID | \
> > > +					ETH_TUNNEL_FILTER_IMAC)
> > 
> > I thought you agree that these definitions are useless?
> 
> Sorry, this MAY be  some misunderstanding, I don't think these definition
> are useless. I just thought change "uint16_t filter_type" is better than
> define "enum filter_type".
> 
> Let me explain here again.
> The filter condition are: 
> 1.  inner MAC + inner VLAN
> 2. inner MAC + IVLAN + tenant ID
> ..
> 5. outer MAC + tenant ID + inner MAC
> 
> For each filter condition, we need to check if the mandatory parameters are
> valid by checking corresponding bit MASK.

Checking bit mask doesn't imply to define all combinations of bit masks.
There's probably something obvious that one of us is missing.

> An pseudo code example:
> 
>        Switch (filter_type)
>        Case 1:  //inner MAC + inner VLAN
> 	If (filter_type & ETH_TUNNEL_FILTER_IMAC )
>                             if   (IMAC==NULL)
>                                       return -1;
> 
>        case 5: // outer MAC + tenant ID + inner MAC
>             If (filter_type & ETH_TUNNEL_FILTER_IMAC )
>                             if   (IMAC==NULL)
>                                       return -1;
>           
>              If (filter_type & ETH_TUNNEL_FILTER_OMAC )
>                             if   (IMAC==NULL)
>                                       return -1;
>    ......
  
Jijiang Liu Oct. 22, 2014, 11:03 a.m. UTC | #6
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, October 22, 2014 5:31 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 5/9] librte_ether:add data structures of
> VxLAN filter
> 
> 2014-10-22 02:25, Liu, Jijiang:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-10-21 16:46, Jijiang Liu:
> > > > +#define RTE_TUNNEL_FILTER_IMAC_IVLAN (ETH_TUNNEL_FILTER_IMAC
> | \
> > > > +					ETH_TUNNEL_FILTER_IVLAN)
> > > > +#define RTE_TUNNEL_FILTER_IMAC_IVLAN_TENID
> (ETH_TUNNEL_FILTER_IMAC | \
> > > > +					ETH_TUNNEL_FILTER_IVLAN | \
> > > > +					ETH_TUNNEL_FILTER_TENID)
> > > > +#define RTE_TUNNEL_FILTER_IMAC_TENID (ETH_TUNNEL_FILTER_IMAC
> | \
> > > > +					ETH_TUNNEL_FILTER_TENID)
> > > > +#define RTE_TUNNEL_FILTER_OMAC_TENID_IMAC
> (ETH_TUNNEL_FILTER_OMAC | \
> > > > +					ETH_TUNNEL_FILTER_TENID | \
> > > > +					ETH_TUNNEL_FILTER_IMAC)
> > >
> > > I thought you agree that these definitions are useless?
> >
> > Sorry, this MAY be  some misunderstanding, I don't think these
> > definition are useless. I just thought change "uint16_t filter_type"
> > is better than define "enum filter_type".
> >
> > Let me explain here again.
> > The filter condition are:
> > 1.  inner MAC + inner VLAN
> > 2. inner MAC + IVLAN + tenant ID
> > ..
> > 5. outer MAC + tenant ID + inner MAC
> >
> > For each filter condition, we need to check if the mandatory
> > parameters are valid by checking corresponding bit MASK.
> 
> Checking bit mask doesn't imply to define all combinations of bit masks.
> There's probably something obvious that one of us is missing.

Anybody else have comments on this? 

> > An pseudo code example:
> >
> >        Switch (filter_type)
> >        Case 1:  //inner MAC + inner VLAN
> > 	If (filter_type & ETH_TUNNEL_FILTER_IMAC )
> >                             if   (IMAC==NULL)
> >                                       return -1;
> >
> >        case 5: // outer MAC + tenant ID + inner MAC
> >             If (filter_type & ETH_TUNNEL_FILTER_IMAC )
> >                             if   (IMAC==NULL)
> >                                       return -1;
> >
> >              If (filter_type & ETH_TUNNEL_FILTER_OMAC )
> >                             if   (IMAC==NULL)
> >                                       return -1;
> >    ......
  
Jijiang Liu Oct. 22, 2014, 1:54 p.m. UTC | #7
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, October 22, 2014 5:25 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 5/9] librte_ether:add data structures of
> VxLAN filter
> 
> 2014-10-22 06:45, Liu, Jijiang:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-10-21 16:46, Jijiang Liu:
> > > > +#define RTE_TUNNEL_FILTER_TO_QUEUE 1 /**< point to an queue by
> > > > +filter
> > > type */
> > >
> > > Sorry, I don't understand what is this value for?
> >
> > This MACRO is used to indicate if user application hope to filter
> > incoming packet(s) to a specific queue(multi-queue configuration is
> > required) using filter type(such as inner MAC + tenant ID).
> > If the flag is not set, and all incoming packet will always go to
> > queue 0.
> 
> 1) It's a boolean, so a new constant is not required.
OK. define bool to_queue. 
> 2) You set the variable "to_queue" with this value but the variable is not
> used
> 3) Is there a sense to add a filter with this variable to 0?
> 
> I think "to_queue" variable and this constant are useless.
No, It is useful. I will change to_queue to be configurable. 
> PS: it seems I'm the only person worried by the filtering API.
> So maybe we shouldn't integrate it at all?

I know you are joking. 
 We should certainly integrate the feature into dpdk.org
> --
> Thomas
  
Chilikin, Andrey Oct. 23, 2014, 9:06 a.m. UTC | #8
For me these defines make perfect sense - tunnelling filters require combinations of different tunnel components, but not all combinations are valid. So defining valid combinations separately helps. 

Regards,
Andrey

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, October 22, 2014 5:31 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 5/9] librte_ether:add data 
> structures of VxLAN filter
> 
> 2014-10-22 02:25, Liu, Jijiang:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-10-21 16:46, Jijiang Liu:
> > > > +#define RTE_TUNNEL_FILTER_IMAC_IVLAN (ETH_TUNNEL_FILTER_IMAC
> | \
> > > > +					ETH_TUNNEL_FILTER_IVLAN)
> > > > +#define RTE_TUNNEL_FILTER_IMAC_IVLAN_TENID
> (ETH_TUNNEL_FILTER_IMAC | \
> > > > +					ETH_TUNNEL_FILTER_IVLAN | \
> > > > +					ETH_TUNNEL_FILTER_TENID)
> > > > +#define RTE_TUNNEL_FILTER_IMAC_TENID (ETH_TUNNEL_FILTER_IMAC
> | \
> > > > +					ETH_TUNNEL_FILTER_TENID)
> > > > +#define RTE_TUNNEL_FILTER_OMAC_TENID_IMAC
> (ETH_TUNNEL_FILTER_OMAC | \
> > > > +					ETH_TUNNEL_FILTER_TENID | \
> > > > +					ETH_TUNNEL_FILTER_IMAC)
> > >
> > > I thought you agree that these definitions are useless?
> >
> > Sorry, this MAY be  some misunderstanding, I don't think these 
> > definition are useless. I just thought change "uint16_t filter_type"
> > is better than define "enum filter_type".
> >
> > Let me explain here again.
> > The filter condition are:
> > 1.  inner MAC + inner VLAN
> > 2. inner MAC + IVLAN + tenant ID
> > ..
> > 5. outer MAC + tenant ID + inner MAC
> >
> > For each filter condition, we need to check if the mandatory 
> > parameters are valid by checking corresponding bit MASK.
> 
> Checking bit mask doesn't imply to define all combinations of bit masks.
> There's probably something obvious that one of us is missing.

Anybody else have comments on this? 

> > An pseudo code example:
> >
> >        Switch (filter_type)
> >        Case 1:  //inner MAC + inner VLAN
> > 	If (filter_type & ETH_TUNNEL_FILTER_IMAC )
> >                             if   (IMAC==NULL)
> >                                       return -1;
> >
> >        case 5: // outer MAC + tenant ID + inner MAC
> >             If (filter_type & ETH_TUNNEL_FILTER_IMAC )
> >                             if   (IMAC==NULL)
> >                                       return -1;
> >
> >              If (filter_type & ETH_TUNNEL_FILTER_OMAC )
> >                             if   (IMAC==NULL)
> >                                       return -1;
> >    ......
  

Patch

diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
index df21ac6..a04333c 100644
--- a/lib/librte_ether/rte_eth_ctrl.h
+++ b/lib/librte_ether/rte_eth_ctrl.h
@@ -51,6 +51,7 @@  extern "C" {
  */
 enum rte_filter_type {
 	RTE_ETH_FILTER_NONE = 0,
+	RTE_ETH_FILTER_TUNNEL,
 	RTE_ETH_FILTER_MAX
 };
 
@@ -71,6 +72,69 @@  enum rte_filter_op {
 	RTE_ETH_FILTER_OP_MAX
 };
 
+/**
+ * filter type of tunneling packet
+ */
+#define ETH_TUNNEL_FILTER_OMAC  0x01 /**< filter by outer MAC addr */
+#define ETH_TUNNEL_FILTER_OIP   0x02 /**< filter by outer IP Addr */
+#define ETH_TUNNEL_FILTER_TENID 0x04 /**< filter by tenant ID */
+#define ETH_TUNNEL_FILTER_IMAC  0x08 /**< filter by inner MAC addr */
+#define ETH_TUNNEL_FILTER_IVLAN 0x10 /**< filter by inner VLAN ID */
+#define ETH_TUNNEL_FILTER_IIP   0x20 /**< filter by inner IP addr */
+
+#define RTE_TUNNEL_FILTER_TO_QUEUE 1 /**< point to an queue by filter type */
+
+#define RTE_TUNNEL_FILTER_IMAC_IVLAN (ETH_TUNNEL_FILTER_IMAC | \
+					ETH_TUNNEL_FILTER_IVLAN)
+#define RTE_TUNNEL_FILTER_IMAC_IVLAN_TENID (ETH_TUNNEL_FILTER_IMAC | \
+					ETH_TUNNEL_FILTER_IVLAN | \
+					ETH_TUNNEL_FILTER_TENID)
+#define RTE_TUNNEL_FILTER_IMAC_TENID (ETH_TUNNEL_FILTER_IMAC | \
+					ETH_TUNNEL_FILTER_TENID)
+#define RTE_TUNNEL_FILTER_OMAC_TENID_IMAC (ETH_TUNNEL_FILTER_OMAC | \
+					ETH_TUNNEL_FILTER_TENID | \
+					ETH_TUNNEL_FILTER_IMAC)
+
+/**
+ *  Select IPv4 or IPv6 for tunnel filters.
+ */
+enum rte_tunnel_iptype {
+	RTE_TUNNEL_IPTYPE_IPV4 = 0, /**< IPv4. */
+	RTE_TUNNEL_IPTYPE_IPV6,     /**< IPv6. */
+};
+
+/**
+ * Tunneled type.
+ */
+enum rte_eth_tunnel_type {
+	RTE_TUNNEL_TYPE_NONE = 0,
+	RTE_TUNNEL_TYPE_VXLAN,
+	RTE_TUNNEL_TYPE_GENEVE,
+	RTE_TUNNEL_TYPE_TEREDO,
+	RTE_TUNNEL_TYPE_NVGRE,
+	RTE_TUNNEL_TYPE_MAX,
+};
+
+/**
+ * Tunneling Packet filter configuration.
+ */
+struct rte_eth_tunnel_filter_conf {
+	struct ether_addr *outer_mac;  /**< Outer MAC address filter. */
+	struct ether_addr *inner_mac;  /**< Inner MAC address filter. */
+	uint16_t inner_vlan;           /**< Inner VLAN filter. */
+	enum rte_tunnel_iptype ip_type; /**< IP address type. */
+	union {
+		uint32_t ipv4_addr;    /**< IPv4 source address to match. */
+		uint32_t ipv6_addr[4]; /**< IPv6 source address to match. */
+	} ip_addr; /**< IPv4/IPv6 source address to match (union of above). */
+
+	uint16_t filter_type;   /**< Filter type. */
+	uint8_t to_queue;       /**< Use MAC and VLAN to point to a queue. */
+	enum rte_eth_tunnel_type tunnel_type; /**< Tunnel Type. */
+	uint32_t tenant_id;     /** < Tenant number. */
+	uint16_t queue_id;      /** < queue number. */
+};
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 9ad11ec..c8fb89a 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -718,18 +718,6 @@  struct rte_eth_udp_tunnel {
 };
 
 /**
- * Tunneled type.
- */
-enum rte_eth_tunnel_type {
-	RTE_TUNNEL_TYPE_NONE = 0,
-	RTE_TUNNEL_TYPE_VXLAN,
-	RTE_TUNNEL_TYPE_GENEVE,
-	RTE_TUNNEL_TYPE_TEREDO,
-	RTE_TUNNEL_TYPE_NVGRE,
-	RTE_TUNNEL_TYPE_MAX,
-};
-
-/**
  *  Possible l4type of FDIR filters.
  */
 enum rte_l4type {