On Fri, Jan 10, 2020 at 2:20 AM Ben Pfaff <[email protected]> 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 <[email protected]>
Acked-by: Numan Siddique <[email protected]> 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
