Yeah, I'd like to get a gauge of this as well. I'll follow up on this.
On 28 May 2014 14:28, Ethan Jackson <[email protected]> wrote: > Could we measure if this actually causes a performance degradation? > I'd honestly be surprised if it does . . . My guess is our bottlenecks > are elsewhere, but perhaps they aren't. > > Ethan > > On Tue, May 27, 2014 at 7:13 PM, Joe Stringer <[email protected]> > wrote: > > Thanks for the Ack, I pushed this to branch-2.3. > > > > I'd like to send a patch for master anyway, in part so that we ensure to > > address this (fix the bug, and make the code more readable), and in part > > because it still matters if newer userspace is used with an older kernel. > > > > > > On 28 May 2014 13:52, Alex Wang <[email protected]> wrote: > >> > >> Acked-by: Alex Wang <[email protected]> > >> > >> > >> For master, I'm about to refactor the datapath so that we never dump > >> duplicated flows. So, I think we should consider refactoring master > after > >> that. > >> > >> > >> Thanks, > >> Alex Wang, > >> > >> > >> On Tue, May 27, 2014 at 6:19 PM, Joe Stringer <[email protected]> > >> wrote: > >>> > >>> A series of bugs have been identified recently that are caused by a > >>> combination of the awkward flow dump API, possibility of duplicate > flows > >>> in a flow dump, and premature optimisation of the revalidator logic. > >>> This patch attempts to simplify the revalidator logic by combining > >>> multiple critical sections into one, which should make the state more > >>> consistent. > >>> > >>> The new flow of logic is: > >>> + Lookup the ukey. > >>> + If the ukey doesn't exist, create it. > >>> + Insert the ukey into the udpif. If we can't insert it, skip this > flow. > >>> + Lock the ukey. If we can't lock it, skip it. > >>> + Determine if the ukey was already handled. If it has, skip it. > >>> + Revalidate. > >>> + Update ukey's fields (mark, flow_exists). > >>> + Unlock the ukey. > >>> > >>> Previously, we would attempt process a flow without creating a ukey if > >>> it hadn't been dumped before and it was due to be deleted. This patch > >>> changes this to always create a ukey, allowing the ukey's > >>> mutex to be used as the basis for preventing a flow from being handled > >>> twice. This is expected to cause some minor performance regression for > >>> cases like TCP_CRR, in favour of correctness and readability. > >>> > >>> Bug #1252997. > >>> > >>> Signed-off-by: Joe Stringer <[email protected]> > >>> --- > >>> ofproto/ofproto-dpif-upcall.c | 64 > >>> ++++++++++++++++++----------------------- > >>> 1 file changed, 28 insertions(+), 36 deletions(-) > >>> > >>> diff --git a/ofproto/ofproto-dpif-upcall.c > >>> b/ofproto/ofproto-dpif-upcall.c > >>> index 64444d9..906ccb4 100644 > >>> --- a/ofproto/ofproto-dpif-upcall.c > >>> +++ b/ofproto/ofproto-dpif-upcall.c > >>> @@ -1205,6 +1205,7 @@ revalidate_ukey(struct udpif *udpif, struct > >>> udpif_key *ukey, > >>> const struct nlattr *mask, size_t mask_len, > >>> const struct nlattr *actions, size_t actions_len, > >>> const struct dpif_flow_stats *stats) > >>> + OVS_REQUIRES(ukey->mutex) > >>> { > >>> uint64_t slow_path_buf[128 / 8]; > >>> struct xlate_out xout, *xoutp; > >>> @@ -1225,7 +1226,6 @@ revalidate_ukey(struct udpif *udpif, struct > >>> udpif_key *ukey, > >>> xoutp = NULL; > >>> netflow = NULL; > >>> > >>> - ovs_mutex_lock(&ukey->mutex); > >>> last_used = ukey->stats.used; > >>> push.used = stats->used; > >>> push.tcp_flags = stats->tcp_flags; > >>> @@ -1236,11 +1236,6 @@ revalidate_ukey(struct udpif *udpif, struct > >>> udpif_key *ukey, > >>> ? stats->n_bytes - ukey->stats.n_bytes > >>> : 0; > >>> > >>> - if (!ukey->flow_exists) { > >>> - /* Don't bother revalidating if the flow was already deleted. > */ > >>> - goto exit; > >>> - } > >>> - > >>> if (udpif->need_revalidate && last_used > >>> && !should_revalidate(push.n_packets, last_used)) { > >>> ok = false; > >>> @@ -1320,7 +1315,6 @@ revalidate_ukey(struct udpif *udpif, struct > >>> udpif_key *ukey, > >>> ok = true; > >>> > >>> exit: > >>> - ovs_mutex_unlock(&ukey->mutex); > >>> if (netflow) { > >>> if (!ok) { > >>> netflow_expire(netflow, &flow); > >>> @@ -1460,25 +1454,38 @@ revalidate(struct revalidator *revalidator) > >>> ukey = ukey_lookup(udpif, key, key_len, hash); > >>> > >>> used = stats->used; > >>> - if (ukey) { > >>> - ovs_mutex_lock(&ukey->mutex); > >>> - > >>> - if (ukey->mark || !ukey->flow_exists) { > >>> - /* The flow has already been dumped. This can > >>> occasionally > >>> - * occur if the datapath is changed in the middle of a > >>> flow > >>> - * dump. Rather than perform the same work twice, skip > >>> the > >>> - * flow this time. */ > >>> - ovs_mutex_unlock(&ukey->mutex); > >>> + if (!ukey) { > >>> + ukey = ukey_create(key, key_len, used); > >>> + if (!udpif_insert_ukey(udpif, ukey, hash)) { > >>> + /* The same ukey has already been created. This means > >>> that > >>> + * another revalidator is processing this flow > >>> + * concurrently, so don't bother processing it. */ > >>> COVERAGE_INC(upcall_duplicate_flow); > >>> + ukey_delete(NULL, ukey); > >>> goto next; > >>> } > >>> + } > >>> > >>> - if (!used) { > >>> - used = ukey->created; > >>> - } > >>> + if (ovs_mutex_trylock(&ukey->mutex)) { > >>> + /* The flow has been dumped, and is being handled by > another > >>> + * revalidator concurrently. This can occasionally occur > if > >>> the > >>> + * datapath is changed in the middle of a flow dump. > Rather > >>> than > >>> + * perform the same work twice, skip the flow this time. > */ > >>> + COVERAGE_INC(upcall_duplicate_flow); > >>> + goto next; > >>> + } > >>> + > >>> + if (ukey->mark || !ukey->flow_exists) { > >>> + /* The flow has already been dumped and handled by another > >>> + * revalidator during this flow dump operation. Skip it. > */ > >>> + COVERAGE_INC(upcall_duplicate_flow); > >>> ovs_mutex_unlock(&ukey->mutex); > >>> + goto next; > >>> } > >>> > >>> + if (!used) { > >>> + used = ukey->created; > >>> + } > >>> n_flows = udpif_get_n_flows(udpif); > >>> max_idle = ofproto_max_idle; > >>> if (n_flows > flow_limit) { > >>> @@ -1488,30 +1495,15 @@ revalidate(struct revalidator *revalidator) > >>> if ((used && used < now - max_idle) || n_flows > flow_limit * > 2) > >>> { > >>> mark = false; > >>> } else { > >>> - if (!ukey) { > >>> - ukey = ukey_create(key, key_len, used); > >>> - if (!udpif_insert_ukey(udpif, ukey, hash)) { > >>> - /* The same ukey has already been created. This > >>> means that > >>> - * another revalidator is processing this flow > >>> - * concurrently, so don't bother processing it. */ > >>> - ukey_delete(NULL, ukey); > >>> - goto next; > >>> - } > >>> - } > >>> - > >>> mark = revalidate_ukey(udpif, ukey, mask, mask_len, > actions, > >>> actions_len, stats); > >>> } > >>> - > >>> - if (ukey) { > >>> - ovs_mutex_lock(&ukey->mutex); > >>> - ukey->mark = ukey->flow_exists = mark; > >>> - ovs_mutex_unlock(&ukey->mutex); > >>> - } > >>> + ukey->mark = ukey->flow_exists = mark; > >>> > >>> if (!mark) { > >>> dump_op_init(&ops[n_ops++], key, key_len, ukey); > >>> } > >>> + ovs_mutex_unlock(&ukey->mutex); > >>> > >>> next: > >>> may_destroy = > dpif_flow_dump_next_may_destroy_keys(&udpif->dump, > >>> -- > >>> 1.7.10.4 > >>> > >> > > > > > > _______________________________________________ > > dev mailing list > > [email protected] > > http://openvswitch.org/mailman/listinfo/dev > > >
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
