I couldn't measure a difference in TCP_CRR. I dropped that part of the message from the patch that I sent for master:
http://openvswitch.org/pipermail/dev/2014-May/040863.html On 28 May 2014 14:33, Joe Stringer <joestrin...@nicira.com> wrote: > 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 <et...@nicira.com> 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 <joestrin...@nicira.com> >> 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 <al...@nicira.com> wrote: >> >> >> >> Acked-by: Alex Wang <al...@nicira.com> >> >> >> >> >> >> 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 <joestrin...@nicira.com> >> >> 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 <joestrin...@nicira.com> >> >>> --- >> >>> 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 >> > dev@openvswitch.org >> > http://openvswitch.org/mailman/listinfo/dev >> > >> > >
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
