[ovs-dev,repost,1/2] ofproto-dpif-upcall: Get rid of udpif_synchronize().

RCU provides the semantics we want from udpif_synchronize() and it
should be much more lightweight than killing and restarting all the
upcall threads.  It looks like udpif_synchronize() was written before
the OVS tree had RCU support, which is probably why we didn't use it
here from the beginning.  So we can just change udpif_synchronize()
to a single ovsrcu_synchronize() call.

However, udpif_synchronize() only has a single caller, which calls
ovsrcu_synchronize() anyway just beforehand, via xlate_txn_commit().
So we can get rid of udpif_synchronize() entirely, which this patch
does.

As a side effect, this eliminates one reason why terminating OVS cleanly
clears the datapath flow table.  An upcoming patch will eliminate
other reasons.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ofproto/ofproto-dpif-upcall.c | 17 -----------------
 ofproto/ofproto-dpif-upcall.h |  1 -
 ofproto/ofproto-dpif-xlate.c  | 14 +++++++++-----
 ofproto/ofproto-dpif.c        |  4 ----
 4 files changed, 9 insertions(+), 27 deletions(-)

On Fri, Jan 10, 2020 at 2:20 AM Ben Pfaff <blp@ovn.org> wrote:
>
> RCU provides the semantics we want from udpif_synchronize() and it
> should be much more lightweight than killing and restarting all the
> upcall threads.  It looks like udpif_synchronize() was written before
> the OVS tree had RCU support, which is probably why we didn't use it
> here from the beginning.  So we can just change udpif_synchronize()
> to a single ovsrcu_synchronize() call.
>
> However, udpif_synchronize() only has a single caller, which calls
> ovsrcu_synchronize() anyway just beforehand, via xlate_txn_commit().
> So we can get rid of udpif_synchronize() entirely, which this patch
> does.
>
> As a side effect, this eliminates one reason why terminating OVS cleanly
> clears the datapath flow table.  An upcoming patch will eliminate
> other reasons.
>
> Signed-off-by: Ben Pfaff <blp@ovn.org>


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

Numan

> ---
>  ofproto/ofproto-dpif-upcall.c | 17 -----------------
>  ofproto/ofproto-dpif-upcall.h |  1 -
>  ofproto/ofproto-dpif-xlate.c  | 14 +++++++++-----
>  ofproto/ofproto-dpif.c        |  4 ----
>  4 files changed, 9 insertions(+), 27 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 409286ab1587..03cb8a1dd486 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -644,23 +644,6 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers_,
>      }
>  }
>
> -/* Waits for all ongoing upcall translations to complete.  This ensures that
> - * there are no transient references to any removed ofprotos (or other
> - * objects).  In particular, this should be called after an ofproto is removed
> - * (e.g. via xlate_remove_ofproto()) but before it is destroyed. */
> -void
> -udpif_synchronize(struct udpif *udpif)
> -{
> -    /* This is stronger than necessary.  It would be sufficient to ensure
> -     * (somehow) that each handler and revalidator thread had passed through
> -     * its main loop once. */
> -    size_t n_handlers_ = udpif->n_handlers;
> -    size_t n_revalidators_ = udpif->n_revalidators;
> -
> -    udpif_stop_threads(udpif);
> -    udpif_start_threads(udpif, n_handlers_, n_revalidators_);
> -}
> -
>  /* Notifies 'udpif' that something changed which may render previous
>   * xlate_actions() results invalid. */
>  void
> diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h
> index cef1d34198d6..693107ae56c1 100644
> --- a/ofproto/ofproto-dpif-upcall.h
> +++ b/ofproto/ofproto-dpif-upcall.h
> @@ -33,7 +33,6 @@ struct udpif *udpif_create(struct dpif_backer *, struct dpif *);
>  void udpif_run(struct udpif *udpif);
>  void udpif_set_threads(struct udpif *, size_t n_handlers,
>                         size_t n_revalidators);
> -void udpif_synchronize(struct udpif *);
>  void udpif_destroy(struct udpif *);
>  void udpif_revalidate(struct udpif *);
>  void udpif_get_memory_usage(struct udpif *, struct simap *usage);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 4407f9c97a9e..0b45ecf3d4da 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1171,11 +1171,15 @@ xlate_xport_copy(struct xbridge *xbridge, struct xbundle *xbundle,
>   *
>   * A sample workflow:
>   *
> - * xlate_txn_start();
> - * ...
> - * edit_xlate_configuration();
> - * ...
> - * xlate_txn_commit(); */
> + *     xlate_txn_start();
> + *     ...
> + *     edit_xlate_configuration();
> + *     ...
> + *     xlate_txn_commit();
> + *
> + * The ovsrcu_synchronize() call here also ensures that the upcall threads
> + * retain no references to anything in the previous configuration.
> + */
>  void
>  xlate_txn_commit(void)
>  {
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index d3cb392077df..0222ec82f00a 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1751,10 +1751,6 @@ destruct(struct ofproto *ofproto_, bool del)
>      xlate_remove_ofproto(ofproto);
>      xlate_txn_commit();
>
> -    /* Ensure that the upcall processing threads have no remaining references
> -     * to the ofproto or anything in it. */
> -    udpif_synchronize(ofproto->backer->udpif);
> -
>      hmap_remove(&all_ofproto_dpifs_by_name,
>                  &ofproto->all_ofproto_dpifs_by_name_node);
>      hmap_remove(&all_ofproto_dpifs_by_uuid,
> --
> 2.23.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>