On 12/21/2017 2:25 PM, Justin Pettit wrote:
     - This reduces the number of times upcall cookies are processed.
     - It separate true miss calls from slow-path actions.

The reorganization will also be useful for a future commit.

Signed-off-by: Justin Pettit <jpet...@ovn.org>

Justin,

The problem I'm seeing arises when this patch is applied.  I'm puzzled though because if
I create a script to do the exact same thing as the test I see no problems.

Here is my script:

ovs-ofctl add-flow br0 "actions=normal"
ip netns add at_ns0
ip netns add at_ns1
ip link add p0 type veth peer name ovs-p0
ip link set p0 netns at_ns0
ip link set dev ovs-p0 up
ovs-vsctl add-port br0 ovs-p0 -- set interface ovs-p0 external-ids:iface-id="p0"
ip netns exec at_ns0 ip addr add 10.1.1.1/24 dev p0
ip netns exec at_ns0 ip link set dev p0 up
ip link add p1 type veth peer name ovs-p1
ip link set p1 netns at_ns1
ip link set dev ovs-p1 up
ovs-vsctl add-port br0 ovs-p1 -- set interface ovs-p1 external-ids:iface-id="p1"
ip netns exec at_ns1 ip addr add 10.1.1.2/24 dev p1
ip netns exec at_ns1 ip link set dev p1 up
ip netns exec at_ns0 ping -q -c 3 -i 0.3 -w 2 10.1.1.2
ip netns exec at_ns0 ping -s 1600 -q -c 3 -i 0.3 -w 2 10.1.1.2
ip netns exec at_ns0 ping -s 3200 -q -c 3 -i 0.3 -w 2 10.1.1.2
ovs-vsctl del-port br0 ovs-p1
ovs-vsctl del-port br0 ovs-p0
ip netns del at_ns0
ip netns del at_ns1
ovs-ofctl del-flows br0

SFAICT it emulates exactly what the system-traffic.at test 001 does.  And it works fine... /shrug.

What distribution, kernel, etc are you using for your check-kmod testing?  I'll try to emulate that
exactly and then see if I can get similar results.

Thanks,

- Greg


---
  ofproto/ofproto-dpif-upcall.c | 91 +++++++++++++++++++++----------------------
  1 file changed, 45 insertions(+), 46 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 02cf5415bc3d..46b75fe35a2b 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -183,6 +183,7 @@ struct udpif {
  enum upcall_type {
      BAD_UPCALL,                 /* Some kind of bug somewhere. */
      MISS_UPCALL,                /* A flow miss.  */
+    SLOW_PATH_UPCALL,           /* Slow path upcall.  */
      SFLOW_UPCALL,               /* sFlow sample. */
      FLOW_SAMPLE_UPCALL,         /* Per-flow sampling. */
      IPFIX_UPCALL                /* Per-bridge sampling. */
@@ -210,8 +211,7 @@ struct upcall {
      uint16_t mru;                  /* If !0, Maximum receive unit of
                                        fragmented IP packet */
- enum dpif_upcall_type type; /* Datapath type of the upcall. */
-    const struct nlattr *userdata; /* Userdata for DPIF_UC_ACTION Upcalls. */
+    enum upcall_type type;         /* Type of the upcall. */
      const struct nlattr *actions;  /* Flow actions in DPIF_UC_ACTION Upcalls. 
*/
bool xout_initialized; /* True if 'xout' must be uninitialized. */
@@ -235,6 +235,8 @@ struct upcall {
      size_t key_len;                /* Datapath flow key length. */
      const struct nlattr *out_tun_key;  /* Datapath output tunnel key. */
+ union user_action_cookie cookie;
+
      uint64_t odp_actions_stub[1024 / 8]; /* Stub for odp_actions. */
  };
@@ -367,7 +369,8 @@ static int ukey_acquire(struct udpif *, const struct dpif_flow *,
  static void ukey_delete__(struct udpif_key *);
  static void ukey_delete(struct umap *, struct udpif_key *);
  static enum upcall_type classify_upcall(enum dpif_upcall_type type,
-                                        const struct nlattr *userdata);
+                                        const struct nlattr *userdata,
+                                        union user_action_cookie *cookie);
static void put_op_init(struct ukey_op *op, struct udpif_key *ukey,
                          enum dpif_flow_put_flags flags);
@@ -813,7 +816,6 @@ recv_upcalls(struct handler *handler)
upcall->key = dupcall->key;
          upcall->key_len = dupcall->key_len;
-        upcall->ufid = &dupcall->ufid;
upcall->out_tun_key = dupcall->out_tun_key;
          upcall->actions = dupcall->actions;
@@ -969,11 +971,13 @@ udpif_revalidator(void *arg)
  }
  
  static enum upcall_type
-classify_upcall(enum dpif_upcall_type type, const struct nlattr *userdata)
+classify_upcall(enum dpif_upcall_type type, const struct nlattr *userdata,
+                union user_action_cookie *cookie)
  {
-    union user_action_cookie cookie;
      size_t userdata_len;
+ memset(cookie, 0, sizeof *cookie);
+
      /* First look at the upcall type. */
      switch (type) {
      case DPIF_UC_ACTION:
@@ -994,29 +998,30 @@ classify_upcall(enum dpif_upcall_type type, const struct 
nlattr *userdata)
          return BAD_UPCALL;
      }
      userdata_len = nl_attr_get_size(userdata);
-    if (userdata_len < sizeof cookie.type
-        || userdata_len > sizeof cookie) {
+    if (userdata_len < sizeof cookie->type
+        || userdata_len > sizeof *cookie) {
          VLOG_WARN_RL(&rl, "action upcall cookie has unexpected size 
%"PRIuSIZE,
                       userdata_len);
          return BAD_UPCALL;
      }
-    memset(&cookie, 0, sizeof cookie);
-    memcpy(&cookie, nl_attr_get(userdata), userdata_len);
-    if (userdata_len == MAX(8, sizeof cookie.sflow)
-        && cookie.type == USER_ACTION_COOKIE_SFLOW) {
+
+    memcpy(cookie, nl_attr_get(userdata), userdata_len);
+
+    if (userdata_len == MAX(8, sizeof cookie->sflow)
+        && cookie->type == USER_ACTION_COOKIE_SFLOW) {
          return SFLOW_UPCALL;
-    } else if (userdata_len == MAX(8, sizeof cookie.slow_path)
-               && cookie.type == USER_ACTION_COOKIE_SLOW_PATH) {
-        return MISS_UPCALL;
-    } else if (userdata_len == MAX(8, sizeof cookie.flow_sample)
-               && cookie.type == USER_ACTION_COOKIE_FLOW_SAMPLE) {
+    } else if (userdata_len == MAX(8, sizeof cookie->slow_path)
+               && cookie->type == USER_ACTION_COOKIE_SLOW_PATH) {
+        return SLOW_PATH_UPCALL;
+    } else if (userdata_len == MAX(8, sizeof cookie->flow_sample)
+               && cookie->type == USER_ACTION_COOKIE_FLOW_SAMPLE) {
          return FLOW_SAMPLE_UPCALL;
-    } else if (userdata_len == MAX(8, sizeof cookie.ipfix)
-               && cookie.type == USER_ACTION_COOKIE_IPFIX) {
+    } else if (userdata_len == MAX(8, sizeof cookie->ipfix)
+               && cookie->type == USER_ACTION_COOKIE_IPFIX) {
          return IPFIX_UPCALL;
      } else {
          VLOG_WARN_RL(&rl, "invalid user cookie of type %"PRIu16
-                     " and size %"PRIuSIZE, cookie.type, userdata_len);
+                     " and size %"PRIuSIZE, cookie->type, userdata_len);
          return BAD_UPCALL;
      }
  }
@@ -1078,6 +1083,11 @@ upcall_receive(struct upcall *upcall, const struct 
dpif_backer *backer,
  {
      int error;
+ upcall->type = classify_upcall(type, userdata, &upcall->cookie);
+    if (upcall->type == BAD_UPCALL) {
+        return EAGAIN;
+    }
+
      error = xlate_lookup(backer, flow, &upcall->ofproto, &upcall->ipfix,
                           &upcall->sflow, NULL, &upcall->in_port);
      if (error) {
@@ -1090,8 +1100,6 @@ upcall_receive(struct upcall *upcall, const struct 
dpif_backer *backer,
      upcall->packet = packet;
      upcall->ufid = ufid;
      upcall->pmd_id = pmd_id;
-    upcall->type = type;
-    upcall->userdata = userdata;
      ofpbuf_use_stub(&upcall->odp_actions, upcall->odp_actions_stub,
                      sizeof upcall->odp_actions_stub);
      ofpbuf_init(&upcall->put_actions, 0);
@@ -1127,7 +1135,7 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
                    upcall->flow, upcall->in_port, NULL,
                    stats.tcp_flags, upcall->packet, wc, odp_actions);
- if (upcall->type == DPIF_UC_MISS) {
+    if (upcall->type == MISS_UPCALL) {
          xin.resubmit_stats = &stats;
if (xin.frozen_state) {
@@ -1176,7 +1184,7 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
      /* This function is also called for slow-pathed flows.  As we are only
       * going to create new datapath flows for actual datapath misses, there is
       * no point in creating a ukey otherwise. */
-    if (upcall->type == DPIF_UC_MISS) {
+    if (upcall->type == MISS_UPCALL) {
          upcall->ukey = ukey_create_from_upcall(upcall, wc);
      }
  }
@@ -1212,7 +1220,7 @@ should_install_flow(struct udpif *udpif, struct upcall 
*upcall)
  {
      unsigned int flow_limit;
- if (upcall->type != DPIF_UC_MISS) {
+    if (upcall->type != MISS_UPCALL) {
          return false;
      } else if (upcall->recirc && !upcall->have_recirc_ref) {
          VLOG_DBG_RL(&rl, "upcall: no reference for recirc flow");
@@ -1325,6 +1333,7 @@ dpif_read_actions(struct udpif *udpif, struct upcall 
*upcall,
          break;
      case BAD_UPCALL:
      case MISS_UPCALL:
+    case SLOW_PATH_UPCALL:
      default:
          break;
      }
@@ -1336,53 +1345,46 @@ static int
  process_upcall(struct udpif *udpif, struct upcall *upcall,
                 struct ofpbuf *odp_actions, struct flow_wildcards *wc)
  {
-    const struct nlattr *userdata = upcall->userdata;
      const struct dp_packet *packet = upcall->packet;
      const struct flow *flow = upcall->flow;
      size_t actions_len = 0;
-    enum upcall_type upcall_type = classify_upcall(upcall->type, userdata);
- switch (upcall_type) {
+    switch (upcall->type) {
      case MISS_UPCALL:
+    case SLOW_PATH_UPCALL:
          upcall_xlate(udpif, upcall, odp_actions, wc);
          return 0;
case SFLOW_UPCALL:
          if (upcall->sflow) {
-            union user_action_cookie cookie;
              struct dpif_sflow_actions sflow_actions;
memset(&sflow_actions, 0, sizeof sflow_actions);
-            memset(&cookie, 0, sizeof cookie);
-            memcpy(&cookie, nl_attr_get(userdata), sizeof cookie.sflow);
- actions_len = dpif_read_actions(udpif, upcall, flow, upcall_type,
-                                            &sflow_actions);
+            actions_len = dpif_read_actions(udpif, upcall, flow,
+                                            upcall->type, &sflow_actions);
              dpif_sflow_received(upcall->sflow, packet, flow,
-                                flow->in_port.odp_port, &cookie,
+                                flow->in_port.odp_port, &upcall->cookie,
                                  actions_len > 0 ? &sflow_actions : NULL);
          }
          break;
case IPFIX_UPCALL:
          if (upcall->ipfix) {
-            union user_action_cookie cookie;
              struct flow_tnl output_tunnel_key;
              struct dpif_ipfix_actions ipfix_actions;
- memset(&cookie, 0, sizeof cookie);
-            memcpy(&cookie, nl_attr_get(userdata), sizeof cookie.ipfix);
              memset(&ipfix_actions, 0, sizeof ipfix_actions);
if (upcall->out_tun_key) {
                  odp_tun_key_from_attr(upcall->out_tun_key, 
&output_tunnel_key);
              }
- actions_len = dpif_read_actions(udpif, upcall, flow, upcall_type,
-                                            &ipfix_actions);
+            actions_len = dpif_read_actions(udpif, upcall, flow,
+                                            upcall->type, &ipfix_actions);
              dpif_ipfix_bridge_sample(upcall->ipfix, packet, flow,
                                       flow->in_port.odp_port,
-                                     cookie.ipfix.output_odp_port,
+                                     upcall->cookie.ipfix.output_odp_port,
                                       upcall->out_tun_key ?
                                           &output_tunnel_key : NULL,
                                       actions_len > 0 ? &ipfix_actions: NULL);
@@ -1391,24 +1393,21 @@ process_upcall(struct udpif *udpif, struct upcall 
*upcall,
case FLOW_SAMPLE_UPCALL:
          if (upcall->ipfix) {
-            union user_action_cookie cookie;
              struct flow_tnl output_tunnel_key;
              struct dpif_ipfix_actions ipfix_actions;
- memset(&cookie, 0, sizeof cookie);
-            memcpy(&cookie, nl_attr_get(userdata), sizeof cookie.flow_sample);
              memset(&ipfix_actions, 0, sizeof ipfix_actions);
if (upcall->out_tun_key) {
                  odp_tun_key_from_attr(upcall->out_tun_key, 
&output_tunnel_key);
              }
- actions_len = dpif_read_actions(udpif, upcall, flow, upcall_type,
-                                            &ipfix_actions);
+            actions_len = dpif_read_actions(udpif, upcall, flow,
+                                            upcall->type, &ipfix_actions);
              /* The flow reflects exactly the contents of the packet.
               * Sample the packet using it. */
              dpif_ipfix_flow_sample(upcall->ipfix, packet, flow,
-                                   &cookie, flow->in_port.odp_port,
+                                   &upcall->cookie, flow->in_port.odp_port,
                                     upcall->out_tun_key ?
                                         &output_tunnel_key : NULL,
                                     actions_len > 0 ? &ipfix_actions: NULL);

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to