[ovs-dev] [PATCH 1/2] ofproto-dpif-upcall: Don't purge ukeys while in a quiescent state.
Ben Pfaff
blp at ovn.org
Fri Nov 2 18:27:57 UTC 2018
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/
More information about the dev
mailing list