[ovs-dev] [PATCHv4 5/7] dpif: Don't synchronize flow_dump_next() status.
Joe Stringer
joestringer at nicira.com
Thu Feb 27 22:13:09 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 e3b547d..c50694b 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -552,6 +552,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);
@@ -563,7 +564,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)
@@ -640,6 +645,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 dcf621c..af21dff 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4166,6 +4166,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) {
@@ -4183,7 +4184,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,
@@ -4201,8 +4205,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 8f1716d..4b00118 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -761,10 +761,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);
@@ -783,15 +784,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,
@@ -827,8 +831,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