[ovs-dev] [PATCH 07/10] dpif: Don't fetch actions in dpif_flow_dump_next().
Joe Stringer
joestringer at nicira.com
Fri Jan 10 19:43:12 UTC 2014
Previously when dumping flows from the datapath, callers of
dpif_flow_dump_next() could specify whether they wish to fetch the
actions for a flow. Depending on the datapath implementation, these may
be fetched at no performance cost or may require additional resources
to fetch.
The most frequent caller of this function is the flow_dumper, which
does not care about fetching actions. This patch removes the actions
parameters from dpif_flow_dump_next(), to provide more consistent
performance in this common use case. If callers wish to get the actions
for a flow, they can make a separate call to flow_get().
Signed-off-by: Joe Stringer <joestringer at nicira.com>
---
lib/dpif-linux.c | 24 ------------------------
lib/dpif-netdev.c | 20 +++-----------------
lib/dpif-provider.h | 7 +------
lib/dpif.c | 13 ++-----------
lib/dpif.h | 1 -
ofproto/ofproto-dpif-upcall.c | 4 ++--
ofproto/ofproto-dpif.c | 16 +++++++++++-----
utilities/ovs-dpctl.c | 15 ++++++++++-----
8 files changed, 29 insertions(+), 71 deletions(-)
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 45a2492..cac3f52 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -1024,7 +1024,6 @@ static int
dpif_linux_flow_dump_next(const struct dpif *dpif_ OVS_UNUSED, void *state_,
const struct nlattr **key, size_t *key_len,
const struct nlattr **mask, size_t *mask_len,
- const struct nlattr **actions, size_t *actions_len,
const struct dpif_flow_stats **stats)
{
struct dpif_linux_flow_state *state = state_;
@@ -1040,31 +1039,8 @@ dpif_linux_flow_dump_next(const struct dpif *dpif_ OVS_UNUSED, void *state_,
if (error) {
return error;
}
-
- if (actions && !state->flow.actions) {
- struct ofpbuf *tmp_buf = NULL;
-
- error = dpif_linux_flow_get__(dpif_, state->flow.key,
- state->flow.key_len,
- &state->flow, &tmp_buf);
- if (tmp_buf) {
- ofpbuf_delete(state->buf);
- state->buf = tmp_buf;
- }
-
- if (error == ENOENT) {
- VLOG_DBG("dumped flow disappeared on get");
- } else if (error) {
- VLOG_WARN("error fetching dumped flow: %s",
- ovs_strerror(error));
- }
- }
} while (error);
- if (actions) {
- *actions = state->flow.actions;
- *actions_len = state->flow.actions_len;
- }
if (key) {
*key = state->flow.key;
*key_len = state->flow.key_len;
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index fdea0a7..1c3869a 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1302,7 +1302,6 @@ dpif_netdev_flow_del(struct dpif *dpif, const struct dpif_flow_del *del)
struct dp_netdev_flow_state {
uint32_t bucket;
uint32_t offset;
- struct dp_netdev_actions *actions;
struct odputil_keybuf keybuf;
struct odputil_keybuf maskbuf;
struct dpif_flow_stats stats;
@@ -1316,7 +1315,6 @@ dpif_netdev_flow_dump_start(const struct dpif *dpif OVS_UNUSED, void **statep)
*statep = state = xmalloc(sizeof *state);
state->bucket = 0;
state->offset = 0;
- state->actions = NULL;
return 0;
}
@@ -1324,7 +1322,6 @@ static int
dpif_netdev_flow_dump_next(const struct dpif *dpif, void *state_,
const struct nlattr **key, size_t *key_len,
const struct nlattr **mask, size_t *mask_len,
- const struct nlattr **actions, size_t *actions_len,
const struct dpif_flow_stats **stats)
{
struct dp_netdev_flow_state *state = state_;
@@ -1367,20 +1364,10 @@ dpif_netdev_flow_dump_next(const struct dpif *dpif, void *state_,
*mask_len = buf.size;
}
- if (actions || stats) {
- dp_netdev_actions_unref(state->actions);
- state->actions = NULL;
-
+ if (stats) {
ovs_mutex_lock(&netdev_flow->mutex);
- if (actions) {
- state->actions = dp_netdev_actions_ref(netdev_flow->actions);
- *actions = state->actions->actions;
- *actions_len = state->actions->size;
- }
- if (stats) {
- get_dpif_flow_stats(netdev_flow, &state->stats);
- *stats = &state->stats;
- }
+ get_dpif_flow_stats(netdev_flow, &state->stats);
+ *stats = &state->stats;
ovs_mutex_unlock(&netdev_flow->mutex);
}
@@ -1394,7 +1381,6 @@ dpif_netdev_flow_dump_done(const struct dpif *dpif OVS_UNUSED, void *state_)
{
struct dp_netdev_flow_state *state = state_;
- dp_netdev_actions_unref(state->actions);
free(state);
return 0;
}
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index adc5242..3323e16 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -285,12 +285,8 @@ struct dpif_class {
* must be set to Netlink attributes with types of OVS_KEY_ATTR_*
* representing the dumped flow's mask.
*
- * - If 'actions' and 'actions_len' are nonnull then they should be set
- * to Netlink attributes with types OVS_ACTION_ATTR_* representing
- * the dumped flow's actions.
- *
* - If 'stats' is nonnull then it should be set to the dumped flow's
- * statistics.
+ * statistics.
*
* All of the returned data is owned by 'dpif', not by the caller, and the
* caller must not modify or free it. 'dpif' must guarantee that it
@@ -299,7 +295,6 @@ struct dpif_class {
int (*flow_dump_next)(const struct dpif *dpif, void *state,
const struct nlattr **key, size_t *key_len,
const struct nlattr **mask, size_t *mask_len,
- const struct nlattr **actions, size_t *actions_len,
const struct dpif_flow_stats **stats);
/* Releases resources from 'dpif' for 'state', which was initialized by a
diff --git a/lib/dpif.c b/lib/dpif.c
index 2b79a6e..ecaeb28 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -984,9 +984,7 @@ dpif_flow_dump_start(struct dpif_flow_dump *dump, const struct dpif *dpif)
*
* On success, if 'key' and 'key_len' are nonnull then '*key' and '*key_len'
* will be set to Netlink attributes with types OVS_KEY_ATTR_* representing the
- * dumped flow's key. If 'actions' and 'actions_len' are nonnull then they are
- * set to Netlink attributes with types OVS_ACTION_ATTR_* representing the
- * dumped flow's actions. If 'stats' is nonnull then it will be set to the
+ * dumped flow's key. If 'stats' is nonnull then it will be set to the
* dumped flow's statistics.
*
* All of the returned data is owned by 'dpif', not by the caller, and the
@@ -997,7 +995,6 @@ bool
dpif_flow_dump_next(struct dpif_flow_dump *dump,
const struct nlattr **key, size_t *key_len,
const struct nlattr **mask, size_t *mask_len,
- const struct nlattr **actions, size_t *actions_len,
const struct dpif_flow_stats **stats)
{
const struct dpif *dpif = dump->dpif;
@@ -1007,7 +1004,6 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump,
error = dpif->dpif_class->flow_dump_next(dpif, dump->state,
key, key_len,
mask, mask_len,
- actions, actions_len,
stats);
if (error) {
dpif->dpif_class->flow_dump_done(dpif, dump->state);
@@ -1022,10 +1018,6 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump,
*mask = NULL;
*mask_len = 0;
}
- if (actions) {
- *actions = NULL;
- *actions_len = 0;
- }
if (stats) {
*stats = NULL;
}
@@ -1037,8 +1029,7 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump,
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);
+ stats ? *stats : NULL, NULL, 0);
}
}
dump->error = error;
diff --git a/lib/dpif.h b/lib/dpif.h
index 7f986f9..277c1f1 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -513,7 +513,6 @@ void dpif_flow_dump_start(struct dpif_flow_dump *, const struct dpif *);
bool dpif_flow_dump_next(struct dpif_flow_dump *,
const struct nlattr **key, size_t *key_len,
const struct nlattr **mask, size_t *mask_len,
- const struct nlattr **actions, size_t *actions_len,
const struct dpif_flow_stats **);
int dpif_flow_dump_done(struct dpif_flow_dump *);
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index dd24f5c..4789f43 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -534,8 +534,8 @@ udpif_flow_dumper(void *arg)
start_time = time_msec();
dpif_flow_dump_start(&dump, udpif->dpif);
- while (dpif_flow_dump_next(&dump, &key, &key_len, &mask, &mask_len,
- NULL, NULL, &stats)
+ while (dpif_flow_dump_next(&dump, &key, &key_len,
+ &mask, &mask_len, &stats)
&& !latch_is_set(&udpif->exit_latch)) {
struct udpif_flow_dump *udump = xmalloc(sizeof *udump);
struct revalidator *revalidator;
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 62c1e4c..b70b66d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4056,10 +4056,8 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
const struct dpif_flow_stats *stats;
const struct ofproto_dpif *ofproto;
struct dpif_flow_dump flow_dump;
- const struct nlattr *actions;
const struct nlattr *mask;
const struct nlattr *key;
- size_t actions_len;
size_t mask_len;
size_t key_len;
bool verbosity = false;
@@ -4084,18 +4082,26 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
ds_init(&ds);
dpif_flow_dump_start(&flow_dump, ofproto->backer->dpif);
- while (dpif_flow_dump_next(&flow_dump, &key, &key_len, &mask, &mask_len,
- &actions, &actions_len, &stats)) {
+ while (dpif_flow_dump_next(&flow_dump, &key, &key_len,
+ &mask, &mask_len, &stats)) {
+ struct ofpbuf *actions;
+
if (!ofproto_dpif_contains_flow(ofproto, key, key_len)) {
continue;
}
+ if (dpif_flow_get(flow_dump.dpif, key, key_len, &actions, NULL)) {
+ VLOG_DBG("dpif/dump_flows: failed to retrieve actions");
+ continue;
+ }
+
odp_flow_format(key, key_len, mask, mask_len, &portno_names, &ds,
verbosity);
ds_put_cstr(&ds, ", ");
dpif_flow_stats_format(stats, &ds);
ds_put_cstr(&ds, ", actions:");
- format_odp_actions(&ds, actions, actions_len);
+ format_odp_actions(&ds, actions->data, actions->size);
+ ofpbuf_delete(actions);
ds_put_char(&ds, '\n');
}
diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index 09db084..0a7dd15 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -747,7 +747,6 @@ static void
dpctl_dump_flows(int argc, char *argv[])
{
const struct dpif_flow_stats *stats;
- const struct nlattr *actions;
struct dpif_flow_dump flow_dump;
const struct nlattr *key;
const struct nlattr *mask;
@@ -755,7 +754,6 @@ dpctl_dump_flows(int argc, char *argv[])
struct dpif_port_dump port_dump;
struct hmap portno_names;
struct simap names_portno;
- size_t actions_len;
struct dpif *dpif;
size_t key_len;
size_t mask_len;
@@ -791,8 +789,9 @@ dpctl_dump_flows(int argc, char *argv[])
ds_init(&ds);
dpif_flow_dump_start(&flow_dump, dpif);
while (dpif_flow_dump_next(&flow_dump, &key, &key_len,
- &mask, &mask_len,
- &actions, &actions_len, &stats)) {
+ &mask, &mask_len, &stats)) {
+ struct ofpbuf *actions;
+
if (filter) {
struct flow flow;
struct flow_wildcards wc;
@@ -813,6 +812,11 @@ dpctl_dump_flows(int argc, char *argv[])
}
minimatch_destroy(&minimatch);
}
+
+ if (dpif_flow_get(flow_dump.dpif, key, key_len, &actions, NULL)) {
+ continue;
+ }
+
ds_clear(&ds);
odp_flow_format(key, key_len, mask, mask_len, &portno_names, &ds,
verbosity);
@@ -820,7 +824,8 @@ dpctl_dump_flows(int argc, char *argv[])
dpif_flow_stats_format(stats, &ds);
ds_put_cstr(&ds, ", actions:");
- format_odp_actions(&ds, actions, actions_len);
+ format_odp_actions(&ds, actions->data, actions->size);
+ ofpbuf_delete(actions);
printf("%s\n", ds_cstr(&ds));
}
dpif_flow_dump_done(&flow_dump);
--
1.7.9.5
More information about the dev
mailing list