Re: LTTng "TIF_KERNEL_TRACE"

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Tue, Apr 28, 2009 at 11:40:46AM -0400, Mathieu Desnoyers wrote:
> > * Ingo Molnar (mingo@elte.hu) wrote:
> > > 
> > > * Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> > > 
> > > > Hi Ingo,
> > > > 
> > > > Looking at the current -tip tree, I notice that the 
> > > > TIF_SYSCALL_FTRACE flag is only implemented for x86.
> > > > 
> > > > I have TIF_KERNEL_TRACE in my lttng tree which applies to all 
> > > > architectures to do the exact same thing :
> > > > 
> > > > lttng-kernel-trace-thread-flag-alpha.patch
> > > > lttng-kernel-trace-thread-flag-arm.patch
> > > > lttng-kernel-trace-thread-flag-avr32.patch
> > > > lttng-kernel-trace-thread-flag-blackfin.patch
> > > > lttng-kernel-trace-thread-flag-cris.patch
> > > > lttng-kernel-trace-thread-flag-frv.patch
> > > > lttng-kernel-trace-thread-flag-h8300.patch
> > > > lttng-kernel-trace-thread-flag-ia64.patch
> > > > lttng-kernel-trace-thread-flag-m32r.patch
> > > > lttng-kernel-trace-thread-flag-m68k.patch
> > > > lttng-kernel-trace-thread-flag-mips.patch
> > > > lttng-kernel-trace-thread-flag-parisc.patch
> > > > lttng-kernel-trace-thread-flag-powerpc.patch
> > > > lttng-kernel-trace-thread-flag-s390.patch
> > > > lttng-kernel-trace-thread-flag-sh.patch
> > > > lttng-kernel-trace-thread-flag-sparc.patch
> > > > lttng-kernel-trace-thread-flag-um.patch
> > > > lttng-kernel-trace-thread-flag-x86.patch
> > > > lttng-kernel-trace-thread-flag-xtensa.patch
> > > > lttng-kernel-trace-thread-flag-api.patch
> > > > 
> > > > Is there any way we could get this merged ?
> > > > 
> > > > One thing I like about the name TIF_KERNEL_TRACE compared to 
> > > > TIF_SYSCALL_FTRACE is that it gives us a per-thread flag that 
> > > > could eventually be used for more kernel tracing purposes than 
> > > > just syscalls.
> > > 
> > > Yeah - TIF_KERNEL_TRACE indeed sounds more descriptive and less 
> > > restrictive. TIF_SYSCALL_FTRACE was a bit ad-hoc.
> > > 
> > 
> > Second question :
> > 
> > LTTng :
> >         read_lock(&tasklist_lock);
> >         do_each_thread(p, t) {
> >                 set_tsk_thread_flag(t, TIF_KERNEL_TRACE);
> >         } while_each_thread(p, t);
> >         read_unlock(&tasklist_lock);
> > 
> > Ftrace:
> >         read_lock_irqsave(&tasklist_lock, flags);
> > 
> >         do_each_thread(g, t) {
> >                 clear_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
> >         } while_each_thread(g, t);
> > 
> >         read_unlock_irqrestore(&tasklist_lock, flags);
> > 
> > With or without irqsave ?
> > 
> > Arguments against irqsave for this read lock :
> > 
> > - it's not used consistently for this read lock all over the kernel.
> >   Sometimes the read lock is taken without irqsave.
> > - it can be a long iteration, and therefore disables interrupts for a
> >   long time.
> > 
> > Arguments for irqsave for this read lock :
> > 
> > - Taking any kind of spin/rwlock with inconsistent irq disabling leads
> >   to races where interrupts can be disabled for an unbounded amount of
> >   time if a spinlock with irqoff waits on a spinlock with irqs on. This
> >   is a general problem with current kernel rwlock usage. See my
> >   "priority sifting reader-writer lock" patchset for a fix to this
> >   problem.
> > 
> > Mathieu
> > 
> 
> 
> I don't know why I used irqsave here, I guess I was tired.
> 
> $ git-grep "read_lock_irqsave(&tasklist_lock)" | wc -l
> 0
> $ git-grep "write_lock_irqsave(&tasklist_lock)" | wc -l
> 0
> 
> It is never used in an irq safe fashion, unless one of these sites
> has the irqs disabled.
> Lockdep should even have complained about this, when you hold
> a lock class in an irq safe fashion and thereafter you try to hold
> it in an irq unsafe fashion, then the state of the kernel becomes unsafe
> and lockdep is supposed to complain about that.

No, read-write lock is a "special case" where it does not deadlock if
you have an interrupt handler taking the read lock over another read
lock. It's just the write lock that _must absolutely_ disable
interrupts.

However, the "latency race" scenario I explained above applies here,
because the write lock disables interrupts and the read locks doesn't.

Mathieu

> 
> Frederic.
> 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68