[ovs-dev] [PATCH 1/2] ofproto-dpif-upcall: Don't purge ukeys while in a quiescent state.

[Ben Pfaff]

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 <joe at ovn.org>
> >> Fixes: 9fce0584a643 ("revalidator: Use 'cmap' for storing ukeys.")
> >> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
> > 
> > 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/