Looks good to me :-) Acked-by: Jarno Rajahalme <[email protected]>
> On Mar 13, 2014, at 10:20 PM, Andy Zhou <[email protected]> wrote: > > Found during development. > > Before this commit, all datapath flows are cleared with dpif_flush(), > but the revalidator thread still holds ukeys, which are caches of the > datapath flows in the revalidaor. Flushing ukeys causes flow_del > messages to be sent to the datapath again on flows that have been > deleted by the dpif_flush() already. > > Double deletion by itself is not problem, per se, may an efficiency > issue. However, for ever flow_del message sent to the datapath, a log > message, at the warning level, will be generated in case datapath > failed to execute the command. In addition to cause spurious log > messages, Double deletion causes unit tests to report erroneous > failures as all warning messages are considered test failures. > > The fix is to simply shut down the revalidator threads to flush all > ukeys, then flush the datapth before restarting the revalidator threads. > > dpif_flush() was implemented as flush flows of all datapaths while > most of its invocation should only flush its local datapath. > Only megaflow on/off commands should flush all dapapaths. This bug is > also fixed. > > Signed-off-by: Andy Zhou <[email protected]> > --- > ofproto/ofproto-dpif-upcall.c | 27 +++++++++++++++++++++------ > ofproto/ofproto-dpif-upcall.h | 2 +- > ofproto/ofproto-dpif.c | 9 +++++++-- > 3 files changed, 29 insertions(+), 9 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 496593b..cc4982f 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -279,7 +279,7 @@ void > udpif_destroy(struct udpif *udpif) > { > udpif_set_threads(udpif, 0, 0); > - udpif_flush(); > + udpif_flush(udpif); > > list_remove(&udpif->list_node); > latch_destroy(&udpif->exit_latch); > @@ -470,16 +470,31 @@ udpif_get_memory_usage(struct udpif *udpif, struct > simap *usage) > } > } > > -/* Removes all flows from all datapaths. */ > +/* Remove flows from a single datapath. */ > void > -udpif_flush(void) > +udpif_flush(struct udpif *udpif) > +{ > + size_t n_handlers, n_revalidators; > + > + n_handlers = udpif->n_handlers; > + n_revalidators = udpif->n_revalidators; > + > + udpif_set_threads(udpif, 0, 0); > + dpif_flow_flush(udpif->dpif); > + udpif_set_threads(udpif, n_handlers, n_revalidators); > +} > + > +/* Removes all flows from all datapaths. */ > +static void > +udpif_flush_all_datapaths(void) > { > struct udpif *udpif; > > LIST_FOR_EACH (udpif, list_node, &all_udpifs) { > - dpif_flow_flush(udpif->dpif); > + udpif_flush(udpif); > } > } > + > > /* Destroys and deallocates 'upcall'. */ > static void > @@ -1657,7 +1672,7 @@ upcall_unixctl_disable_megaflows(struct unixctl_conn > *conn, > void *aux OVS_UNUSED) > { > atomic_store(&enable_megaflows, false); > - udpif_flush(); > + udpif_flush_all_datapaths(); > unixctl_command_reply(conn, "megaflows disabled"); > } > > @@ -1672,7 +1687,7 @@ upcall_unixctl_enable_megaflows(struct unixctl_conn > *conn, > void *aux OVS_UNUSED) > { > atomic_store(&enable_megaflows, true); > - udpif_flush(); > + udpif_flush_all_datapaths(); > unixctl_command_reply(conn, "megaflows enabled"); > } > > diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h > index 9eeee5b..6846f87 100644 > --- a/ofproto/ofproto-dpif-upcall.h > +++ b/ofproto/ofproto-dpif-upcall.h > @@ -34,6 +34,6 @@ void udpif_destroy(struct udpif *); > void udpif_revalidate(struct udpif *); > void udpif_get_memory_usage(struct udpif *, struct simap *usage); > struct seq *udpif_dump_seq(struct udpif *); > -void udpif_flush(void); > +void udpif_flush(struct udpif *); > > #endif /* ofproto-dpif-upcall.h */ > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 97eb2b8..bb414f2 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -1371,9 +1371,14 @@ type_get_memory_usage(const char *type, struct simap > *usage) > } > > static void > -flush(struct ofproto *ofproto OVS_UNUSED) > +flush(struct ofproto *ofproto_) > { > - udpif_flush(); > + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); > + struct dpif_backer *backer = ofproto->backer; > + > + if (backer) { > + udpif_flush(backer->udpif); > + } > } > > static void > -- > 1.7.9.5 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
