ofproto-dpif: Unset DPIF_FP_MODIFY flag when creating a new flo

[cascardo/ovs.git] / ofproto / ofproto-dpif.c

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 93b0579..aa087b2 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3771,11 +3771,7 @@ handle_flow_miss_with_facet(struct flow_miss *miss, struct facet *facet,
     struct subfacet *subfacet;
     struct ofpbuf *packet;
 
-    subfacet = subfacet_create(facet, miss, now);
     want_path = facet->xout.slow ? SF_SLOW_PATH : SF_FAST_PATH;
-    if (stats) {
-        subfacet_update_stats(subfacet, stats);
-    }
 
     LIST_FOR_EACH (packet, list_node, &miss->packets) {
         struct flow_miss_op *op = &ops[*n_ops];
@@ -3800,6 +3796,27 @@ handle_flow_miss_with_facet(struct flow_miss *miss, struct facet *facet,
         }
     }
 
+    /* Don't install the flow if it's the result of the "userspace"
+     * action for an already installed facet.  This can occur when a
+     * datapath flow with wildcards has a "userspace" action and flows
+     * sent to userspace result in a different subfacet, which will then
+     * be rejected as overlapping by the datapath. */
+    if (miss->upcall_type == DPIF_UC_ACTION
+        && !list_is_empty(&facet->subfacets)) {
+        if (stats) {
+            facet->used = MAX(facet->used, stats->used);
+            facet->packet_count += stats->n_packets;
+            facet->byte_count += stats->n_bytes;
+            facet->tcp_flags |= stats->tcp_flags;
+        }
+        return;
+    }
+
+    subfacet = subfacet_create(facet, miss, now);
+    if (stats) {
+        subfacet_update_stats(subfacet, stats);
+    }
+
     if (miss->upcall_type == DPIF_UC_MISS || subfacet->path != want_path) {
         struct flow_miss_op *op = &ops[(*n_ops)++];
         struct dpif_flow_put *put = &op->dpif_op.u.flow_put;
@@ -3815,7 +3832,7 @@ handle_flow_miss_with_facet(struct flow_miss *miss, struct facet *facet,
         op->xout_garbage = false;
         op->dpif_op.type = DPIF_OP_FLOW_PUT;
         op->subfacet = subfacet;
-        put->flags = DPIF_FP_CREATE | DPIF_FP_MODIFY;
+        put->flags = DPIF_FP_CREATE;
         put->key = miss->key;
         put->key_len = miss->key_len;
         put->mask = op->mask.data;
@@ -4160,6 +4177,18 @@ handle_miss_upcalls(struct dpif_backer *backer, struct dpif_upcall *upcalls,
 
             COVERAGE_INC(subfacet_install_fail);
 
+            /* Zero-out subfacet counters when installation failed, but
+             * datapath reported hits.  This should not happen and
+             * indicates a bug, since if the datapath flow exists, we
+             * should not be attempting to create a new subfacet.  A
+             * buggy datapath could trigger this, so just zero out the
+             * counters and log an error. */
+            if (subfacet->dp_packet_count || subfacet->dp_byte_count) {
+                VLOG_ERR_RL(&rl, "failed to install subfacet for which "
+                            "datapath reported hits");
+                subfacet->dp_packet_count = subfacet->dp_byte_count = 0;
+            }
+
             subfacet->path = SF_NOT_INSTALLED;
         }
 
@@ -5417,7 +5446,8 @@ subfacet_install(struct subfacet *subfacet, const struct ofpbuf *odp_actions,
     enum dpif_flow_put_flags flags;
     int ret;
 
-    flags = DPIF_FP_CREATE | DPIF_FP_MODIFY;
+    flags = subfacet->path == SF_NOT_INSTALLED ? DPIF_FP_CREATE
+                                               : DPIF_FP_MODIFY;
     if (stats) {
         flags |= DPIF_FP_ZERO_STATS;
     }
@@ -6702,6 +6732,10 @@ xlate_fin_timeout(struct xlate_ctx *ctx,
     if (ctx->xin->tcp_flags & (TCP_FIN | TCP_RST) && ctx->rule) {
         struct rule_dpif *rule = ctx->rule;
 
+        if (list_is_empty(&rule->up.expirable)) {
+            list_insert(&ctx->ofproto->up.expirable, &rule->up.expirable);
+        }
+
         reduce_timeout(oft->fin_idle_timeout, &rule->up.idle_timeout);
         reduce_timeout(oft->fin_hard_timeout, &rule->up.hard_timeout);
     }
@@ -7096,12 +7130,12 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
      *   to another device without any modifications this will cause us to
      *   insert a new tag since the original one was stripped off by the
      *   VLAN device.
-     * - Tunnel 'flow' is largely cleared when transitioning between
-     *   the input and output stages since it does not make sense to output
-     *   a packet with the exact headers that it was received with (i.e.
-     *   the destination IP is us).  The one exception is the tun_id, which
-     *   is preserved to allow use in later resubmit lookups and loads into
-     *   registers.
+     * - Tunnel metadata as received is retained in 'flow'. This allows
+     *   tunnel metadata matching also in later tables.
+     *   Since a kernel action for setting the tunnel metadata will only be
+     *   generated with actual tunnel output, changing the tunnel metadata
+     *   values in 'flow' (such as tun_id) will only have effect with a later
+     *   tunnel output action.
      * - Tunnel 'base_flow' is completely cleared since that is what the
      *   kernel does.  If we wish to maintain the original values an action
      *   needs to be generated. */
@@ -8800,8 +8834,17 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
 
     HMAP_FOR_EACH (subfacet, hmap_node, &ofproto->subfacets) {
         struct facet *facet = subfacet->facet;
+        struct odputil_keybuf maskbuf;
+        struct ofpbuf mask;
+
+        ofpbuf_use_stack(&mask, &maskbuf, sizeof maskbuf);
+        if (enable_megaflows) {
+            odp_flow_key_from_mask(&mask, &facet->xout.wc.masks,
+                                   &facet->flow, UINT32_MAX);
+        }
 
-        odp_flow_key_format(subfacet->key, subfacet->key_len, &ds);
+        odp_flow_format(subfacet->key, subfacet->key_len,
+                        mask.data, mask.size, &ds);
 
         ds_put_format(&ds, ", packets:%"PRIu64", bytes:%"PRIu64", used:",
                       subfacet->dp_packet_count, subfacet->dp_byte_count);