This patch makes it the caller's responsibility to allocate and pass a buffer down to the dpif_flow_dump_next() implementation, to act as storage for the next flow. The implementation can expect to be called from multiple threads with the same 'state' and different 'buffer's.
When flow_dump_next() returns non-zero, the implementation must ensure that subsequent calls with the same arguments also return non-zero. Subsequent calls with the same 'state' and different 'buffer' may return zero, but should make progress towards returning non-zero. Furthermore, the 'stats' argument becomes a pointer, not a double pointer. If this argument is non-null, then the 'dpif' will populate it with the stats of the flow. This should make reallocation unnecessary. Signed-off-by: Joe Stringer <[email protected]> --- lib/dpif-linux.c | 25 +++++++--------- lib/dpif-netdev.c | 64 ++++++++++++++++++++++++----------------- lib/dpif-provider.h | 39 +++++++++++++++---------- lib/dpif.c | 11 +++---- lib/dpif.h | 4 +-- ofproto/ofproto-dpif-upcall.c | 9 ++++-- ofproto/ofproto-dpif.c | 9 ++++-- utilities/ovs-dpctl.c | 9 ++++-- 8 files changed, 96 insertions(+), 74 deletions(-) diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c index cac3f52..babb9e7 100644 --- a/lib/dpif-linux.c +++ b/lib/dpif-linux.c @@ -991,9 +991,6 @@ dpif_linux_flow_del(struct dpif *dpif_, const struct dpif_flow_del *del) struct dpif_linux_flow_state { struct nl_dump dump; - struct dpif_linux_flow flow; - struct dpif_flow_stats stats; - struct ofpbuf *buf; }; static int @@ -1015,43 +1012,42 @@ dpif_linux_flow_dump_start(const struct dpif *dpif_, void **statep) nl_dump_start(&state->dump, NETLINK_GENERIC, buf); ofpbuf_delete(buf); - state->buf = ofpbuf_new(1024); - return 0; } static int dpif_linux_flow_dump_next(const struct dpif *dpif_ OVS_UNUSED, void *state_, + struct ofpbuf *buffer, const struct nlattr **key, size_t *key_len, const struct nlattr **mask, size_t *mask_len, - const struct dpif_flow_stats **stats) + struct dpif_flow_stats *stats) { struct dpif_linux_flow_state *state = state_; + struct dpif_linux_flow flow; struct ofpbuf buf; int error; do { - if (!nl_dump_next(&state->dump, &buf, state->buf)) { + if (!nl_dump_next(&state->dump, &buf, buffer)) { return EOF; } - error = dpif_linux_flow_from_ofpbuf(&state->flow, &buf); + error = dpif_linux_flow_from_ofpbuf(&flow, &buf); if (error) { return error; } } while (error); if (key) { - *key = state->flow.key; - *key_len = state->flow.key_len; + *key = flow.key; + *key_len = flow.key_len; } if (mask) { - *mask = state->flow.mask; - *mask_len = state->flow.mask ? state->flow.mask_len : 0; + *mask = flow.mask; + *mask_len = flow.mask ? flow.mask_len : 0; } if (stats) { - dpif_linux_flow_get_stats(&state->flow, &state->stats); - *stats = &state->stats; + dpif_linux_flow_get_stats(&flow, stats); } return error; } @@ -1061,7 +1057,6 @@ dpif_linux_flow_dump_done(const struct dpif *dpif OVS_UNUSED, void *state_) { struct dpif_linux_flow_state *state = state_; int error = nl_dump_done(&state->dump); - ofpbuf_delete(state->buf); free(state); return error; } diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 1c3869a..f880ad0 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -1302,9 +1302,8 @@ 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 odputil_keybuf keybuf; - struct odputil_keybuf maskbuf; - struct dpif_flow_stats stats; + int status; + struct ovs_mutex mutex; }; static int @@ -1315,59 +1314,71 @@ 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->status = 0; + ovs_mutex_init(&state->mutex); return 0; } static int dpif_netdev_flow_dump_next(const struct dpif *dpif, void *state_, + struct ofpbuf *buffer, const struct nlattr **key, size_t *key_len, const struct nlattr **mask, size_t *mask_len, - const struct dpif_flow_stats **stats) + struct dpif_flow_stats *stats) { struct dp_netdev_flow_state *state = state_; struct dp_netdev *dp = get_dp_netdev(dpif); struct dp_netdev_flow *netdev_flow; - struct hmap_node *node; + int error; - ovs_rwlock_rdlock(&dp->cls.rwlock); - node = hmap_at_position(&dp->flow_table, &state->bucket, &state->offset); - if (node) { - netdev_flow = CONTAINER_OF(node, struct dp_netdev_flow, node); - dp_netdev_flow_ref(netdev_flow); + ovs_mutex_lock(&state->mutex); + error = state->status; + if (!error) { + struct hmap_node *node; + + ovs_rwlock_rdlock(&dp->cls.rwlock); + node = hmap_at_position(&dp->flow_table, &state->bucket, + &state->offset); + if (node) { + netdev_flow = CONTAINER_OF(node, struct dp_netdev_flow, node); + dp_netdev_flow_ref(netdev_flow); + } + ovs_rwlock_unlock(&dp->cls.rwlock); + if (!node) { + state->status = error = EOF; + } } - ovs_rwlock_unlock(&dp->cls.rwlock); - if (!node) { - return EOF; + ovs_mutex_unlock(&state->mutex); + if (error) { + return error; } if (key) { - struct ofpbuf buf; - - ofpbuf_use_stack(&buf, &state->keybuf, sizeof state->keybuf); - odp_flow_key_from_flow(&buf, &netdev_flow->flow, + odp_flow_key_from_flow(buffer, &netdev_flow->flow, netdev_flow->flow.in_port.odp_port); - *key = buf.data; - *key_len = buf.size; + *key = buffer->data; + *key_len = buffer->size; + + ofpbuf_pull(buffer, buffer->size); } if (key && mask) { - struct ofpbuf buf; struct flow_wildcards wc; - ofpbuf_use_stack(&buf, &state->maskbuf, sizeof state->maskbuf); minimask_expand(&netdev_flow->cr.match.mask, &wc); - odp_flow_key_from_mask(&buf, &wc.masks, &netdev_flow->flow, + odp_flow_key_from_mask(buffer, &wc.masks, &netdev_flow->flow, odp_to_u32(wc.masks.in_port.odp_port)); - *mask = buf.data; - *mask_len = buf.size; + *mask = buffer->data; + *mask_len = buffer->size; + + ofpbuf_pull(buffer, buffer->size); } if (stats) { ovs_mutex_lock(&netdev_flow->mutex); - get_dpif_flow_stats(netdev_flow, &state->stats); - *stats = &state->stats; + get_dpif_flow_stats(netdev_flow, stats); ovs_mutex_unlock(&netdev_flow->mutex); } @@ -1381,6 +1392,7 @@ dpif_netdev_flow_dump_done(const struct dpif *dpif OVS_UNUSED, void *state_) { struct dp_netdev_flow_state *state = state_; + ovs_mutex_destroy(&state->mutex); free(state); return 0; } diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index 3323e16..3086893 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -267,13 +267,17 @@ struct dpif_class { * failure, returns a positive errno value. */ int (*flow_dump_start)(const struct dpif *dpif, void **statep); - /* Attempts to retrieve another flow from 'dpif' for 'state', which was - * initialized by a successful call to the 'flow_dump_start' function for - * 'dpif'. On success, updates the output parameters as described below - * and returns 0. Returns EOF if the end of the flow table has been - * reached, or a positive errno value on error. This function will not be - * called again once it returns nonzero within a given iteration (but the - * 'flow_dump_done' function will be called afterward). + /* Attempts to retrieve another flow from 'dpif' for 'state', using + * 'buffer' for storage. 'state' must have been initialized by a successful + * call to the 'flow_dump_start' function for 'dpif'. 'buffer' must have + * been initialised with enough space to fit a netlink message. + * + * On success, updates the output parameters as described below and returns + * 0. Returns EOF if the end of the flow table has been reached, or a + * positive errno value on error. Multiple threads may use the same 'dpif' + * and 'state' with this function, but must provide an independent + * 'buffer'. If this function returns non-zero, subsequent calls with the + * same arguments will also return non-zero. * * On success: * @@ -285,20 +289,25 @@ struct dpif_class { * must be set to Netlink attributes with types of OVS_KEY_ATTR_* * representing the dumped flow's mask. * - * - If 'stats' is nonnull then it should be set to the dumped flow's - * statistics. + * - If 'stats' is nonnull then it should be populated with the dumped + * flow's 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 - * remains accessible and unchanging until at least the next call to - * 'flow_dump_next' or 'flow_dump_done' for 'state'. */ + * The returned data is owned by the caller, and points within 'buffer'. + * 'state' is owned by 'dpif', and the caller must not modify or free it. + * 'buffer' is reserved by 'dpif', and the caller should not modify or free + * it until flow_dump_next() returns non-zero for the same 'state' and + * 'buffer'. + */ int (*flow_dump_next)(const struct dpif *dpif, void *state, + struct ofpbuf *buffer, const struct nlattr **key, size_t *key_len, const struct nlattr **mask, size_t *mask_len, - const struct dpif_flow_stats **stats); + struct dpif_flow_stats *stats); /* Releases resources from 'dpif' for 'state', which was initialized by a - * successful call to the 'flow_dump_start' function for 'dpif'. */ + * successful call to the 'flow_dump_start' function for 'dpif'. Callers + * must ensure that this function is called once within a given iteration, + * as the final flow dump operation. */ int (*flow_dump_done)(const struct dpif *dpif, void *state); /* Performs the 'execute->actions_len' bytes of actions in diff --git a/lib/dpif.c b/lib/dpif.c index ecaeb28..afbab35 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -992,16 +992,16 @@ dpif_flow_dump_start(struct dpif_flow_dump *dump, const struct dpif *dpif) * accessible and unchanging until at least the next call to 'flow_dump_next' * or 'flow_dump_done' for 'dump'. */ bool -dpif_flow_dump_next(struct dpif_flow_dump *dump, +dpif_flow_dump_next(struct dpif_flow_dump *dump, struct ofpbuf *buffer, const struct nlattr **key, size_t *key_len, const struct nlattr **mask, size_t *mask_len, - const struct dpif_flow_stats **stats) + struct dpif_flow_stats *stats) { const struct dpif *dpif = dump->dpif; int error = dump->error; if (!error) { - error = dpif->dpif_class->flow_dump_next(dpif, dump->state, + error = dpif->dpif_class->flow_dump_next(dpif, dump->state, buffer, key, key_len, mask, mask_len, stats); @@ -1018,9 +1018,6 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump, *mask = NULL; *mask_len = 0; } - if (stats) { - *stats = NULL; - } } if (!dump->error) { if (error == EOF) { @@ -1029,7 +1026,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, NULL, 0); + stats ? stats : NULL, NULL, 0); } } dump->error = error; diff --git a/lib/dpif.h b/lib/dpif.h index 277c1f1..2e8b7b8 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -510,10 +510,10 @@ struct dpif_flow_dump { void *state; }; void dpif_flow_dump_start(struct dpif_flow_dump *, const struct dpif *); -bool dpif_flow_dump_next(struct dpif_flow_dump *, +bool dpif_flow_dump_next(struct dpif_flow_dump *, struct ofpbuf *buffer, const struct nlattr **key, size_t *key_len, const struct nlattr **mask, size_t *mask_len, - const struct dpif_flow_stats **); + struct dpif_flow_stats *); int dpif_flow_dump_done(struct dpif_flow_dump *); /* Operation batching interface. diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 4789f43..1f5d385 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -501,8 +501,9 @@ udpif_flow_dumper(void *arg) set_subprogram_name("flow_dumper"); while (!latch_is_set(&udpif->exit_latch)) { - const struct dpif_flow_stats *stats; + struct dpif_flow_stats stats; long long int start_time, duration; + struct ofpbuf buffer; const struct nlattr *key, *mask; struct dpif_flow_dump dump; size_t key_len, mask_len; @@ -534,7 +535,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, + ofpbuf_init(&buffer, 4096); + while (dpif_flow_dump_next(&dump, &buffer, &key, &key_len, &mask, &mask_len, &stats) && !latch_is_set(&udpif->exit_latch)) { struct udpif_flow_dump *udump = xmalloc(sizeof *udump); @@ -549,7 +551,7 @@ udpif_flow_dumper(void *arg) udump->mask = (struct nlattr *) &udump->mask_buf; udump->mask_len = mask_len; - udump->stats = *stats; + memcpy(&udump->stats, &stats, sizeof stats); udump->need_revalidate = need_revalidate; revalidator = &udpif->revalidators[udump->key_hash @@ -566,6 +568,7 @@ udpif_flow_dumper(void *arg) xpthread_cond_signal(&revalidator->wake_cond); ovs_mutex_unlock(&revalidator->mutex); } + ofpbuf_uninit(&buffer); dpif_flow_dump_done(&dump); /* Let all the revalidators finish and garbage collect. */ diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index b70b66d..40ef2b1 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4053,9 +4053,10 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn, void *aux OVS_UNUSED) { struct ds ds = DS_EMPTY_INITIALIZER; - const struct dpif_flow_stats *stats; + struct dpif_flow_stats stats; const struct ofproto_dpif *ofproto; struct dpif_flow_dump flow_dump; + struct ofpbuf buffer; const struct nlattr *mask; const struct nlattr *key; size_t mask_len; @@ -4081,8 +4082,9 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn, } ds_init(&ds); + ofpbuf_init(&buffer, 4096); dpif_flow_dump_start(&flow_dump, ofproto->backer->dpif); - while (dpif_flow_dump_next(&flow_dump, &key, &key_len, + while (dpif_flow_dump_next(&flow_dump, &buffer, &key, &key_len, &mask, &mask_len, &stats)) { struct ofpbuf *actions; @@ -4098,12 +4100,13 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn, odp_flow_format(key, key_len, mask, mask_len, &portno_names, &ds, verbosity); ds_put_cstr(&ds, ", "); - dpif_flow_stats_format(stats, &ds); + dpif_flow_stats_format(&stats, &ds); ds_put_cstr(&ds, ", actions:"); format_odp_actions(&ds, actions->data, actions->size); ofpbuf_delete(actions); ds_put_char(&ds, '\n'); } + ofpbuf_uninit(&buffer); if (dpif_flow_dump_done(&flow_dump)) { ds_clear(&ds); diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c index 0a7dd15..f7f5b79 100644 --- a/utilities/ovs-dpctl.c +++ b/utilities/ovs-dpctl.c @@ -746,8 +746,9 @@ dpctl_dump_dps(int argc OVS_UNUSED, char *argv[] OVS_UNUSED) static void dpctl_dump_flows(int argc, char *argv[]) { - const struct dpif_flow_stats *stats; + struct dpif_flow_stats stats; struct dpif_flow_dump flow_dump; + struct ofpbuf buffer; const struct nlattr *key; const struct nlattr *mask; struct dpif_port dpif_port; @@ -787,8 +788,9 @@ dpctl_dump_flows(int argc, char *argv[]) } ds_init(&ds); + ofpbuf_init(&buffer, 4096); dpif_flow_dump_start(&flow_dump, dpif); - while (dpif_flow_dump_next(&flow_dump, &key, &key_len, + while (dpif_flow_dump_next(&flow_dump, &buffer, &key, &key_len, &mask, &mask_len, &stats)) { struct ofpbuf *actions; @@ -822,13 +824,14 @@ dpctl_dump_flows(int argc, char *argv[]) verbosity); ds_put_cstr(&ds, ", "); - dpif_flow_stats_format(stats, &ds); + dpif_flow_stats_format(&stats, &ds); ds_put_cstr(&ds, ", actions:"); format_odp_actions(&ds, actions->data, actions->size); ofpbuf_delete(actions); printf("%s\n", ds_cstr(&ds)); } dpif_flow_dump_done(&flow_dump); + ofpbuf_uninit(&buffer); free(filter); odp_portno_names_destroy(&portno_names); -- 1.7.9.5 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
