Message ID | 20200109204944.4092346-1-blp@ovn.org |
---|---|
State | Accepted |
Commit | 586cd3101e7fda54d14fb5bf12d847f35d968627 |
Headers | show |
Series | [ovs-dev,repost,1/2] ofproto-dpif-upcall: Get rid of udpif_synchronize(). | expand |
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 >
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,
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(-)