Re: LTTng "TIF_KERNEL_TRACE"

[Frederic Weisbecker <fweisbec@gmail.com>]

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.

Frederic.