On Fri, Nov 02, 2018 at 11:24:30AM +0300, Ilya Maximets wrote: > On 01.11.2018 21:29, Ben Pfaff wrote: > > On Thu, Nov 01, 2018 at 04:58:40PM +0300, Ilya Maximets wrote: > >> revalidator_purge() iterates and modifies umap->cmap. This should > >> not happen in quiescent state, because cmap implementation based > >> on rcu protected variables. Let's move the thread to active state > >> before purging to avoid possible wrong memory accesses. > >> > >> CC: Joe Stringer <[email protected]> > >> Fixes: 9fce0584a643 ("revalidator: Use 'cmap' for storing ukeys.") > >> Signed-off-by: Ilya Maximets <[email protected]> > > > > Thank you for catching the problem, and for the fix. This is quite > > subtle. > > > > Looking at the code here, and the history, it's not at all clear to me > > why there needs to be such a wide window of RCU quiescing. Usually, RCU > > quiescing is to avoid delaying the RCU freeing thread across an > > operation that can block for a considerable amount of time. Across > > udpif_stop_threads() and udpif_start_threads(), I only see a few > > operations that can do that, in particular the xpthread_join() calls. > > dpif_disable_upcall() could also arguably be slow for dpif-netdev since > > it's capturing a rwlock for writing. > > > > So, I'd be inclined to fix this by narrowing the quiescent window, > > something like the following. Do you see flaws in my analysis? > > I agree with you. One thing is that pthread_create() also could > consume valuable amount of time on some systems. Up to few hundreds > of milliseconds. This could be a problem in case of big number of > revalidators. So, I'd like to have following incremental: > > --- > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 57869cb32..e44339338 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -565,6 +565,8 @@ udpif_start_threads(struct udpif *udpif, size_t > n_handlers_, > size_t n_revalidators_) > { > if (udpif && n_handlers_ && n_revalidators_) { > + ovsrcu_quiesce_start(); > + > udpif->n_handlers = n_handlers_; > udpif->n_revalidators = n_revalidators_; > > @@ -595,6 +597,7 @@ udpif_start_threads(struct udpif *udpif, size_t > n_handlers_, > revalidator->thread = ovs_thread_create( > "revalidator", udpif_revalidator, revalidator); > } > + ovsrcu_quiesce_end(); > } > } > > --- > > What do you think? > > Will you format a patch?
Thanks, I applied all your comments and sent v2: https://patchwork.ozlabs.org/patch/992499/ _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
