On Wed, Jan 06, 2016 at 02:50:36PM -0800, Joe Stringer wrote: > On 5 January 2016 at 16:24, Ben Pfaff <b...@ovn.org> wrote: > > revalidate_sweep__() has two cases where it calls ukey_delete() to > > remove a ukey from the umap via cmap_remove(). The first case is a direct > > call to ukey_delete(), when !flow_exists. The second case is an indirect > > call via push_ukey_ops(), when result != UKEY_KEEP. If both of these > > conditions are simultaneously true, however, the code would call > > ukey_delete() twice, causing an assertion failure in the second call. This > > commit fixes the problem by eliminating one of the calls. > > > > Reported-by: Keith Holleman <keith.holle...@gmail.com> > > Reported-at: > > http://openvswitch.org/pipermail/discuss/2015-December/019772.html > > CC: Joe Stringer <j...@ovn.org> > > VMware-BZ: #1579057 > > Signed-off-by: Ben Pfaff <b...@ovn.org> > > > > --- > > ofproto/ofproto-dpif-upcall.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > > index cd8a9f0..ca25701 100644 > > --- a/ofproto/ofproto-dpif-upcall.c > > +++ b/ofproto/ofproto-dpif-upcall.c > > @@ -1,4 +1,4 @@ > > -/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc. > > +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, > > Inc. > > * > > * Licensed under the Apache License, Version 2.0 (the "License"); > > * you may not use this file except in compliance with the License. > > @@ -2274,7 +2274,7 @@ revalidator_sweep__(struct revalidator *revalidator, > > bool purge) > > n_ops = 0; > > } > > > > - if (!flow_exists) { > > + if (result == UKEY_KEEP && !flow_exists) { > > ovs_mutex_lock(&umap->mutex); > > ukey_delete(umap, ukey); > > ovs_mutex_unlock(&umap->mutex); > > Thanks for finding this. > > In the common case, I would expect the flow to be deleted during the > revalidator "dump" (mark) phase. With the current code, and with this > approach, we may end up attempting to delete the same flows multiple > times. I suggest the below instead: > > @@ -2262,7 +2264,7 @@ revalidator_sweep__(struct revalidator > *revalidator, bool purge) > result = revalidate_ukey(udpif, ukey, &stats, &odp_actions, > reval_seq, &recircs); > } > - if (result != UKEY_KEEP) { > + if (flow_exists && result != UKEY_KEEP) { > /* Takes ownership of 'recircs'. */ > reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs, > &odp_actions);
Thanks! I applied this change and pushed it to master and branch-2.5. > I notice a couple of other minor issues as well while looking at this code: > - We may currently revalidate ukeys in revalidator_sweep() whenever > the dump_seq/reval_seq mismatch, even if the ukey's corresponding flow > is already deleted. This is unnecessary: If we have previously deleted > the flow, we can just delete the ukey. > - If there is a seq mismatch in dump_seq or reval_seq, and > revalidate_ukey() returns UKEY_MODIFY, the flow will be modified and > the ukey deleted. I believe that this will not clear the datapath > stats, so next time the flow is dumped, all previous stats for the > flow will be double-attributed. This seems fairly unlikely as it > should only happen if the seqs change in between dump phase and > revalidation phase, or if the flow is not dumped by the kernel (as > well as only if revalidation requires modification of the flow - ie > actions changed but flows did not). > > I will send patches for these latter issues. Thanks! _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
