[dpdk-dev,v4,07/12] librte_ether: extend flow director struct

Message ID 1457580346-18550-8-git-send-email-jingjing.wu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Bruce Richardson
Headers

Commit Message

Jingjing Wu March 10, 2016, 3:25 a.m. UTC
  This patch changed rte_eth_fdir_flow from union to struct to
support more packets formats, for example, Vxlan and GRE tunnel
packets with IP inner frame.

This patch also add new RTE_FDIR_TUNNEL_TYPE_GRE enum.

Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>
---
 doc/guides/rel_notes/deprecation.rst   |  4 ----
 doc/guides/rel_notes/release_16_04.rst |  3 +++
 lib/librte_ether/rte_eth_ctrl.h        | 27 +++++++++++++++------------
 3 files changed, 18 insertions(+), 16 deletions(-)
  

Comments

Thomas Monjalon March 18, 2016, 11:44 a.m. UTC | #1
Hi Jingjing,

2016-03-10 11:25, Jingjing Wu:
> This patch changed rte_eth_fdir_flow from union to struct to
> support more packets formats, for example, Vxlan and GRE tunnel
> packets with IP inner frame.

I think we need a lot more explanations about how it should work.
From this point we should collect some acknowledgements from the
maintainers of other drivers having this kind of flow steering need.
Maybe that a better API, more generic, is possible.

> This patch also add new RTE_FDIR_TUNNEL_TYPE_GRE enum.

OK to add GRE to the existing API.

> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> Acked-by: Helin Zhang <helin.zhang@intel.com>
[...]
>  /**
> - * An union contains the inputs for all types of flow
> + * A struct contains the inputs for all types of 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_fdir_flow {
> +	union {
> +		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;
>  };

Please explain somewhere how to use this API change in order to have more
discussions with other maintainers.

I'm sorry to comment this change only now. I took time to realize that
we need more consensus about the filtering API to make it usable by
more drivers.

For the 16.04 release, I suggest to remove this change from the series.
Thanks for your understanding.
  
Jingjing Wu March 20, 2016, 8:56 a.m. UTC | #2
Hi, Thomas


> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, March 18, 2016 7:44 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 07/12] librte_ether: extend flow director struct
> 
> Hi Jingjing,
> 
> 2016-03-10 11:25, Jingjing Wu:
> > This patch changed rte_eth_fdir_flow from union to struct to
> > support more packets formats, for example, Vxlan and GRE tunnel
> > packets with IP inner frame.
> 
> I think we need a lot more explanations about how it should work.
> From this point we should collect some acknowledgements from the
> maintainers of other drivers having this kind of flow steering need.
> Maybe that a better API, more generic, is possible.
> 
> > This patch also add new RTE_FDIR_TUNNEL_TYPE_GRE enum.
> 
> OK to add GRE to the existing API.
> 
> > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> > Acked-by: Helin Zhang <helin.zhang@intel.com>
> [...]
> >  /**
> > - * An union contains the inputs for all types of flow
> > + * A struct contains the inputs for all types of 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_fdir_flow {
> > +	union {
> > +		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;
> >  };
> 
> Please explain somewhere how to use this API change in order to have more
> discussions with other maintainers.
> 
> I'm sorry to comment this change only now. I took time to realize that
> we need more consensus about the filtering API to make it usable by
> more drivers.
>

> For the 16.04 release, I suggest to remove this change from the series.
> Thanks for your understanding.
OK. Understand that we need to comments more on the change. And as you know, the whole patch set actually contains two changes on filter API: One the change is in this patch, which adds tunnel filtering using flow director, another one is the patch "[PATCH v4,05/12] i40e: extend flow director to filter by IP " (http://www.dpdk.org/dev/patchwork/patch/11358/ ). If you are OK to the latter one, I will send another version which just contains the change and drops tunnel supporting in 16.04 release.
And let's discuss more on the filtering API in future.

Thanks
Jingjing
  
Jingjing Wu March 20, 2016, 9:02 a.m. UTC | #3
> -----Original Message-----
> From: Wu, Jingjing
> Sent: Sunday, March 20, 2016 4:57 PM
> To: 'Thomas Monjalon' <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v4 07/12] librte_ether: extend flow director struct
> 
> Hi, Thomas
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Friday, March 18, 2016 7:44 PM
> > To: Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v4 07/12] librte_ether: extend flow director struct
> >
> > Hi Jingjing,
> >
> > 2016-03-10 11:25, Jingjing Wu:
> > > This patch changed rte_eth_fdir_flow from union to struct to
> > > support more packets formats, for example, Vxlan and GRE tunnel
> > > packets with IP inner frame.
> >
> > I think we need a lot more explanations about how it should work.
> > From this point we should collect some acknowledgements from the
> > maintainers of other drivers having this kind of flow steering need.
> > Maybe that a better API, more generic, is possible.
> >
> > > This patch also add new RTE_FDIR_TUNNEL_TYPE_GRE enum.
> >
> > OK to add GRE to the existing API.
> >
> > > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> > > Acked-by: Helin Zhang <helin.zhang@intel.com>
> > [...]
> > >  /**
> > > - * An union contains the inputs for all types of flow
> > > + * A struct contains the inputs for all types of 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_fdir_flow {
> > > +	union {
> > > +		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;
> > >  };
> >
> > Please explain somewhere how to use this API change in order to have more
> > discussions with other maintainers.
> >
> > I'm sorry to comment this change only now. I took time to realize that
> > we need more consensus about the filtering API to make it usable by
> > more drivers.
> >
> 
> > For the 16.04 release, I suggest to remove this change from the series.
> > Thanks for your understanding.
> OK. Understand that we need to comments more on the change. And as you know, the
> whole patch set actually contains two changes on filter API: One the change is in this patch,
> which adds tunnel filtering using flow director, another one is the patch "[PATCH v4,05/12]
> i40e: extend flow director to filter by IP "
> (http://www.dpdk.org/dev/patchwork/patch/11358/ ). If you are OK to the latter one, I will
> send another version which just contains the change and drops tunnel supporting in 16.04
> release.
Correct the mistake:
The other change is "[v4,01/12] ethdev: extend flow director for input selection" http://www.dpdk.org/dev/patchwork/patch/11354/. 

> And let's discuss more on the filtering API in future.
> 
> Thanks
> Jingjing
  
Thomas Monjalon March 20, 2016, 10:38 a.m. UTC | #4
2016-03-20 09:02, Wu, Jingjing:
> From: Wu, Jingjing
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-03-10 11:25, Jingjing Wu:
> > > > This patch changed rte_eth_fdir_flow from union to struct to
> > > > support more packets formats, for example, Vxlan and GRE tunnel
> > > > packets with IP inner frame.
> > >
> > > I think we need a lot more explanations about how it should work.
> > > From this point we should collect some acknowledgements from the
> > > maintainers of other drivers having this kind of flow steering need.
> > > Maybe that a better API, more generic, is possible.
> > >
> > > > This patch also add new RTE_FDIR_TUNNEL_TYPE_GRE enum.
> > >
> > > OK to add GRE to the existing API.
> > >
> > > > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> > > > Acked-by: Helin Zhang <helin.zhang@intel.com>
> > > [...]
> > > >  /**
> > > > - * An union contains the inputs for all types of flow
> > > > + * A struct contains the inputs for all types of 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_fdir_flow {
> > > > +	union {
> > > > +		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;
> > > >  };
> > >
> > > Please explain somewhere how to use this API change in order to have more
> > > discussions with other maintainers.
> > >
> > > I'm sorry to comment this change only now. I took time to realize that
> > > we need more consensus about the filtering API to make it usable by
> > > more drivers.
> > >
> > 
> > > For the 16.04 release, I suggest to remove this change from the series.
> > > Thanks for your understanding.
> > OK. Understand that we need to comments more on the change. And as you know, the
> > whole patch set actually contains two changes on filter API: One the change is in this patch,
> > which adds tunnel filtering using flow director, another one is the patch "[PATCH v4,05/12]
> > i40e: extend flow director to filter by IP "
> > (http://www.dpdk.org/dev/patchwork/patch/11358/ ). If you are OK to the latter one, I will
> > send another version which just contains the change and drops tunnel supporting in 16.04
> > release.
> Correct the mistake:
> The other change is "[v4,01/12] ethdev: extend flow director for input selection" http://www.dpdk.org/dev/patchwork/patch/11354/. 

I'm OK with patch 01 and "RTE_FDIR_TUNNEL_TYPE_GRE" add in patch 07.

> > And let's discuss more on the filtering API in future.

Thanks
  

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index e94d4a2..7fa8639 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -20,10 +20,6 @@  Deprecation Notices
   tables (512 queues).
   It should be integrated in release 2.3.
 
-* ABI changes are planned for struct rte_eth_fdir_flow in order to support
-  extend flow director's input set. The release 2.2 does not contain these ABI
-  changes, but release 2.3 will, and no backwards compatibility is planned.
-
 * ABI changes are planned for rte_eth_ipv4_flow and rte_eth_ipv6_flow to
   include more fields to be matched against. The release 2.2 does not
   contain these ABI changes, but release 2.3 will.
diff --git a/doc/guides/rel_notes/release_16_04.rst b/doc/guides/rel_notes/release_16_04.rst
index 5abf48a..87ec402 100644
--- a/doc/guides/rel_notes/release_16_04.rst
+++ b/doc/guides/rel_notes/release_16_04.rst
@@ -238,6 +238,9 @@  ABI Changes
   the previous releases and made in this release. Use fixed width quotes for
   ``rte_function_names`` or ``rte_struct_names``. Use the past tense.
 
+* The ethdev flow director structure ``rte_eth_fdir_flow`` structure was
+  changed. New fields were added to extend flow director's input set, and
+  organizing is also changed to support multiple input format.
 
 Shared Library Versions
 -----------------------
diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
index e2ac686..5b2a6ed 100644
--- a/lib/librte_ether/rte_eth_ctrl.h
+++ b/lib/librte_ether/rte_eth_ctrl.h
@@ -495,6 +495,7 @@  enum rte_eth_fdir_tunnel_type {
 	RTE_FDIR_TUNNEL_TYPE_UNKNOWN = 0,
 	RTE_FDIR_TUNNEL_TYPE_NVGRE,
 	RTE_FDIR_TUNNEL_TYPE_VXLAN,
+	RTE_FDIR_TUNNEL_TYPE_GRE,
 };
 
 /**
@@ -508,18 +509,20 @@  struct rte_eth_tunnel_flow {
 };
 
 /**
- * An union contains the inputs for all types of flow
+ * A struct contains the inputs for all types of 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_fdir_flow {
+	union {
+		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;
 };
@@ -540,7 +543,7 @@  struct rte_eth_fdir_flow_ext {
  */
 struct rte_eth_fdir_input {
 	uint16_t flow_type;
-	union rte_eth_fdir_flow flow;
+	struct rte_eth_fdir_flow flow;
 	/**< Flow fields to match, dependent on flow_type */
 	struct rte_eth_fdir_flow_ext flow_ext;
 	/**< Additional fields to match */