[ovs-dev] [PATCH 4/8] ofproto-dpif: Probe datapath for MPLS stack depth supported

[Ben Pfaff]

On Wed, Jan 15, 2014 at 04:13:21PM +0900, Simon Horman wrote:
> This is an proposed enhancement to
> "Implement OpenFlow support for MPLS, for up to 3 labels."
> 
> It is in preparation for not adding flows to the datapath
> which have more MPLS LSEs than the datapath supports.
> 
> Signed-off-by: Simon Horman <horms at verge.net.au>

Thanks a lot!

I think that every iteration of the array probed the same depth, so I
folded this in:

-        flow_set_mpls_bos(&flow, ARRAY_SIZE(flow.mpls_lse) - 1, 1);
+        flow_set_mpls_bos(&flow, i, 1);

I decided that probing from 0 up, instead of from the max down, was a
slight improvement, because if you run into a funny error along the
way you can always return the best you've seen so far instead of
failing all the way back to 0.

I decided that DPIF_FP_CREATE | DPIF_FP_MODIFY made more sense than
just DPIF_FP_CREATE, because I wouldn't want some leftover flow from a
previous run to cause a problem.

I decided that EEXIST was actually a form of success: it means we hit
some overlapping flow, which in turn means that the flow we're trying
to add is OK.

I decided that 'n' was a better name for the variable than 'i' ;-)

Here's what I'm folding in:

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 1fe5957..b9811b5 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -981,47 +981,41 @@ check_variable_length_userdata(struct dpif_backer *backer)
 static size_t
 check_max_mpls_depth(struct dpif_backer *backer)
 {
-    size_t i;
     struct flow flow;
-    int error;
+    size_t n;
 
-    /* If ARRAY_SIZE(flow.mpls_lse) becomes large then it might
-     * make more sense to do a binary search here. */
-    for (i = ARRAY_SIZE(flow.mpls_lse) - 1; i > 0; i--) {
+    for (n = 0; n < ARRAY_SIZE(flow.mpls_lse); n++) {
         struct odputil_keybuf keybuf;
         struct ofpbuf key;
+        int error;
 
         memset(&flow, 0, sizeof flow);
         flow.dl_type = htons(ETH_TYPE_MPLS);
-        flow_set_mpls_bos(&flow, ARRAY_SIZE(flow.mpls_lse) - 1, 1);
+        flow_set_mpls_bos(&flow, n, 1);
 
         ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
         odp_flow_key_from_flow(&key, &flow, 0);
 
-        error = dpif_flow_put(backer->dpif, DPIF_FP_CREATE, key.data, key.size,
-                              NULL, 0, NULL, 0, NULL);
-        if (!error) {
-            error = dpif_flow_del(backer->dpif, key.data, key.size, NULL);
-            if (error) {
-                goto err;
+        error = dpif_flow_put(backer->dpif, DPIF_FP_CREATE | DPIF_FP_MODIFY,
+                              key.data, key.size, NULL, 0, NULL, 0, NULL);
+        if (error && error != EEXIST) {
+            if (error != EINVAL) {
+                VLOG_WARN("%s: MPLS stack length feature probe failed (%s)",
+                          dpif_name(backer->dpif), ovs_strerror(error));
             }
-            i++;
             break;
-        } else if (error != EINVAL) {
-            goto err;
+        }
+
+        error = dpif_flow_del(backer->dpif, key.data, key.size, NULL);
+        if (error) {
+            VLOG_WARN("%s: failed to delete MPLS feature probe flow",
+                      dpif_name(backer->dpif));
         }
     }
 
     VLOG_INFO("%s: MPLS label stack length probed as %"PRIuSIZE,
-              dpif_name(backer->dpif), i);
-    return i;
-
-err:
-    /* Something odd happened.  We're not sure how large an
-     * MPLS label stack the datapath supports. Default to 0. */
-    VLOG_WARN("%s: mpls stack length feature probe failed (%s)",
-              dpif_name(backer->dpif), ovs_strerror(error));
-    return 0;
+              dpif_name(backer->dpif), n);
+    return n;
 }
 
 static int