[ovs-dev,1/3] ofproto-dpif-sflow: propagate actions within clone

Message ID 1512470596-18091-2-git-send-email-zoltan.balogh@ericsson.com
State New
Headers show
Series
  • Fix tunnel neighbor cache population
Related show

Commit Message

Zoltan Balogh Dec. 5, 2017, 10:43 a.m.
By avoiding Tx recirculation and embracing tnl_push action within clone,
the tunnel metadata is not propagated. Unless clone action is handled in
the dpif_sflow_read_actions() function as well. This commit resolves
this issue.
In addition, some sflow data needs to be stored and restored in
ofproto-dpif-xlate when native_tunnel_output() is invoked. Otherwise the
output action of underlay bridge is getting counted as well if sFlow is
set on the overlay bridge.

Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
CC: Sugesh Chandran <sugesh.chandran@intel.com>
---
 ofproto/ofproto-dpif-sflow.c  | 19 +++++++++++++------
 ofproto/ofproto-dpif-sflow.h  |  2 +-
 ofproto/ofproto-dpif-upcall.c |  2 +-
 ofproto/ofproto-dpif-xlate.c  |  7 +++++++
 tests/ofproto-dpif.at         |  2 +-
 5 files changed, 23 insertions(+), 9 deletions(-)

Comments

Gregory Rose Dec. 12, 2017, 4:08 p.m. | #1
On 12/5/2017 2:43 AM, Zoltan Balogh wrote:
> By avoiding Tx recirculation and embracing tnl_push action within clone,
> the tunnel metadata is not propagated. Unless clone action is handled in
> the dpif_sflow_read_actions() function as well. This commit resolves
> this issue.
> In addition, some sflow data needs to be stored and restored in
> ofproto-dpif-xlate when native_tunnel_output() is invoked. Otherwise the
> output action of underlay bridge is getting counted as well if sFlow is
> set on the overlay bridge.
>
> Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
> CC: Sugesh Chandran <sugesh.chandran@intel.com>
> ---
>   ofproto/ofproto-dpif-sflow.c  | 19 +++++++++++++------
>   ofproto/ofproto-dpif-sflow.h  |  2 +-
>   ofproto/ofproto-dpif-upcall.c |  2 +-
>   ofproto/ofproto-dpif-xlate.c  |  7 +++++++
>   tests/ofproto-dpif.at         |  2 +-
>   5 files changed, 23 insertions(+), 9 deletions(-)

There are checkpatch errors on this patch.

== Checking 
"0001-ofproto-dpif-sflow-propagate-actions-within-clone.patch" ==
WARNING: Line has non-spaces leading whitespace
#38 FILE: ofproto/ofproto-dpif-sflow.c:1092:
                         struct dpif_sflow_actions *sflow_actions, bool 
capture_mpls)

WARNING: Line length is >79-characters long
WARNING: Line has non-spaces leading whitespace
#84 FILE: ofproto/ofproto-dpif-sflow.c:1347:
         && (num_outputs == 1 || num_outputs == 2)) { /* push_tnl or 
clone + push_tnl */

WARNING: Line has non-spaces leading whitespace
#93 FILE: ofproto/ofproto-dpif-sflow.c:1377:
         && num_outputs == 1) {

WARNING: Line has non-spaces leading whitespace
#106 FILE: ofproto/ofproto-dpif-sflow.h:74:
                              struct dpif_sflow_actions *, bool 
capture_mpls);

Lines checked: 164, Warnings: 5, Errors: 0

- Greg

> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> index ccf89645b..0bdb3fdfa 100644
> --- a/ofproto/ofproto-dpif-sflow.c
> +++ b/ofproto/ofproto-dpif-sflow.c
> @@ -961,7 +961,7 @@ sflow_read_set_action(const struct nlattr *attr,
>           } else {
>               dpif_sflow_read_actions(NULL,
>                                       nl_attr_get(attr), nl_attr_get_size(attr),
> -                                    sflow_actions);
> +                                    sflow_actions, true);
>           }
>           break;
>       case OVS_KEY_ATTR_PRIORITY:
> @@ -1089,7 +1089,7 @@ dpif_sflow_capture_input_mpls(const struct flow *flow,
>   void
>   dpif_sflow_read_actions(const struct flow *flow,
>   			const struct nlattr *actions, size_t actions_len,
> -			struct dpif_sflow_actions *sflow_actions)
> +			struct dpif_sflow_actions *sflow_actions, bool capture_mpls)
>   {
>       const struct nlattr *a;
>       unsigned int left;
> @@ -1099,7 +1099,7 @@ dpif_sflow_read_actions(const struct flow *flow,
>   	return;
>       }
>   
> -    if (flow != NULL) {
> +    if (flow != NULL && capture_mpls == true) {
>   	/* Make sure the MPLS output stack
>   	 * is seeded with the input stack.
>   	 */
> @@ -1197,8 +1197,13 @@ dpif_sflow_read_actions(const struct flow *flow,
>   	     * structure to report this.
>   	     */
>   	    break;
> +    case OVS_ACTION_ATTR_CLONE:
> +        if (flow != NULL) {
> +            dpif_sflow_read_actions(flow, nl_attr_get(a), nl_attr_get_size(a),
> +                                    sflow_actions, false);
> +        }
> +        break;
>   	case OVS_ACTION_ATTR_SAMPLE:
> -	case OVS_ACTION_ATTR_CLONE:
>           case OVS_ACTION_ATTR_ENCAP_NSH:
>           case OVS_ACTION_ATTR_DECAP_NSH:
>   	case OVS_ACTION_ATTR_UNSPEC:
> @@ -1266,6 +1271,7 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct dp_packet *packet,
>       struct dpif_sflow_port *in_dsp;
>       struct dpif_sflow_port *out_dsp;
>       ovs_be16 vlan_tci;
> +    uint32_t num_outputs;
>   
>       ovs_mutex_lock(&mutex);
>       sampler = ds->sflow_agent->samplers;
> @@ -1333,11 +1339,12 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct dp_packet *packet,
>   	}
>       }
>   
> +    num_outputs = dpif_sflow_cookie_num_outputs(cookie);
>       /* Output tunnel. */
>       if (sflow_actions
>   	&& sflow_actions->encap_depth == 1
>   	&& !sflow_actions->tunnel_err
> -	&& dpif_sflow_cookie_num_outputs(cookie) == 1) {
> +	&& (num_outputs == 1 || num_outputs == 2)) { /* push_tnl or clone + push_tnl */
>   	tnlOutProto = sflow_actions->tunnel_ipproto;
>   	if (tnlOutProto == 0) {
>   	    /* Try to infer the ip-protocol from the output port. */
> @@ -1367,7 +1374,7 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct dp_packet *packet,
>       if (sflow_actions
>   	&& sflow_actions->mpls_stack_depth > 0
>   	&& !sflow_actions->mpls_err
> -	&& dpif_sflow_cookie_num_outputs(cookie) == 1) {
> +	&& num_outputs == 1) {
>   	memset(&mplsElem, 0, sizeof(mplsElem));
>   	mplsElem.tag = SFLFLOW_EX_MPLS;
>   	dpif_sflow_encode_mpls_stack(&mplsElem.flowType.mpls.out_stack,
> diff --git a/ofproto/ofproto-dpif-sflow.h b/ofproto/ofproto-dpif-sflow.h
> index 014e6cce3..01dd7d590 100644
> --- a/ofproto/ofproto-dpif-sflow.h
> +++ b/ofproto/ofproto-dpif-sflow.h
> @@ -71,7 +71,7 @@ void dpif_sflow_wait(struct dpif_sflow *);
>   
>   void dpif_sflow_read_actions(const struct flow *,
>   			     const struct nlattr *actions, size_t actions_len,
> -			     struct dpif_sflow_actions *);
> +			     struct dpif_sflow_actions *, bool capture_mpls);
>   
>   void dpif_sflow_received(struct dpif_sflow *, const struct dp_packet *,
>                            const struct flow *, odp_port_t odp_port,
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 02cf5415b..dc496344f 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1317,7 +1317,7 @@ dpif_read_actions(struct udpif *udpif, struct upcall *upcall,
>   
>       switch (type) {
>       case SFLOW_UPCALL:
> -        dpif_sflow_read_actions(flow, actions, actions_len, upcall_data);
> +        dpif_sflow_read_actions(flow, actions, actions_len, upcall_data, true);
>           break;
>       case FLOW_SAMPLE_UPCALL:
>       case IPFIX_UPCALL:
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index fcced344e..19343aca3 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3259,6 +3259,9 @@ native_tunnel_output(struct xlate_ctx *ctx, const struct xport *xport,
>       char buf_sip6[INET6_ADDRSTRLEN];
>       char buf_dip6[INET6_ADDRSTRLEN];
>   
> +    /* Store sFlow data. */
> +    uint32_t sflow_n_outputs = ctx->sflow_n_outputs;
> +
>       /* Structures to backup Ethernet and IP of base_flow. */
>       struct flow old_base_flow;
>       struct flow old_flow;
> @@ -3410,6 +3413,10 @@ native_tunnel_output(struct xlate_ctx *ctx, const struct xport *xport,
>       /* Restore the flows after the translation. */
>       memcpy(&ctx->xin->flow, &old_flow, sizeof ctx->xin->flow);
>       memcpy(&ctx->base_flow, &old_base_flow, sizeof ctx->base_flow);
> +
> +    /* Restore sFlow data. */
> +    ctx->sflow_n_outputs = sflow_n_outputs;
> +
>       return 0;
>   }
>   
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 9a51a1364..cdf48f4de 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6286,7 +6286,7 @@ AT_CHECK([ovs-appctl ovs/route/add 1.1.2.92/24 br0], [0], [OK
>   AT_CHECK([ovs-ofctl add-flow br0 action=normal])
>   
>   dnl Prime ARP Cache for 1.1.2.92
> -AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)'])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)'])
>   
>   dnl configure sflow on int-br only
>   ovs-vsctl \

Patch

diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index ccf89645b..0bdb3fdfa 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -961,7 +961,7 @@  sflow_read_set_action(const struct nlattr *attr,
         } else {
             dpif_sflow_read_actions(NULL,
                                     nl_attr_get(attr), nl_attr_get_size(attr),
-                                    sflow_actions);
+                                    sflow_actions, true);
         }
         break;
     case OVS_KEY_ATTR_PRIORITY:
@@ -1089,7 +1089,7 @@  dpif_sflow_capture_input_mpls(const struct flow *flow,
 void
 dpif_sflow_read_actions(const struct flow *flow,
 			const struct nlattr *actions, size_t actions_len,
-			struct dpif_sflow_actions *sflow_actions)
+			struct dpif_sflow_actions *sflow_actions, bool capture_mpls)
 {
     const struct nlattr *a;
     unsigned int left;
@@ -1099,7 +1099,7 @@  dpif_sflow_read_actions(const struct flow *flow,
 	return;
     }
 
-    if (flow != NULL) {
+    if (flow != NULL && capture_mpls == true) {
 	/* Make sure the MPLS output stack
 	 * is seeded with the input stack.
 	 */
@@ -1197,8 +1197,13 @@  dpif_sflow_read_actions(const struct flow *flow,
 	     * structure to report this.
 	     */
 	    break;
+    case OVS_ACTION_ATTR_CLONE:
+        if (flow != NULL) {
+            dpif_sflow_read_actions(flow, nl_attr_get(a), nl_attr_get_size(a),
+                                    sflow_actions, false);
+        }
+        break;
 	case OVS_ACTION_ATTR_SAMPLE:
-	case OVS_ACTION_ATTR_CLONE:
         case OVS_ACTION_ATTR_ENCAP_NSH:
         case OVS_ACTION_ATTR_DECAP_NSH:
 	case OVS_ACTION_ATTR_UNSPEC:
@@ -1266,6 +1271,7 @@  dpif_sflow_received(struct dpif_sflow *ds, const struct dp_packet *packet,
     struct dpif_sflow_port *in_dsp;
     struct dpif_sflow_port *out_dsp;
     ovs_be16 vlan_tci;
+    uint32_t num_outputs;
 
     ovs_mutex_lock(&mutex);
     sampler = ds->sflow_agent->samplers;
@@ -1333,11 +1339,12 @@  dpif_sflow_received(struct dpif_sflow *ds, const struct dp_packet *packet,
 	}
     }
 
+    num_outputs = dpif_sflow_cookie_num_outputs(cookie);
     /* Output tunnel. */
     if (sflow_actions
 	&& sflow_actions->encap_depth == 1
 	&& !sflow_actions->tunnel_err
-	&& dpif_sflow_cookie_num_outputs(cookie) == 1) {
+	&& (num_outputs == 1 || num_outputs == 2)) { /* push_tnl or clone + push_tnl */
 	tnlOutProto = sflow_actions->tunnel_ipproto;
 	if (tnlOutProto == 0) {
 	    /* Try to infer the ip-protocol from the output port. */
@@ -1367,7 +1374,7 @@  dpif_sflow_received(struct dpif_sflow *ds, const struct dp_packet *packet,
     if (sflow_actions
 	&& sflow_actions->mpls_stack_depth > 0
 	&& !sflow_actions->mpls_err
-	&& dpif_sflow_cookie_num_outputs(cookie) == 1) {
+	&& num_outputs == 1) {
 	memset(&mplsElem, 0, sizeof(mplsElem));
 	mplsElem.tag = SFLFLOW_EX_MPLS;
 	dpif_sflow_encode_mpls_stack(&mplsElem.flowType.mpls.out_stack,
diff --git a/ofproto/ofproto-dpif-sflow.h b/ofproto/ofproto-dpif-sflow.h
index 014e6cce3..01dd7d590 100644
--- a/ofproto/ofproto-dpif-sflow.h
+++ b/ofproto/ofproto-dpif-sflow.h
@@ -71,7 +71,7 @@  void dpif_sflow_wait(struct dpif_sflow *);
 
 void dpif_sflow_read_actions(const struct flow *,
 			     const struct nlattr *actions, size_t actions_len,
-			     struct dpif_sflow_actions *);
+			     struct dpif_sflow_actions *, bool capture_mpls);
 
 void dpif_sflow_received(struct dpif_sflow *, const struct dp_packet *,
                          const struct flow *, odp_port_t odp_port,
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 02cf5415b..dc496344f 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1317,7 +1317,7 @@  dpif_read_actions(struct udpif *udpif, struct upcall *upcall,
 
     switch (type) {
     case SFLOW_UPCALL:
-        dpif_sflow_read_actions(flow, actions, actions_len, upcall_data);
+        dpif_sflow_read_actions(flow, actions, actions_len, upcall_data, true);
         break;
     case FLOW_SAMPLE_UPCALL:
     case IPFIX_UPCALL:
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index fcced344e..19343aca3 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3259,6 +3259,9 @@  native_tunnel_output(struct xlate_ctx *ctx, const struct xport *xport,
     char buf_sip6[INET6_ADDRSTRLEN];
     char buf_dip6[INET6_ADDRSTRLEN];
 
+    /* Store sFlow data. */
+    uint32_t sflow_n_outputs = ctx->sflow_n_outputs;
+
     /* Structures to backup Ethernet and IP of base_flow. */
     struct flow old_base_flow;
     struct flow old_flow;
@@ -3410,6 +3413,10 @@  native_tunnel_output(struct xlate_ctx *ctx, const struct xport *xport,
     /* Restore the flows after the translation. */
     memcpy(&ctx->xin->flow, &old_flow, sizeof ctx->xin->flow);
     memcpy(&ctx->base_flow, &old_base_flow, sizeof ctx->base_flow);
+
+    /* Restore sFlow data. */
+    ctx->sflow_n_outputs = sflow_n_outputs;
+
     return 0;
 }
 
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 9a51a1364..cdf48f4de 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6286,7 +6286,7 @@  AT_CHECK([ovs-appctl ovs/route/add 1.1.2.92/24 br0], [0], [OK
 AT_CHECK([ovs-ofctl add-flow br0 action=normal])
 
 dnl Prime ARP Cache for 1.1.2.92
-AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)'])
 
 dnl configure sflow on int-br only
 ovs-vsctl \