[ovs-dev] [PATCH] Simplify kernel sFlow implementation

pravin shelar pshelar at nicira.com
Tue Aug 9 17:46:38 UTC 2011


 This patch is first step toward generic/simple sampling
 action which will be completely independent of sflow.
 Following patch cleanup sampling upcall by striping it down to
 minimum essential data that userspace can not figure out for
 itself.

Signed-off by: Pravin Shelar <pshelar at nicira.com>

---
 datapath/actions.c                      |   27 +++++++--------------------
 datapath/datapath.c                     |   17 -----------------
 datapath/datapath.h                     |    3 ---
 include/openvswitch/datapath-protocol.h |    1 -
 lib/dpif-linux.c                        |   12 ------------
 lib/dpif.h                              |    5 -----
 ofproto/ofproto-dpif-sflow.c            |   16 +++++++++++++---
 ofproto/ofproto-dpif-sflow.h            |    3 ++-
 ofproto/ofproto-dpif.c                  |   23 +++++++++++++++++++----
 ofproto/ofproto.h                       |    3 +++
 10 files changed, 44 insertions(+), 66 deletions(-)

diff --git a/datapath/actions.c b/datapath/actions.c
index 78712c6..1bf6c40 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -218,7 +218,8 @@ error:
 	kfree_skb(skb);
 }
 
-static int output_userspace(struct datapath *dp, struct sk_buff *skb, u64 arg)
+static int output_userspace(struct datapath *dp, struct sk_buff *skb, u8 cmd,
+			    u64 arg)
 {
 	struct dp_upcall_info upcall;
 
@@ -226,12 +227,9 @@ static int output_userspace(struct datapath *dp, struct sk_buff *skb, u64 arg)
 	if (!skb)
 		return -ENOMEM;
 
-	upcall.cmd = ODP_PACKET_CMD_ACTION;
+	upcall.cmd = cmd;
 	upcall.key = &OVS_CB(skb)->flow->key;
 	upcall.userdata = arg;
-	upcall.sample_pool = 0;
-	upcall.actions = NULL;
-	upcall.actions_len = 0;
 	return dp_upcall(dp, skb, &upcall);
 }
 
@@ -263,7 +261,8 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			break;
 
 		case ODP_ACTION_ATTR_USERSPACE:
-			err = output_userspace(dp, skb, nla_get_u64(a));
+			err = output_userspace(dp, skb, ODP_PACKET_CMD_ACTION,
+					       nla_get_u64(a));
 			break;
 
 		case ODP_ACTION_ATTR_SET_TUNNEL:
@@ -330,9 +329,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 static void sflow_sample(struct datapath *dp, struct sk_buff *skb,
 			 struct sw_flow_actions *acts)
 {
-	struct sk_buff *nskb;
 	struct vport *p = OVS_CB(skb)->vport;
-	struct dp_upcall_info upcall;
 
 	if (unlikely(!p))
 		return;
@@ -340,18 +337,8 @@ static void sflow_sample(struct datapath *dp, struct sk_buff *skb,
 	atomic_inc(&p->sflow_pool);
 	if (net_random() >= dp->sflow_probability)
 		return;
-
-	nskb = skb_clone(skb, GFP_ATOMIC);
-	if (unlikely(!nskb))
-		return;
-
-	upcall.cmd = ODP_PACKET_CMD_SAMPLE;
-	upcall.key = &OVS_CB(skb)->flow->key;
-	upcall.userdata = 0;
-	upcall.sample_pool = atomic_read(&p->sflow_pool);
-	upcall.actions = acts->actions;
-	upcall.actions_len = acts->actions_len;
-	dp_upcall(dp, nskb, &upcall);
+	output_userspace(dp, skb, ODP_PACKET_CMD_SAMPLE,
+			 atomic_read(&p->sflow_pool));
 }
 
 /* Execute a list of actions against 'skb'. */
diff --git a/datapath/datapath.c b/datapath/datapath.c
index b191239..73b0724 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -303,9 +303,6 @@ void dp_process_received_packet(struct vport *p, struct sk_buff *skb)
 			upcall.cmd = ODP_PACKET_CMD_MISS;
 			upcall.key = &key;
 			upcall.userdata = 0;
-			upcall.sample_pool = 0;
-			upcall.actions = NULL;
-			upcall.actions_len = 0;
 			dp_upcall(dp, skb, &upcall);
 			stats_counter_off = offsetof(struct dp_stats_percpu, n_missed);
 			goto out;
@@ -466,10 +463,6 @@ static int queue_userspace_packets(struct datapath *dp, struct sk_buff *skb,
 		len += nla_total_size(FLOW_BUFSIZE);
 		if (upcall_info->userdata)
 			len += nla_total_size(8);
-		if (upcall_info->sample_pool)
-			len += nla_total_size(4);
-		if (upcall_info->actions_len)
-			len += nla_total_size(upcall_info->actions_len);
 
 		user_skb = genlmsg_new(len, GFP_ATOMIC);
 		if (!user_skb) {
@@ -486,16 +479,6 @@ static int queue_userspace_packets(struct datapath *dp, struct sk_buff *skb,
 
 		if (upcall_info->userdata)
 			nla_put_u64(user_skb, ODP_PACKET_ATTR_USERDATA, upcall_info->userdata);
-		if (upcall_info->sample_pool)
-			nla_put_u32(user_skb, ODP_PACKET_ATTR_SAMPLE_POOL, upcall_info->sample_pool);
-		if (upcall_info->actions_len) {
-			const struct nlattr *actions = upcall_info->actions;
-			u32 actions_len = upcall_info->actions_len;
-
-			nla = nla_nest_start(user_skb, ODP_PACKET_ATTR_ACTIONS);
-			memcpy(__skb_put(user_skb, actions_len), actions, actions_len);
-			nla_nest_end(user_skb, nla);
-		}
 
 		nla = __nla_reserve(user_skb, ODP_PACKET_ATTR_PACKET, skb->len);
 		if (skb->ip_summed == CHECKSUM_PARTIAL)
diff --git a/datapath/datapath.h b/datapath/datapath.h
index 15a9898..9f36112 100644
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -138,9 +138,6 @@ struct dp_upcall_info {
 	u8 cmd;
 	const struct sw_flow_key *key;
 	u64 userdata;
-	u32 sample_pool;
-	const struct nlattr *actions;
-	u32 actions_len;
 };
 
 extern struct notifier_block dp_device_notifier;
diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
index 426236c..6534c64 100644
--- a/include/openvswitch/datapath-protocol.h
+++ b/include/openvswitch/datapath-protocol.h
@@ -181,7 +181,6 @@ enum odp_packet_attr {
 	ODP_PACKET_ATTR_PACKET,      /* Packet data. */
 	ODP_PACKET_ATTR_KEY,         /* Nested ODP_KEY_ATTR_* attributes. */
 	ODP_PACKET_ATTR_USERDATA,    /* u64 ODP_ACTION_ATTR_USERSPACE arg. */
-	ODP_PACKET_ATTR_SAMPLE_POOL, /* # sampling candidate packets so far. */
 	ODP_PACKET_ATTR_ACTIONS,     /* Nested ODP_ACTION_ATTR_* attributes. */
 	__ODP_PACKET_ATTR_MAX
 };
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 0f6a140..9d8b017 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -931,10 +931,6 @@ parse_odp_packet(struct ofpbuf *buf, struct dpif_upcall *upcall,
 
         /* ODP_PACKET_CMD_ACTION only. */
         [ODP_PACKET_ATTR_USERDATA] = { .type = NL_A_U64, .optional = true },
-
-        /* ODP_PACKET_CMD_SAMPLE only. */
-        [ODP_PACKET_ATTR_SAMPLE_POOL] = { .type = NL_A_U32, .optional = true },
-        [ODP_PACKET_ATTR_ACTIONS] = { .type = NL_A_NESTED, .optional = true },
     };
 
     struct odp_header *odp_header;
@@ -974,14 +970,6 @@ parse_odp_packet(struct ofpbuf *buf, struct dpif_upcall *upcall,
     upcall->userdata = (a[ODP_PACKET_ATTR_USERDATA]
                         ? nl_attr_get_u64(a[ODP_PACKET_ATTR_USERDATA])
                         : 0);
-    upcall->sample_pool = (a[ODP_PACKET_ATTR_SAMPLE_POOL]
-                        ? nl_attr_get_u32(a[ODP_PACKET_ATTR_SAMPLE_POOL])
-                           : 0);
-    if (a[ODP_PACKET_ATTR_ACTIONS]) {
-        upcall->actions = (void *) nl_attr_get(a[ODP_PACKET_ATTR_ACTIONS]);
-        upcall->actions_len = nl_attr_get_size(a[ODP_PACKET_ATTR_ACTIONS]);
-    }
-
     *dp_ifindex = odp_header->dp_ifindex;
 
     return 0;
diff --git a/lib/dpif.h b/lib/dpif.h
index 4df2318..eaa329e 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -178,11 +178,6 @@ struct dpif_upcall {
 
     /* DPIF_UC_ACTION only. */
     uint64_t userdata;          /* Argument to ODP_ACTION_ATTR_USERSPACE. */
-
-    /* DPIF_UC_SAMPLE only. */
-    uint32_t sample_pool;       /* # of sampling candidate packets so far. */
-    struct nlattr *actions;     /* Associated flow actions. */
-    size_t actions_len;
 };
 
 int dpif_recv_get_mask(const struct dpif *, int *listen_mask);
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index 303623c..ae76fd0 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -279,11 +279,12 @@ dpif_sflow_is_enabled(const struct dpif_sflow *ds)
 }
 
 struct dpif_sflow *
-dpif_sflow_create(struct dpif *dpif)
+dpif_sflow_create(struct ofproto *ofproto, struct dpif *dpif)
 {
     struct dpif_sflow *ds;
 
     ds = xcalloc(1, sizeof *ds);
+    ds->ofproto = ofproto;
     ds->dpif = dpif;
     ds->next_tick = time_now() + 1;
     hmap_init(&ds->ports);
@@ -486,12 +487,14 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct dpif_upcall *upcall,
     unsigned int left;
     struct nlattr *a;
     size_t n_outputs;
+    struct nlattr *actions;
+    size_t actions_len;
 
     /* Build a flow sample */
     memset(&fs, 0, sizeof fs);
     fs.input = dpif_sflow_odp_port_to_ifindex(ds, flow->in_port);
     fs.output = 0;              /* Filled in correctly below. */
-    fs.sample_pool = upcall->sample_pool;
+    fs.sample_pool = (uint32_t) upcall->userdata;
 
     /* We are going to give it to the sampler that represents this input port.
      * By implementing "ingress-only" sampling like this we ensure that we
@@ -528,8 +531,14 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct dpif_upcall *upcall,
     switchElem.flowType.sw.dst_priority = switchElem.flowType.sw.src_priority;
 
     /* Figure out the output ports. */
+    actions = ofproto_get_actions(ds->ofproto, flow, &actions_len);
+    if (!actions) {
+        VLOG_DBG_RL(&rl, "could not lookup action for sample packet");
+        goto next;
+    }
+
     n_outputs = 0;
-    NL_ATTR_FOR_EACH_UNSAFE (a, left, upcall->actions, upcall->actions_len) {
+    NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) {
         ovs_be16 tci;
 
         switch (nl_attr_type(a)) {
@@ -549,6 +558,7 @@ dpif_sflow_received(struct dpif_sflow *ds, const struct dpif_upcall *upcall,
         }
     }
 
+next:
     /* Set output port, as defined by http://www.sflow.org/sflow_version_5.txt
        (search for "Input/output port information"). */
     if (!n_outputs) {
diff --git a/ofproto/ofproto-dpif-sflow.h b/ofproto/ofproto-dpif-sflow.h
index acafa32..ece3f2a 100644
--- a/ofproto/ofproto-dpif-sflow.h
+++ b/ofproto/ofproto-dpif-sflow.h
@@ -25,8 +25,9 @@ struct dpif;
 struct dpif_upcall;
 struct flow;
 struct ofproto_sflow_options;
+struct ofproto;
 
-struct dpif_sflow *dpif_sflow_create(struct dpif *);
+struct dpif_sflow *dpif_sflow_create(struct ofproto *ofproto, struct dpif *dpif);
 void dpif_sflow_destroy(struct dpif_sflow *);
 void dpif_sflow_set_options(struct dpif_sflow *,
                             const struct ofproto_sflow_options *);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index f53cc43..8e029a8 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -772,7 +772,7 @@ set_sflow(struct ofproto *ofproto_,
         if (!ds) {
             struct ofport_dpif *ofport;
 
-            ds = ofproto->sflow = dpif_sflow_create(ofproto->dpif);
+            ds = ofproto->sflow = dpif_sflow_create(ofproto_, ofproto->dpif);
             HMAP_FOR_EACH (ofport, up.hmap_node, &ofproto->up.ports) {
                 dpif_sflow_add_port(ds, ofport->odp_port,
                                     netdev_get_name(ofport->up.netdev));
@@ -2074,9 +2074,6 @@ execute_odp_actions(struct ofproto_dpif *ofproto, const struct flow *flow,
         upcall.key = NULL;
         upcall.key_len = 0;
         upcall.userdata = nl_attr_get_u64(odp_actions);
-        upcall.sample_pool = 0;
-        upcall.actions = NULL;
-        upcall.actions_len = 0;
 
         send_packet_in(ofproto, &upcall, flow, false);
 
@@ -2559,6 +2556,24 @@ facet_push_stats(struct facet *facet)
     }
 }
 
+/*
+ * Searches ofproto's table of facets for exact flow to get actions.
+ * actions returned might not be valid action as facet is not revalidated.
+ * returns action for given flow */
+struct nlattr *
+ofproto_get_actions(struct ofproto *ofproto_, const struct flow *flow, int *len)
+{
+    struct facet *facet;
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
+
+    facet = facet_find(ofproto, flow);
+    if (facet) {
+        *len = facet->actions_len;
+        return facet->actions;
+    }
+    return NULL;
+}
+
 struct ofproto_push {
     struct action_xlate_ctx ctx;
     uint64_t packets;
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index e0c99ea..0422635 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -239,6 +239,9 @@ int ofproto_port_get_cfm_fault(const struct ofproto *, uint16_t ofp_port);
 void ofproto_get_ofproto_controller_info(const struct ofproto *, struct shash *);
 void ofproto_free_ofproto_controller_info(struct shash *);
 
+struct nlattr *
+ofproto_get_actions(struct ofproto *ofproto, const struct flow *flow, int *len);
+
 #ifdef  __cplusplus
 }
 #endif
-- 
1.7.6.134.gcf13f




More information about the dev mailing list