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
