[ovs-dev] [PATCH] udpif: Bug fix updif_flush

[Jarno Rajahalme]

Looks good to me :-)

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>


> On Mar 13, 2014, at 10:20 PM, Andy Zhou <azhou at 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 <azhou at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev