Re: [PATCH v12 3/3] tracing: Centralize preemptirq tracepoints and unify their usage

[Paul E. McKenney]

On Wed, Aug 08, 2018 at 03:15:31PM -0700, Joel Fernandes wrote:
> On Wed, Aug 8, 2018 at 1:18 PM, Paul E. McKenney
> <paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> [...]
> >> >> >> It does start to seem like a show stopper :-(
> >> >> >
> >> >> > I suppose that an srcu_read_lock_nmi() and srcu_read_unlock_nmi() could
> >> >> > be added, which would do atomic ops on sp->sda->srcu_lock_count.  Not sure
> >> >> > whether this would be fast enough to be useful, but easy to provide:
> >> >> >
> >> >> > int __srcu_read_lock_nmi(struct srcu_struct *sp)  /* UNTESTED. */
> >> >> > {
> >> >> >         int idx;
> >> >> >
> >> >> >         idx = READ_ONCE(sp->srcu_idx) & 0x1;
> >> >> >         atomic_inc(&sp->sda->srcu_lock_count[idx]);
> >> >> >         smp_mb__after_atomic(); /* B */  /* Avoid leaking critical section. */
> >> >> >         return idx;
> >> >> > }
> >> >> >
> >> >> > void __srcu_read_unlock_nmi(struct srcu_struct *sp, int idx)
> >> >> > {
> >> >> >         smp_mb__before_atomic(); /* C */  /* Avoid leaking critical section. */
> >> >> >         atomic_inc(&sp->sda->srcu_unlock_count[idx]);
> >> >> > }
> >> >> >
> >> >> > With appropriate adjustments to also allow Tiny RCU to also work.
> >> >> >
> >> >> > Note that you have to use _nmi() everywhere, not just in NMI handlers.
> >> >> > In fact, the NMI handlers are the one place you -don't- need to use
> >> >> > _nmi(), strangely enough.
> >> >> >
> >> >> > Might be worth a try -- smp_mb__{before,after}_atomic() is a no-op on
> >> >> > some architectures, for example.
> >> >>
> >> >> Continuing Steve's question on regular interrupts, do we need to use
> >> >> this atomic_inc API for regular interrupts as well? So I guess
> >> >
> >> > If NMIs use one srcu_struct and non-NMI uses another, the current
> >> > srcu_read_lock() and srcu_read_unlock() will work just fine.  If any given
> >> > srcu_struct needs both NMI and non-NMI readers, then we really do need
> >> > __srcu_read_lock_nmi() and __srcu_read_unlock_nmi() for that srcu_struct.
> >>
> >> Yes, I believe as long as in_nmi() works reliably, we can use the
> >> right srcu_struct (NMI vs non-NMI) and it would be fine.
> >>
> >> Going through this thread, it sounds though that this_cpu_inc may not
> >> be reliable on all architectures even for non-NMI interrupts and
> >> local_inc may be the way to go.
> >
> > My understanding is that this_cpu_inc() is defined to handle interrupts,
> > so any architecture on which it is unreliable needs to fix its bug.  ;-)
> 
> Yes that's my understanding as well.
> 
> Then may be I'm missing something about yours/Steve's conversations in
> the morning, about why we need bother with the local_inc then. So the
> current SRCU code with the separate NMI handle should work fine (for
> future merge windows) as long as we're using a separate srcu_struct
> for NMI. :-)

I do believe that to be true.  But only as long as that separate
srcu_struct is -only- used for NMI.

If this is what you have been pushing for all along, please accept my
apologies for my being slow!

That said, your approach does require you to have a perfect way to
distinguish between NMI and not-NMI.  If the distinguishing is even
in the slightest imperfect, then some sort of NMI-safe SRCU reader
approach is of course required.

							Thanx, Paul

> >> For next merge window (not this one), lets do that then? Paul, if you
> >> could provide me an SRCU API that uses local_inc, then I believe that
> >> coupled with this patch should be all that's needed:
> >> https://lore.kernel.org/patchwork/patch/972657/
> >>
> >> Steve did express concern though if in_nmi() works reliably (i.e.
> >> tracepoint doesn't fire from "thunk" code before in_nmi() is
> >> available). Any thoughts on that Steve?
> >
> > Agreed, not the upcoming merge window.  But we do need to work out
> > exactly what is the right way to do this.
> 
> Agreed, thanks!
> 
>  - Joel
>