[ovs-dev] [PATCH repost 2/2] ofproto: Do not delete datapath flows on exit by default.

[Numan Siddique]

On Fri, Jan 10, 2020 at 2:20 AM Ben Pfaff <blp at ovn.org> wrote:
>
> Commit e96a5c24e853 ("upcall: Remove datapath flows when setting
> n-threads.") caused OVS to delete datapath flows when it exits through
> any graceful means.  This is not necessarily desirable, especially when
> OVS is being stopped as part of an upgrade.  This commit changes OVS so
> that it only removes datapath flows when requested, via "ovs-appctl
> exit --cleanup".
>
> I've only tested this in the OVS sandbox.
>
> Signed-off-by: Ben Pfaff <blp at ovn.org>

Acked-by: Numan Siddique <numans at ovn.org>

Tested with the kernel datapath and it works as expected.

Thanks
Numan

> ---
>  ofproto/ofproto-dpif-upcall.c | 26 ++++++++++++++++----------
>  ofproto/ofproto.c             |  8 ++++----
>  2 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 03cb8a1dd486..8dfa05b71df4 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -332,7 +332,7 @@ static size_t recv_upcalls(struct handler *);
>  static int process_upcall(struct udpif *, struct upcall *,
>                            struct ofpbuf *odp_actions, struct flow_wildcards *);
>  static void handle_upcalls(struct udpif *, struct upcall *, size_t n_upcalls);
> -static void udpif_stop_threads(struct udpif *);
> +static void udpif_stop_threads(struct udpif *, bool delete_flows);
>  static void udpif_start_threads(struct udpif *, size_t n_handlers,
>                                  size_t n_revalidators);
>  static void udpif_pause_revalidators(struct udpif *);
> @@ -483,7 +483,7 @@ udpif_run(struct udpif *udpif)
>  void
>  udpif_destroy(struct udpif *udpif)
>  {
> -    udpif_stop_threads(udpif);
> +    udpif_stop_threads(udpif, false);
>
>      dpif_register_dp_purge_cb(udpif->dpif, NULL, udpif);
>      dpif_register_upcall_cb(udpif->dpif, NULL, udpif);
> @@ -504,9 +504,15 @@ udpif_destroy(struct udpif *udpif)
>      free(udpif);
>  }
>
> -/* Stops the handler and revalidator threads. */
> +/* Stops the handler and revalidator threads.
> + *
> + * If 'delete_flows' is true, we delete ukeys and delete all flows from the
> + * datapath.  Otherwise, we end up double-counting stats for flows that remain
> + * in the datapath.  If 'delete_flows' is false, we skip this step.  This is
> + * appropriate if OVS is about to exit anyway and it is desirable to let
> + * existing network connections continue being forwarded afterward. */
>  static void
> -udpif_stop_threads(struct udpif *udpif)
> +udpif_stop_threads(struct udpif *udpif, bool delete_flows)
>  {
>      if (udpif && (udpif->n_handlers != 0 || udpif->n_revalidators != 0)) {
>          size_t i;
> @@ -526,10 +532,10 @@ udpif_stop_threads(struct udpif *udpif)
>          dpif_disable_upcall(udpif->dpif);
>          ovsrcu_quiesce_end();
>
> -        /* Delete ukeys, and delete all flows from the datapath to prevent
> -         * double-counting stats. */
> -        for (i = 0; i < udpif->n_revalidators; i++) {
> -            revalidator_purge(&udpif->revalidators[i]);
> +        if (delete_flows) {
> +            for (i = 0; i < udpif->n_revalidators; i++) {
> +                revalidator_purge(&udpif->revalidators[i]);
> +            }
>          }
>
>          latch_poll(&udpif->exit_latch);
> @@ -627,7 +633,7 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers_,
>
>      if (udpif->n_handlers != n_handlers_
>          || udpif->n_revalidators != n_revalidators_) {
> -        udpif_stop_threads(udpif);
> +        udpif_stop_threads(udpif, true);
>      }
>
>      if (!udpif->handlers && !udpif->revalidators) {
> @@ -681,7 +687,7 @@ udpif_flush(struct udpif *udpif)
>      size_t n_handlers_ = udpif->n_handlers;
>      size_t n_revalidators_ = udpif->n_revalidators;
>
> -    udpif_stop_threads(udpif);
> +    udpif_stop_threads(udpif, true);
>      dpif_flow_flush(udpif->dpif);
>      udpif_start_threads(udpif, n_handlers_, n_revalidators_);
>  }
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 08830d837164..5d69a4332f9f 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1601,13 +1601,13 @@ ofproto_rule_delete(struct ofproto *ofproto, struct rule *rule)
>  }
>
>  static void
> -ofproto_flush__(struct ofproto *ofproto)
> +ofproto_flush__(struct ofproto *ofproto, bool del)
>      OVS_EXCLUDED(ofproto_mutex)
>  {
>      struct oftable *table;
>
>      /* This will flush all datapath flows. */
> -    if (ofproto->ofproto_class->flush) {
> +    if (del && ofproto->ofproto_class->flush) {
>          ofproto->ofproto_class->flush(ofproto);
>      }
>
> @@ -1710,7 +1710,7 @@ ofproto_destroy(struct ofproto *p, bool del)
>          return;
>      }
>
> -    ofproto_flush__(p);
> +    ofproto_flush__(p, del);
>      HMAP_FOR_EACH_SAFE (ofport, next_ofport, hmap_node, &p->ports) {
>          ofport_destroy(ofport, del);
>      }
> @@ -2288,7 +2288,7 @@ void
>  ofproto_flush_flows(struct ofproto *ofproto)
>  {
>      COVERAGE_INC(ofproto_flush);
> -    ofproto_flush__(ofproto);
> +    ofproto_flush__(ofproto, false);
>      connmgr_flushed(ofproto->connmgr);
>  }
>
> --
> 2.23.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>