[ovs-dev] [PATCHv3 5/7] dpif: Don't synchronize flow_dump_next() status.

Joe Stringer joestringer at nicira.com
Wed Feb 12 18:37:08 UTC 2014


Recent changes to the flow_dump_next() interface have made it the
responsibility of the dpif implementation to track error status over a
flow dump operation.

This patch removes status tracking from 'struct dpif_flow_dump', allowing
multiple threads to call dpif_flow_dump_next() and track their status
independently. Even if one thread finishes processing flows for a given
iterator and state, it will not prevent other callers from processing
the remaining flows in their buffers.

After this patch, the error code that dpif_flow_dump_next() returns is
only significant for the current state and buffer. As before, the status
of the entire flow dump operation can be obtained by calling
dpif_flow_dump_done().

Signed-off-by: Joe Stringer <joestringer at nicira.com>
---
v3: Rebase.
v2: Completely remove dump->status.
---
 lib/dpif.c                    |   68 +++++++++++++++++------------------------
 lib/dpif.h                    |    3 +-
 ofproto/ofproto-dpif-upcall.c |    8 ++++-
 ofproto/ofproto-dpif.c        |   10 ++++--
 utilities/ovs-dpctl.c         |   22 ++++++++-----
 5 files changed, 59 insertions(+), 52 deletions(-)

diff --git a/lib/dpif.c b/lib/dpif.c
index f972011..117d720 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -978,23 +978,22 @@ dpif_flow_dump_state_uninit(const struct dpif *dpif, void *state)
     dpif->dpif_class->flow_dump_state_uninit(state);
 }
 
-/* Initializes 'dump' to begin dumping the flows in a dpif.
- *
- * This function provides no status indication.  An error status for the entire
- * dump operation is provided when it is completed by calling
- * dpif_flow_dump_done().
- */
-void
+/* Initializes 'dump' to begin dumping the flows in a dpif. On sucess,
+ * initializes 'dump' with any data needed for iteration and returns 0.
+ * Otherwise, returns a positive errno value describing the problem. */
+int
 dpif_flow_dump_start(struct dpif_flow_dump *dump, const struct dpif *dpif)
 {
+    int error;
     dump->dpif = dpif;
-    dump->error = dpif->dpif_class->flow_dump_start(dpif, &dump->iter);
-    log_operation(dpif, "flow_dump_start", dump->error);
+    error = dpif->dpif_class->flow_dump_start(dpif, &dump->iter);
+    log_operation(dpif, "flow_dump_start", error);
+    return error;
 }
 
 /* Attempts to retrieve another flow from 'dump', using 'state' for
- * thread-local storage. 'dump' must have been initialized with
- * dpif_flow_dump_start(), and 'state' must have been initialized with
+ * thread-local storage. 'dump' must have been initialized with a successful
+ * call to dpif_flow_dump_start(), and 'state' must have been initialized with
  * dpif_flow_state_init().
  *
  * On success, updates the output parameters as described below and returns
@@ -1023,18 +1022,11 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump, void *state,
                     const struct dpif_flow_stats **stats)
 {
     const struct dpif *dpif = dump->dpif;
-    int error = dump->error;
+    int error;
 
-    if (!error) {
-        error = dpif->dpif_class->flow_dump_next(dpif, dump->iter, state,
-                                                 key, key_len,
-                                                 mask, mask_len,
-                                                 actions, actions_len,
-                                                 stats);
-        if (error) {
-            dpif->dpif_class->flow_dump_done(dpif, dump->iter);
-        }
-    }
+    error = dpif->dpif_class->flow_dump_next(dpif, dump->iter, state,
+                                             key, key_len, mask, mask_len,
+                                             actions, actions_len, stats);
     if (error) {
         if (key) {
             *key = NULL;
@@ -1052,33 +1044,29 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump, void *state,
             *stats = NULL;
         }
     }
-    if (!dump->error) {
-        if (error == EOF) {
-            VLOG_DBG_RL(&dpmsg_rl, "%s: dumped all flows", dpif_name(dpif));
-        } else if (should_log_flow_message(error)) {
-            log_flow_message(dpif, error, "flow_dump",
-                             key ? *key : NULL, key ? *key_len : 0,
-                             mask ? *mask : NULL, mask ? *mask_len : 0,
-                             stats ? *stats : NULL, actions ? *actions : NULL,
-                             actions ? *actions_len : 0);
-        }
+    if (error == EOF) {
+        VLOG_DBG_RL(&dpmsg_rl, "%s: dumped all flows", dpif_name(dpif));
+    } else if (should_log_flow_message(error)) {
+        log_flow_message(dpif, error, "flow_dump",
+                         key ? *key : NULL, key ? *key_len : 0,
+                         mask ? *mask : NULL, mask ? *mask_len : 0,
+                         stats ? *stats : NULL, actions ? *actions : NULL,
+                         actions ? *actions_len : 0);
     }
-    dump->error = error;
     return !error;
 }
 
 /* Completes flow table dump operation 'dump', which must have been initialized
- * with dpif_flow_dump_start().  Returns 0 if the dump operation was
- * error-free, otherwise a positive errno value describing the problem. */
+ * with a successful call to dpif_flow_dump_start().  Returns 0 if the dump
+ * operation was error-free, otherwise a positive errno value describing the
+ * problem. */
 int
 dpif_flow_dump_done(struct dpif_flow_dump *dump)
 {
     const struct dpif *dpif = dump->dpif;
-    if (!dump->error) {
-        dump->error = dpif->dpif_class->flow_dump_done(dpif, dump->iter);
-        log_operation(dpif, "flow_dump_done", dump->error);
-    }
-    return dump->error == EOF ? 0 : dump->error;
+    int error = dpif->dpif_class->flow_dump_done(dpif, dump->iter);
+    log_operation(dpif, "flow_dump_done", error);
+    return error == EOF ? 0 : error;
 }
 
 struct dpif_execute_helper_aux {
diff --git a/lib/dpif.h b/lib/dpif.h
index 65de686..0c763dc 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -514,11 +514,10 @@ int dpif_flow_get(const struct dpif *,
 
 struct dpif_flow_dump {
     const struct dpif *dpif;
-    int error;
     void *iter;
 };
 void dpif_flow_dump_state_init(const struct dpif *, void **statep);;
-void dpif_flow_dump_start(struct dpif_flow_dump *, const struct dpif *);
+int dpif_flow_dump_start(struct dpif_flow_dump *, const struct dpif *);
 bool dpif_flow_dump_next(struct dpif_flow_dump *, void *state,
                          const struct nlattr **key, size_t *key_len,
                          const struct nlattr **mask, size_t *mask_len,
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index a0e77a8..9a8fc34 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -536,6 +536,7 @@ udpif_flow_dumper(void *arg)
         bool need_revalidate;
         uint64_t reval_seq;
         size_t n_flows, i;
+        int error;
         void *state = NULL;
 
         reval_seq = seq_read(udpif->reval_seq);
@@ -547,7 +548,11 @@ udpif_flow_dumper(void *arg)
         udpif->avg_n_flows = (udpif->avg_n_flows + n_flows) / 2;
 
         start_time = time_msec();
-        dpif_flow_dump_start(&dump, udpif->dpif);
+        error = dpif_flow_dump_start(&dump, udpif->dpif);
+        if (error) {
+            VLOG_INFO("Failed to start flow dump (%s)", ovs_strerror(error));
+            goto skip;
+        }
         dpif_flow_dump_state_init(udpif->dpif, &state);
         while (dpif_flow_dump_next(&dump, state, &key, &key_len,
                                    &mask, &mask_len, NULL, NULL, &stats)
@@ -624,6 +629,7 @@ udpif_flow_dumper(void *arg)
                       duration);
         }
 
+skip:
         poll_timer_wait_until(start_time + MIN(MAX_IDLE, 500));
         seq_wait(udpif->reval_seq, udpif->last_reval_seq);
         latch_wait(&udpif->exit_latch);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 842fb26..0acf343 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4128,6 +4128,7 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
     struct dpif_port_dump port_dump;
     struct hmap portno_names;
     void *state = NULL;
+    int error;
 
     ofproto = ofproto_dpif_lookup(argv[argc - 1]);
     if (!ofproto) {
@@ -4145,7 +4146,10 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
     }
 
     ds_init(&ds);
-    dpif_flow_dump_start(&flow_dump, ofproto->backer->dpif);
+    error = dpif_flow_dump_start(&flow_dump, ofproto->backer->dpif);
+    if (error) {
+        goto exit;
+    }
     dpif_flow_dump_state_init(ofproto->backer->dpif, &state);
     while (dpif_flow_dump_next(&flow_dump, state, &key, &key_len,
                                &mask, &mask_len, &actions, &actions_len,
@@ -4163,8 +4167,10 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
         ds_put_char(&ds, '\n');
     }
     dpif_flow_dump_state_uninit(ofproto->backer->dpif, state);
+    error = dpif_flow_dump_done(&flow_dump);
 
-    if (dpif_flow_dump_done(&flow_dump)) {
+exit:
+    if (error) {
         ds_clear(&ds);
         ds_put_format(&ds, "dpif/dump_flows failed: %s", ovs_strerror(errno));
         unixctl_command_reply_error(conn, ds_cstr(&ds));
diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index 04b3f73..1cc684d 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -760,10 +760,11 @@ dpctl_dump_flows(int argc, char *argv[])
     size_t key_len;
     size_t mask_len;
     struct ds ds;
-    char *name, *error, *filter = NULL;
+    char *name, *filter = NULL;
     struct flow flow_filter;
     struct flow_wildcards wc_filter;
     void *state = NULL;
+    int error;
 
     if (argc > 1 && !strncmp(argv[argc - 1], "filter=", 7)) {
         filter = xstrdup(argv[--argc] + 7);
@@ -782,15 +783,18 @@ dpctl_dump_flows(int argc, char *argv[])
     }
 
     if (filter) {
-        error = parse_ofp_exact_flow(&flow_filter, &wc_filter.masks, filter,
-                                     &names_portno);
-        if (error) {
-            ovs_fatal(0, "Failed to parse filter (%s)", error);
+        char *err = parse_ofp_exact_flow(&flow_filter, &wc_filter.masks,
+                                         filter, &names_portno);
+        if (err) {
+            ovs_fatal(0, "Failed to parse filter (%s)", err);
         }
     }
 
     ds_init(&ds);
-    dpif_flow_dump_start(&flow_dump, dpif);
+    error = dpif_flow_dump_start(&flow_dump, dpif);
+    if (error) {
+        goto exit;
+    }
     dpif_flow_dump_state_init(dpif, &state);
     while (dpif_flow_dump_next(&flow_dump, state, &key, &key_len,
                                &mask, &mask_len, &actions, &actions_len,
@@ -826,8 +830,12 @@ dpctl_dump_flows(int argc, char *argv[])
         printf("%s\n", ds_cstr(&ds));
     }
     dpif_flow_dump_state_uninit(dpif, state);
-    dpif_flow_dump_done(&flow_dump);
+    error = dpif_flow_dump_done(&flow_dump);
 
+exit:
+    if (error) {
+        ovs_fatal(error, "Failed to dump flows from datapath");
+    }
     free(filter);
     odp_portno_names_destroy(&portno_names);
     hmap_destroy(&portno_names);
-- 
1.7.9.5




More information about the dev mailing list