Re: LTTng "TIF_KERNEL_TRACE"

Mathieu Desnoyers
2009-04-28T16:43:56Z

* 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