Messages in this thread | | | From | Joel Fernandes <> | Date | Wed, 8 Aug 2018 15:15:31 -0700 | Subject | Re: [PATCH v12 3/3] tracing: Centralize preemptirq tracepoints and unify their usage |
| |
On Wed, Aug 8, 2018 at 1:18 PM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> 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. :-)
> >> 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
| |