[ovs-dev] [branch-2.3] revalidator: Eliminate duplicate flow handling.
Joe Stringer
joestringer at nicira.com
Wed May 28 02:33:21 UTC 2014
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 <ethan at 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 <joestringer at 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 <alexw at nicira.com> wrote:
> >>
> >> Acked-by: Alex Wang <alexw at 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 <joestringer at 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 <joestringer at 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 at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140528/6ac08655/attachment-0005.html>
More information about the dev
mailing list