Thanks for the review. Pushed to the master.
On Sat, Mar 15, 2014 at 11:37 AM, Jarno Rajahalme <jrajaha...@nicira.com>wrote:
> Looks good to me :-)
>
> Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
>
> > On Mar 13, 2014, at 10:20 PM, Andy Zhou <az...@nicira.com> 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 <az...@nicira.com>
> > ---
> > 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
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
