* LTTng "TIF_KERNEL_TRACE" @ 2009-04-28 13:20 Mathieu Desnoyers 2009-04-28 15:01 ` Ingo Molnar 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Desnoyers @ 2009-04-28 13:20 UTC (permalink / raw) To: Ingo Molnar; +Cc: linux-kernel 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. Mathieu -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: LTTng "TIF_KERNEL_TRACE" 2009-04-28 13:20 LTTng "TIF_KERNEL_TRACE" Mathieu Desnoyers @ 2009-04-28 15:01 ` Ingo Molnar 2009-04-28 15:21 ` Mathieu Desnoyers 2009-04-28 15:40 ` Mathieu Desnoyers 0 siblings, 2 replies; 11+ messages in thread From: Ingo Molnar @ 2009-04-28 15:01 UTC (permalink / raw) To: Mathieu Desnoyers, Frédéric Weisbecker, Steven Rostedt, Li Zefan Cc: linux-kernel * 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. Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: LTTng "TIF_KERNEL_TRACE" 2009-04-28 15:01 ` Ingo Molnar @ 2009-04-28 15:21 ` Mathieu Desnoyers 2009-04-28 15:40 ` Mathieu Desnoyers 1 sibling, 0 replies; 11+ messages in thread From: Mathieu Desnoyers @ 2009-04-28 15:21 UTC (permalink / raw) To: Ingo Molnar Cc: Frédéric Weisbecker, Steven Rostedt, Li Zefan, linux-kernel * 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. > > Ingo lttng-kernel-trace-thread-flag-api.patch adds the enable/disable API, with refcount, in kernel/sched.c just because I did not know where to put it. Should we move this to its own file in kernel/trace/???.c ? One option is kernel/trace/trace.c, which is compiled as soon as CONFIG_TRACING is enabled. I added the prototypes to linux/sched.h, they should probably be moved to... linux/trace.h ? Mathieu -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: LTTng "TIF_KERNEL_TRACE" 2009-04-28 15:01 ` Ingo Molnar 2009-04-28 15:21 ` Mathieu Desnoyers @ 2009-04-28 15:40 ` Mathieu Desnoyers 2009-04-28 16:33 ` Frederic Weisbecker 1 sibling, 1 reply; 11+ messages in thread From: Mathieu Desnoyers @ 2009-04-28 15:40 UTC (permalink / raw) To: Ingo Molnar Cc: Frédéric Weisbecker, Steven Rostedt, Li Zefan, linux-kernel * 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 > Ingo -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: LTTng "TIF_KERNEL_TRACE" 2009-04-28 15:40 ` Mathieu Desnoyers @ 2009-04-28 16:33 ` Frederic Weisbecker 2009-04-28 16:37 ` Frédéric Weisbecker ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Frederic Weisbecker @ 2009-04-28 16:33 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: Ingo Molnar, Steven Rostedt, Li Zefan, linux-kernel 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: LTTng "TIF_KERNEL_TRACE" 2009-04-28 16:33 ` Frederic Weisbecker @ 2009-04-28 16:37 ` Frédéric Weisbecker 2009-04-28 16:38 ` Mathieu Desnoyers 2009-04-28 16:43 ` Steven Rostedt 2 siblings, 0 replies; 11+ messages in thread From: Frédéric Weisbecker @ 2009-04-28 16:37 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: Ingo Molnar, Steven Rostedt, Li Zefan, linux-kernel 2009/4/28 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. I mean, it's usually a proof of an unsafe state. Not here because we actually never hold it from irq context. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: LTTng "TIF_KERNEL_TRACE" 2009-04-28 16:33 ` Frederic Weisbecker 2009-04-28 16:37 ` Frédéric Weisbecker @ 2009-04-28 16:38 ` Mathieu Desnoyers 2009-04-28 16:44 ` Frederic Weisbecker 2009-04-28 16:43 ` Steven Rostedt 2 siblings, 1 reply; 11+ messages in thread From: Mathieu Desnoyers @ 2009-04-28 16:38 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: Ingo Molnar, Steven Rostedt, Li Zefan, linux-kernel * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: LTTng "TIF_KERNEL_TRACE" 2009-04-28 16:38 ` Mathieu Desnoyers @ 2009-04-28 16:44 ` Frederic Weisbecker 2009-04-28 16:52 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Frederic Weisbecker @ 2009-04-28 16:44 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: Ingo Molnar, Steven Rostedt, Li Zefan, linux-kernel On Tue, Apr 28, 2009 at 12:38:25PM -0400, Mathieu Desnoyers wrote: > 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. Ah, you're right, I was thinking with spinlock rules in mind :) > However, the "latency race" scenario I explained above applies here, > because the write lock disables interrupts and the read locks doesn't. > > Mathieu > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: LTTng "TIF_KERNEL_TRACE" 2009-04-28 16:44 ` Frederic Weisbecker @ 2009-04-28 16:52 ` Steven Rostedt 0 siblings, 0 replies; 11+ messages in thread From: Steven Rostedt @ 2009-04-28 16:52 UTC (permalink / raw) To: Frederic Weisbecker Cc: Mathieu Desnoyers, Ingo Molnar, Li Zefan, linux-kernel On Tue, 28 Apr 2009, Frederic Weisbecker wrote: > On Tue, Apr 28, 2009 at 12:38:25PM -0400, Mathieu Desnoyers wrote: > > 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. > > > Ah, you're right, I was thinking with spinlock rules in mind :) > The only time you would want to do a read_lock_irqsave is if the write_lock is taken in irq context. But I do not know of any lock where that is the case. -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: LTTng "TIF_KERNEL_TRACE" 2009-04-28 16:33 ` Frederic Weisbecker 2009-04-28 16:37 ` Frédéric Weisbecker 2009-04-28 16:38 ` Mathieu Desnoyers @ 2009-04-28 16:43 ` Steven Rostedt 2009-04-28 16:48 ` Frederic Weisbecker 2 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2009-04-28 16:43 UTC (permalink / raw) To: Frederic Weisbecker Cc: Mathieu Desnoyers, Ingo Molnar, Li Zefan, linux-kernel On Tue, 28 Apr 2009, Frederic Weisbecker wrote: > > I don't know why I used irqsave here, I guess I was tired. It was the "when in doubt use irqsave", or was it the "I don't want to think about it, use irqsave" ;-) -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: LTTng "TIF_KERNEL_TRACE" 2009-04-28 16:43 ` Steven Rostedt @ 2009-04-28 16:48 ` Frederic Weisbecker 0 siblings, 0 replies; 11+ messages in thread From: Frederic Weisbecker @ 2009-04-28 16:48 UTC (permalink / raw) To: Steven Rostedt; +Cc: Mathieu Desnoyers, Ingo Molnar, Li Zefan, linux-kernel On Tue, Apr 28, 2009 at 12:43:25PM -0400, Steven Rostedt wrote: > > On Tue, 28 Apr 2009, Frederic Weisbecker wrote: > > > > I don't know why I used irqsave here, I guess I was tired. > > It was the "when in doubt use irqsave", or was it the "I don't want to > think about it, use irqsave" ;-) A bit of the former and the latter... It was "Argh, it's too late, I should have gone to my bed several hours ago, just use irqsave..." :-/ > -- Steve > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-04-28 16:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-04-28 13:20 LTTng "TIF_KERNEL_TRACE" Mathieu Desnoyers 2009-04-28 15:01 ` Ingo Molnar 2009-04-28 15:21 ` Mathieu Desnoyers 2009-04-28 15:40 ` Mathieu Desnoyers 2009-04-28 16:33 ` Frederic Weisbecker 2009-04-28 16:37 ` Frédéric Weisbecker 2009-04-28 16:38 ` Mathieu Desnoyers 2009-04-28 16:44 ` Frederic Weisbecker 2009-04-28 16:52 ` Steven Rostedt 2009-04-28 16:43 ` Steven Rostedt 2009-04-28 16:48 ` Frederic Weisbecker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox