> Hi Ryan, a few broad comments: > > Could you split this into two patches, the dpif interface change and the > dpif-netdev implementation change? > > Sure will do.
> I'm a little uneasy about the change to the dpif_flow_dump_next() > interface, just because the size of the output parameter was previously > paired with the output parameter itself. With this patch, there's no > programmatic way to check that the output array is the correct size. I > think it would be more explicit to keep the flow_dump_next() max_flows > parameter, and maybe just assert that it is <= thread_->max_flows. > > The comment above dpif_flow_dump_next() may also need to be updated. > Sure I'll add the max_flows argument back to dpif_flow_dump_next() and add an assert. I shouldn't need to change the dpif_flow_dump_next() comment though by doing this. Cheers, Ryan > > > On 19 June 2014 09:50, Ryan Wilson <[email protected]> wrote: > >> Previously, flows were retrieved one by one when dumping flows for >> datapaths of type 'netdev'. This increased contention for the dump's >> mutex, negatively affecting revalidator performance. >> >> This patch retrieves batches of flows when dumping flows for datapaths >> of type 'netdev'. >> >> Signed-off-by: Ryan Wilson <[email protected]> >> --- >> lib/dpif-linux.c | 7 ++-- >> lib/dpif-netdev.c | 87 >> +++++++++++++++++++++++------------------ >> lib/dpif-provider.h | 9 +++-- >> lib/dpif.c | 10 ++--- >> lib/dpif.h | 4 +- >> ofproto/ofproto-dpif-upcall.c | 5 ++- >> ofproto/ofproto-dpif.c | 4 +- >> utilities/ovs-dpctl.c | 4 +- >> 8 files changed, 72 insertions(+), 58 deletions(-) >> >> diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c >> index afe9340..a46e2db 100644 >> --- a/lib/dpif-linux.c >> +++ b/lib/dpif-linux.c >> @@ -1203,13 +1203,13 @@ dpif_linux_flow_dump_thread_cast(struct >> dpif_flow_dump_thread *thread) >> } >> >> static struct dpif_flow_dump_thread * >> -dpif_linux_flow_dump_thread_create(struct dpif_flow_dump *dump_) >> +dpif_linux_flow_dump_thread_create(struct dpif_flow_dump *dump_, int >> max_flows) >> { >> struct dpif_linux_flow_dump *dump = dpif_linux_flow_dump_cast(dump_); >> struct dpif_linux_flow_dump_thread *thread; >> >> thread = xmalloc(sizeof *thread); >> - dpif_flow_dump_thread_init(&thread->up, &dump->up); >> + dpif_flow_dump_thread_init(&thread->up, &dump->up, max_flows); >> thread->dump = dump; >> ofpbuf_init(&thread->nl_flows, NL_DUMP_BUFSIZE); >> thread->nl_actions = NULL; >> @@ -1243,12 +1243,13 @@ dpif_linux_flow_to_dpif_flow(struct dpif_flow >> *dpif_flow, >> >> static int >> dpif_linux_flow_dump_next(struct dpif_flow_dump_thread *thread_, >> - struct dpif_flow *flows, int max_flows) >> + struct dpif_flow *flows) >> { >> struct dpif_linux_flow_dump_thread *thread >> = dpif_linux_flow_dump_thread_cast(thread_); >> struct dpif_linux_flow_dump *dump = thread->dump; >> struct dpif_linux *dpif = dpif_linux_cast(thread->up.dpif); >> + int max_flows = thread_->max_flows; >> int n_flows; >> >> ofpbuf_delete(thread->nl_actions); >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index 6c281fe..36c442d 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -1423,8 +1423,8 @@ dpif_netdev_flow_dump_destroy(struct dpif_flow_dump >> *dump_) >> struct dpif_netdev_flow_dump_thread { >> struct dpif_flow_dump_thread up; >> struct dpif_netdev_flow_dump *dump; >> - struct odputil_keybuf keybuf; >> - struct odputil_keybuf maskbuf; >> + struct odputil_keybuf *keybuf; >> + struct odputil_keybuf *maskbuf; >> }; >> >> static struct dpif_netdev_flow_dump_thread * >> @@ -1434,14 +1434,16 @@ dpif_netdev_flow_dump_thread_cast(struct >> dpif_flow_dump_thread *thread) >> } >> >> static struct dpif_flow_dump_thread * >> -dpif_netdev_flow_dump_thread_create(struct dpif_flow_dump *dump_) >> +dpif_netdev_flow_dump_thread_create(struct dpif_flow_dump *dump_, int >> max_flows) >> { >> struct dpif_netdev_flow_dump *dump = >> dpif_netdev_flow_dump_cast(dump_); >> struct dpif_netdev_flow_dump_thread *thread; >> >> thread = xmalloc(sizeof *thread); >> - dpif_flow_dump_thread_init(&thread->up, &dump->up); >> + dpif_flow_dump_thread_init(&thread->up, &dump->up, max_flows); >> thread->dump = dump; >> + thread->keybuf = xmalloc(max_flows * sizeof *thread->keybuf); >> + thread->maskbuf = xmalloc(max_flows * sizeof *thread->maskbuf); >> return &thread->up; >> } >> >> @@ -1451,29 +1453,35 @@ dpif_netdev_flow_dump_thread_destroy(struct >> dpif_flow_dump_thread *thread_) >> struct dpif_netdev_flow_dump_thread *thread >> = dpif_netdev_flow_dump_thread_cast(thread_); >> >> + free(thread->keybuf); >> + free(thread->maskbuf); >> free(thread); >> } >> >> /* XXX the caller must use 'actions' without quiescing */ >> static int >> dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_, >> - struct dpif_flow *f, int max_flows OVS_UNUSED) >> + struct dpif_flow *f) >> { >> struct dpif_netdev_flow_dump_thread *thread >> = dpif_netdev_flow_dump_thread_cast(thread_); >> struct dpif_netdev_flow_dump *dump = thread->dump; >> struct dpif_netdev *dpif = dpif_netdev_cast(thread->up.dpif); >> struct dp_netdev *dp = get_dp_netdev(&dpif->dpif); >> - struct dp_netdev_flow *netdev_flow; >> - struct flow_wildcards wc; >> - struct dp_netdev_actions *dp_actions; >> - struct ofpbuf buf; >> - int error; >> + int max_flows = thread_->max_flows; >> + int n_flows = 0; >> >> ovs_mutex_lock(&dump->mutex); >> - error = dump->status; >> - if (!error) { >> + >> + while (n_flows < max_flows && !dump->status) { >> + struct dp_netdev_flow *netdev_flow; >> + struct flow_wildcards wc; >> + struct dp_netdev_actions *dp_actions; >> struct hmap_node *node; >> + struct ofpbuf buf; >> + struct dpif_flow *flow = &f[n_flows]; >> + struct odputil_keybuf *keybuf = &thread->keybuf[n_flows]; >> + struct odputil_keybuf *maskbuf = &thread->maskbuf[n_flows]; >> >> fat_rwlock_rdlock(&dp->cls.rwlock); >> node = hmap_at_position(&dp->flow_table, &dump->bucket, >> &dump->offset); >> @@ -1482,40 +1490,41 @@ dpif_netdev_flow_dump_next(struct >> dpif_flow_dump_thread *thread_, >> } >> fat_rwlock_unlock(&dp->cls.rwlock); >> if (!node) { >> - dump->status = error = EOF; >> + dump->status = EOF; >> + break; >> } >> - } >> - ovs_mutex_unlock(&dump->mutex); >> - if (error) { >> - return 0; >> - } >> >> - minimask_expand(&netdev_flow->cr.match.mask, &wc); >> + minimask_expand(&netdev_flow->cr.match.mask, &wc); >> >> - /* Key. */ >> - ofpbuf_use_stack(&buf, &thread->keybuf, sizeof thread->keybuf); >> - odp_flow_key_from_flow(&buf, &netdev_flow->flow, &wc.masks, >> - netdev_flow->flow.in_port.odp_port, true); >> - f->key = ofpbuf_data(&buf); >> - f->key_len = ofpbuf_size(&buf); >> + /* Key. */ >> + ofpbuf_use_stack(&buf, keybuf, sizeof *keybuf); >> + odp_flow_key_from_flow(&buf, &netdev_flow->flow, &wc.masks, >> + netdev_flow->flow.in_port.odp_port, true); >> + flow->key = ofpbuf_data(&buf); >> + flow->key_len = ofpbuf_size(&buf); >> >> - /* Mask. */ >> - ofpbuf_use_stack(&buf, &thread->maskbuf, sizeof thread->maskbuf); >> - odp_flow_key_from_mask(&buf, &wc.masks, &netdev_flow->flow, >> - odp_to_u32(wc.masks.in_port.odp_port), >> - SIZE_MAX, true); >> - f->mask = ofpbuf_data(&buf); >> - f->mask_len = ofpbuf_size(&buf); >> + /* Mask. */ >> + ofpbuf_use_stack(&buf, maskbuf, sizeof *maskbuf); >> + odp_flow_key_from_mask(&buf, &wc.masks, &netdev_flow->flow, >> + odp_to_u32(wc.masks.in_port.odp_port), >> + SIZE_MAX, true); >> + flow->mask = ofpbuf_data(&buf); >> + flow->mask_len = ofpbuf_size(&buf); >> >> - /* Actions. */ >> - dp_actions = dp_netdev_flow_get_actions(netdev_flow); >> - f->actions = dp_actions->actions; >> - f->actions_len = dp_actions->size; >> + /* Actions. */ >> + dp_actions = dp_netdev_flow_get_actions(netdev_flow); >> + flow->actions = dp_actions->actions; >> + flow->actions_len = dp_actions->size; >> >> - /* Stats. */ >> - get_dpif_flow_stats(netdev_flow, &f->stats); >> + /* Stats. */ >> + get_dpif_flow_stats(netdev_flow, &flow->stats); >> + >> + n_flows++; >> + } >> + >> + ovs_mutex_unlock(&dump->mutex); >> >> - return 1; >> + return n_flows; >> } >> >> static int >> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h >> index b762ac0..cc954c9 100644 >> --- a/lib/dpif-provider.h >> +++ b/lib/dpif-provider.h >> @@ -63,13 +63,16 @@ dpif_flow_dump_init(struct dpif_flow_dump *dump, >> const struct dpif *dpif) >> >> struct dpif_flow_dump_thread { >> struct dpif *dpif; >> + int max_flows; >> }; >> >> static inline void >> dpif_flow_dump_thread_init(struct dpif_flow_dump_thread *thread, >> - struct dpif_flow_dump *dump) >> + struct dpif_flow_dump *dump, >> + int max_flows) >> { >> thread->dpif = dump->dpif; >> + thread->max_flows = max_flows; >> } >> >> /* Datapath interface class structure, to be defined by each >> implementation of >> @@ -312,11 +315,11 @@ struct dpif_class { >> int (*flow_dump_destroy)(struct dpif_flow_dump *dump); >> >> struct dpif_flow_dump_thread *(*flow_dump_thread_create)( >> - struct dpif_flow_dump *dump); >> + struct dpif_flow_dump *dump, int max_flows); >> void (*flow_dump_thread_destroy)(struct dpif_flow_dump_thread >> *thread); >> >> int (*flow_dump_next)(struct dpif_flow_dump_thread *thread, >> - struct dpif_flow *flows, int max_flows); >> + struct dpif_flow *flows); >> >> /* Performs the 'execute->actions_len' bytes of actions in >> * 'execute->actions' on the Ethernet frame in 'execute->packet' >> diff --git a/lib/dpif.c b/lib/dpif.c >> index cace47b..3d3cbba 100644 >> --- a/lib/dpif.c >> +++ b/lib/dpif.c >> @@ -1000,9 +1000,10 @@ dpif_flow_dump_destroy(struct dpif_flow_dump *dump) >> >> /* Returns new thread-local state for use with dpif_flow_dump_next(). */ >> struct dpif_flow_dump_thread * >> -dpif_flow_dump_thread_create(struct dpif_flow_dump *dump) >> +dpif_flow_dump_thread_create(struct dpif_flow_dump *dump, int max_flows) >> { >> - return dump->dpif->dpif_class->flow_dump_thread_create(dump); >> + ovs_assert(max_flows > 0); >> + return dump->dpif->dpif_class->flow_dump_thread_create(dump, >> max_flows); >> } >> >> /* Releases 'thread'. */ >> @@ -1031,13 +1032,12 @@ dpif_flow_dump_thread_destroy(struct >> dpif_flow_dump_thread *thread) >> * dpif_flow_dump_next() for 'thread'. */ >> int >> dpif_flow_dump_next(struct dpif_flow_dump_thread *thread, >> - struct dpif_flow *flows, int max_flows) >> + struct dpif_flow *flows) >> { >> struct dpif *dpif = thread->dpif; >> int n; >> >> - ovs_assert(max_flows > 0); >> - n = dpif->dpif_class->flow_dump_next(thread, flows, max_flows); >> + n = dpif->dpif_class->flow_dump_next(thread, flows); >> if (n > 0) { >> struct dpif_flow *f; >> >> diff --git a/lib/dpif.h b/lib/dpif.h >> index f080cde..4a11d2e 100644 >> --- a/lib/dpif.h >> +++ b/lib/dpif.h >> @@ -558,7 +558,7 @@ struct dpif_flow_dump *dpif_flow_dump_create(const >> struct dpif *); >> int dpif_flow_dump_destroy(struct dpif_flow_dump *); >> >> struct dpif_flow_dump_thread *dpif_flow_dump_thread_create( >> - struct dpif_flow_dump *); >> + struct dpif_flow_dump *, int max_flows); >> void dpif_flow_dump_thread_destroy(struct dpif_flow_dump_thread *); >> >> /* A datapath flow as dumped by dpif_flow_dump_next(). */ >> @@ -572,7 +572,7 @@ struct dpif_flow { >> struct dpif_flow_stats stats; /* Flow statistics. */ >> }; >> int dpif_flow_dump_next(struct dpif_flow_dump_thread *, >> - struct dpif_flow *flows, int max_flows); >> + struct dpif_flow *flows); >> >> /* Operation batching interface. >> * >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index b38f226..a25f6b9 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -1332,7 +1332,8 @@ revalidate(struct revalidator *revalidator) >> >> dump_seq = seq_read(udpif->dump_seq); >> atomic_read(&udpif->flow_limit, &flow_limit); >> - dump_thread = dpif_flow_dump_thread_create(udpif->dump); >> + dump_thread = dpif_flow_dump_thread_create(udpif->dump, >> + REVALIDATE_MAX_BATCH); >> for (;;) { >> struct dump_op ops[REVALIDATE_MAX_BATCH]; >> int n_ops = 0; >> @@ -1346,7 +1347,7 @@ revalidate(struct revalidator *revalidator) >> size_t n_dp_flows; >> bool kill_them_all; >> >> - n_dumped = dpif_flow_dump_next(dump_thread, flows, >> ARRAY_SIZE(flows)); >> + n_dumped = dpif_flow_dump_next(dump_thread, flows); >> if (!n_dumped) { >> break; >> } >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index 9e4a455..e3f8945 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -4486,8 +4486,8 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn >> *conn, >> >> ds_init(&ds); >> flow_dump = dpif_flow_dump_create(ofproto->backer->dpif); >> - flow_dump_thread = dpif_flow_dump_thread_create(flow_dump); >> - while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) { >> + flow_dump_thread = dpif_flow_dump_thread_create(flow_dump, 1); >> + while (dpif_flow_dump_next(flow_dump_thread, &f)) { >> if (!ofproto_dpif_contains_flow(ofproto, f.key, f.key_len)) { >> continue; >> } >> diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c >> index 62fc1dd..c842782 100644 >> --- a/utilities/ovs-dpctl.c >> +++ b/utilities/ovs-dpctl.c >> @@ -792,8 +792,8 @@ dpctl_dump_flows(int argc, char *argv[]) >> >> ds_init(&ds); >> flow_dump = dpif_flow_dump_create(dpif); >> - flow_dump_thread = dpif_flow_dump_thread_create(flow_dump); >> - while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) { >> + flow_dump_thread = dpif_flow_dump_thread_create(flow_dump, 1); >> + while (dpif_flow_dump_next(flow_dump_thread, &f)) { >> if (filter) { >> struct flow flow; >> struct flow_wildcards wc; >> -- >> 1.7.9.5 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> http://openvswitch.org/mailman/listinfo/dev >> > >
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
