* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore [not found] ` <1274863724-14906-2-git-send-email-monstr@monstr.eu> @ 2010-05-26 14:42 ` Steven Rostedt 2010-05-26 17:08 ` Andrew Morton 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2010-05-26 14:42 UTC (permalink / raw) To: monstr Cc: linux-kernel, michal.simek, arnd, john.williams, tglx, akpm, peter.fritzsche, anton, mingo On Wed, 2010-05-26 at 10:48 +0200, monstr@monstr.eu wrote: > From: Michal Simek <monstr@monstr.eu> > > start/stop_critical_timing function for preemptirqsoff, preemptoff > and irqsoff tracers contains atomic_inc and atomic_dec operations. > > Atomic operations used local_irq_save/restore macros to ensure > atomic access but they are traced by the same function which is causing > recursion problem. > > The reason is when these tracers are turn ON then local_irq_save/restore > macros are changed in include/linux/irqflags.h to call trace_hardirqs_on/off > which call start/stop_critical_timing. > > Microblaze was affected because use generic atomic implementation. > > Signed-off-by: Michal Simek <monstr@monstr.eu> Acked-by: Steven Rostedt <rostedt@goodmis.org> -- Steve > --- > include/asm-generic/atomic.h | 12 ++++++------ > 1 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h > index 058129e..6c190fd 100644 > --- a/include/asm-generic/atomic.h > +++ b/include/asm-generic/atomic.h > @@ -57,11 +57,11 @@ static inline int atomic_add_return(int i, atomic_t *v) > unsigned long flags; > int temp; > > - local_irq_save(flags); > + raw_local_irq_save(flags); > temp = v->counter; > temp += i; > v->counter = temp; > - local_irq_restore(flags); > + raw_local_irq_restore(flags); > > return temp; > } > @@ -78,11 +78,11 @@ static inline int atomic_sub_return(int i, atomic_t *v) > unsigned long flags; > int temp; > > - local_irq_save(flags); > + raw_local_irq_save(flags); > temp = v->counter; > temp -= i; > v->counter = temp; > - local_irq_restore(flags); > + raw_local_irq_restore(flags); > > return temp; > } > @@ -135,9 +135,9 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr) > unsigned long flags; > > mask = ~mask; > - local_irq_save(flags); > + raw_local_irq_save(flags); > *addr &= mask; > - local_irq_restore(flags); > + raw_local_irq_restore(flags); > } > > #define atomic_xchg(ptr, v) (xchg(&(ptr)->counter, (v))) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore 2010-05-26 14:42 ` [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore Steven Rostedt @ 2010-05-26 17:08 ` Andrew Morton 2010-05-26 17:11 ` Steven Rostedt 2010-07-26 8:49 ` Michal Simek 0 siblings, 2 replies; 11+ messages in thread From: Andrew Morton @ 2010-05-26 17:08 UTC (permalink / raw) To: rostedt Cc: monstr, linux-kernel, michal.simek, arnd, john.williams, tglx, peter.fritzsche, anton, mingo On Wed, 26 May 2010 10:42:43 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Wed, 2010-05-26 at 10:48 +0200, monstr@monstr.eu wrote: > > From: Michal Simek <monstr@monstr.eu> > > > > start/stop_critical_timing function for preemptirqsoff, preemptoff > > and irqsoff tracers contains atomic_inc and atomic_dec operations. > > > > Atomic operations used local_irq_save/restore macros to ensure > > atomic access but they are traced by the same function which is causing > > recursion problem. > > > > The reason is when these tracers are turn ON then local_irq_save/restore > > macros are changed in include/linux/irqflags.h to call trace_hardirqs_on/off > > which call start/stop_critical_timing. > > > > Microblaze was affected because use generic atomic implementation. > > > > Signed-off-by: Michal Simek <monstr@monstr.eu> > > Acked-by: Steven Rostedt <rostedt@goodmis.org> > Sighed-at-by: me. > > diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h > > index 058129e..6c190fd 100644 > > --- a/include/asm-generic/atomic.h > > +++ b/include/asm-generic/atomic.h > > @@ -57,11 +57,11 @@ static inline int atomic_add_return(int i, atomic_t *v) > > unsigned long flags; > > int temp; > > > > - local_irq_save(flags); > > + raw_local_irq_save(flags); > > temp = v->counter; > > temp += i; > > v->counter = temp; > > - local_irq_restore(flags); > > + raw_local_irq_restore(flags); > > > > return temp; > > } If a developer looks at atomic_add_return() and asks himself "why did this use raw_local_irq_save()", the only way of answering that question is to go groveling through the git logs, which is a right PITA if you're trying to get some coding work done. Guys, any time you add code which is non-obvious at the raw C level, it *needs* a comment! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore 2010-05-26 17:08 ` Andrew Morton @ 2010-05-26 17:11 ` Steven Rostedt 2010-05-27 7:14 ` Michal Simek 2010-05-27 9:11 ` Yong Zhang 2010-07-26 8:49 ` Michal Simek 1 sibling, 2 replies; 11+ messages in thread From: Steven Rostedt @ 2010-05-26 17:11 UTC (permalink / raw) To: Andrew Morton Cc: monstr, linux-kernel, michal.simek, arnd, john.williams, tglx, peter.fritzsche, anton, mingo On Wed, 2010-05-26 at 10:08 -0700, Andrew Morton wrote: > > If a developer looks at atomic_add_return() and asks himself "why did > this use raw_local_irq_save()", the only way of answering that question > is to go groveling through the git logs, which is a right PITA if > you're trying to get some coding work done. > > Guys, any time you add code which is non-obvious at the raw C level, it > *needs* a comment! Totally agree! I've gotten pretty good at adding comments to changes like this that I do. I need to get better at telling others to comment their work ;-) -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore 2010-05-26 17:11 ` Steven Rostedt @ 2010-05-27 7:14 ` Michal Simek 2010-05-27 9:11 ` Yong Zhang 1 sibling, 0 replies; 11+ messages in thread From: Michal Simek @ 2010-05-27 7:14 UTC (permalink / raw) To: rostedt Cc: Andrew Morton, linux-kernel, michal.simek, arnd, john.williams, tglx, peter.fritzsche, anton, mingo Steven Rostedt wrote: > On Wed, 2010-05-26 at 10:08 -0700, Andrew Morton wrote: > >> If a developer looks at atomic_add_return() and asks himself "why did >> this use raw_local_irq_save()", the only way of answering that question >> is to go groveling through the git logs, which is a right PITA if >> you're trying to get some coding work done. >> >> Guys, any time you add code which is non-obvious at the raw C level, it >> *needs* a comment! > > Totally agree! I've gotten pretty good at adding comments to changes > like this that I do. I need to get better at telling others to comment > their work ;-) ok. Will add any sensible comment. Michal -- Michal Simek, Ing. (M.Eng) w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/ Microblaze U-BOOT custodian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore 2010-05-26 17:11 ` Steven Rostedt 2010-05-27 7:14 ` Michal Simek @ 2010-05-27 9:11 ` Yong Zhang 1 sibling, 0 replies; 11+ messages in thread From: Yong Zhang @ 2010-05-27 9:11 UTC (permalink / raw) To: Steven Rostedt Cc: Andrew Morton, monstr, linux-kernel, michal.simek, arnd, john.williams, tglx, peter.fritzsche, anton, mingo, Frederic Weisbecker On Wed, May 26, 2010 at 01:11:51PM -0400, Steven Rostedt wrote: > On Wed, 2010-05-26 at 10:08 -0700, Andrew Morton wrote: > > > > > If a developer looks at atomic_add_return() and asks himself "why did > > this use raw_local_irq_save()", the only way of answering that question > > is to go groveling through the git logs, which is a right PITA if > > you're trying to get some coding work done. > > > > Guys, any time you add code which is non-obvious at the raw C level, it > > *needs* a comment! > > Totally agree! I've gotten pretty good at adding comments to changes > like this that I do. I need to get better at telling others to comment > their work ;-) Cc'ed Frederic Weisbecker Seems like this issue has been raised before: http://marc.info/?l=linux-kernel&m=126292794806169&w=2 I'm afraid this patch will trigger lockdep warning if atomic_inc() is used in some locks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore 2010-05-26 17:08 ` Andrew Morton 2010-05-26 17:11 ` Steven Rostedt @ 2010-07-26 8:49 ` Michal Simek 2010-07-27 23:18 ` Andrew Morton 1 sibling, 1 reply; 11+ messages in thread From: Michal Simek @ 2010-07-26 8:49 UTC (permalink / raw) To: Andrew Morton; +Cc: rostedt, linux-kernel, tglx, mingo [-- Attachment #1: Type: text/plain, Size: 2163 bytes --] Andrew Morton wrote: > On Wed, 26 May 2010 10:42:43 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > >> On Wed, 2010-05-26 at 10:48 +0200, monstr@monstr.eu wrote: >>> From: Michal Simek <monstr@monstr.eu> >>> >>> start/stop_critical_timing function for preemptirqsoff, preemptoff >>> and irqsoff tracers contains atomic_inc and atomic_dec operations. >>> >>> Atomic operations used local_irq_save/restore macros to ensure >>> atomic access but they are traced by the same function which is causing >>> recursion problem. >>> >>> The reason is when these tracers are turn ON then local_irq_save/restore >>> macros are changed in include/linux/irqflags.h to call trace_hardirqs_on/off >>> which call start/stop_critical_timing. >>> >>> Microblaze was affected because use generic atomic implementation. >>> >>> Signed-off-by: Michal Simek <monstr@monstr.eu> >> Acked-by: Steven Rostedt <rostedt@goodmis.org> >> > > Sighed-at-by: me. > >>> diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h >>> index 058129e..6c190fd 100644 >>> --- a/include/asm-generic/atomic.h >>> +++ b/include/asm-generic/atomic.h >>> @@ -57,11 +57,11 @@ static inline int atomic_add_return(int i, atomic_t *v) >>> unsigned long flags; >>> int temp; >>> >>> - local_irq_save(flags); >>> + raw_local_irq_save(flags); >>> temp = v->counter; >>> temp += i; >>> v->counter = temp; >>> - local_irq_restore(flags); >>> + raw_local_irq_restore(flags); >>> >>> return temp; >>> } > > If a developer looks at atomic_add_return() and asks himself "why did > this use raw_local_irq_save()", the only way of answering that question > is to go groveling through the git logs, which is a right PITA if > you're trying to get some coding work done. > > Guys, any time you add code which is non-obvious at the raw C level, it > *needs* a comment! Andrew: Can you please add this patch to your tree? I have it in microblaze next branch but should go through different tree. Thanks, Michal -- Michal Simek, Ing. (M.Eng) w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/ Microblaze U-BOOT custodian [-- Attachment #2: 0001-asm-generic-Use-raw_local_irq_save-restore-instead.patch --] [-- Type: text/x-patch, Size: 2295 bytes --] >From 06c2447c8939227b4c4d3340fba1c712f03e70ca Mon Sep 17 00:00:00 2001 From: Michal Simek <monstr@monstr.eu> Date: Wed, 26 May 2010 08:57:36 +0200 Subject: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore start/stop_critical_timing function for preemptirqsoff, preemptoff and irqsoff tracers contains atomic_inc and atomic_dec operations. Atomic operations used local_irq_save/restore macros to ensure atomic access but they are traced by the same function which is causing recursion problem. The reason is when these tracers are turn ON then local_irq_save/restore macros are changed in include/linux/irqflags.h to call trace_hardirqs_on/off which call start/stop_critical_timing. Microblaze was affected because use generic atomic implementation. Signed-off-by: Michal Simek <monstr@monstr.eu> Acked-by: Steven Rostedt <rostedt@goodmis.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- include/asm-generic/atomic.h | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h index 058129e..e53347f 100644 --- a/include/asm-generic/atomic.h +++ b/include/asm-generic/atomic.h @@ -57,11 +57,11 @@ static inline int atomic_add_return(int i, atomic_t *v) unsigned long flags; int temp; - local_irq_save(flags); + raw_local_irq_save(flags); /* Don't trace it in a irqsoff handler */ temp = v->counter; temp += i; v->counter = temp; - local_irq_restore(flags); + raw_local_irq_restore(flags); return temp; } @@ -78,11 +78,11 @@ static inline int atomic_sub_return(int i, atomic_t *v) unsigned long flags; int temp; - local_irq_save(flags); + raw_local_irq_save(flags); /* Don't trace it in a irqsoff handler */ temp = v->counter; temp -= i; v->counter = temp; - local_irq_restore(flags); + raw_local_irq_restore(flags); return temp; } @@ -135,9 +135,9 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr) unsigned long flags; mask = ~mask; - local_irq_save(flags); + raw_local_irq_save(flags); /* Don't trace it in a irqsoff handler */ *addr &= mask; - local_irq_restore(flags); + raw_local_irq_restore(flags); } #define atomic_xchg(ptr, v) (xchg(&(ptr)->counter, (v))) -- 1.5.5.6 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore 2010-07-26 8:49 ` Michal Simek @ 2010-07-27 23:18 ` Andrew Morton 2010-07-27 23:54 ` Mike Frysinger 2010-07-28 5:20 ` Michal Simek 0 siblings, 2 replies; 11+ messages in thread From: Andrew Morton @ 2010-07-27 23:18 UTC (permalink / raw) To: monstr; +Cc: rostedt, linux-kernel, tglx, mingo, linux-arch On Mon, 26 Jul 2010 10:49:50 +0200 Michal Simek <monstr@monstr.eu> wrote: > start/stop_critical_timing function for preemptirqsoff, preemptoff > and irqsoff tracers contains atomic_inc and atomic_dec operations. > > Atomic operations used local_irq_save/restore macros to ensure > atomic access but they are traced by the same function which is causing > recursion problem. > > The reason is when these tracers are turn ON then local_irq_save/restore > macros are changed in include/linux/irqflags.h to call trace_hardirqs_on/off > which call start/stop_critical_timing. > > Microblaze was affected because use generic atomic implementation. Seems that this will also affect blackfin, mn10300 and score. I guess they aren't supporting tracing yet? > Signed-off-by: Michal Simek <monstr@monstr.eu> > Acked-by: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> hm, I wonder how my signoff got there. Doesn't matter. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore 2010-07-27 23:18 ` Andrew Morton @ 2010-07-27 23:54 ` Mike Frysinger 2010-07-28 5:23 ` Michal Simek 2010-07-28 5:20 ` Michal Simek 1 sibling, 1 reply; 11+ messages in thread From: Mike Frysinger @ 2010-07-27 23:54 UTC (permalink / raw) To: Andrew Morton; +Cc: monstr, rostedt, linux-kernel, tglx, mingo, linux-arch On Tue, Jul 27, 2010 at 19:18, Andrew Morton wrote: > On Mon, 26 Jul 2010 10:49:50 +0200 Michal Simek wrote: >> start/stop_critical_timing function for preemptirqsoff, preemptoff >> and irqsoff tracers contains atomic_inc and atomic_dec operations. >> >> Atomic operations used local_irq_save/restore macros to ensure >> atomic access but they are traced by the same function which is causing >> recursion problem. >> >> The reason is when these tracers are turn ON then local_irq_save/restore >> macros are changed in include/linux/irqflags.h to call trace_hardirqs_on/off >> which call start/stop_critical_timing. >> >> Microblaze was affected because use generic atomic implementation. > > Seems that this will also affect blackfin, mn10300 and score. I guess > they aren't supporting tracing yet? do you mean TRACE_IRQFLAGS_SUPPORT ? Blackfin should ... my understanding was that arches just needed to implement asm/irqflags.h for it. -mike ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore 2010-07-27 23:54 ` Mike Frysinger @ 2010-07-28 5:23 ` Michal Simek 0 siblings, 0 replies; 11+ messages in thread From: Michal Simek @ 2010-07-28 5:23 UTC (permalink / raw) To: Mike Frysinger Cc: Andrew Morton, rostedt, linux-kernel, tglx, mingo, linux-arch Mike Frysinger wrote: > On Tue, Jul 27, 2010 at 19:18, Andrew Morton wrote: >> On Mon, 26 Jul 2010 10:49:50 +0200 Michal Simek wrote: >>> start/stop_critical_timing function for preemptirqsoff, preemptoff >>> and irqsoff tracers contains atomic_inc and atomic_dec operations. >>> >>> Atomic operations used local_irq_save/restore macros to ensure >>> atomic access but they are traced by the same function which is causing >>> recursion problem. >>> >>> The reason is when these tracers are turn ON then local_irq_save/restore >>> macros are changed in include/linux/irqflags.h to call trace_hardirqs_on/off >>> which call start/stop_critical_timing. >>> >>> Microblaze was affected because use generic atomic implementation. >> Seems that this will also affect blackfin, mn10300 and score. I guess >> they aren't supporting tracing yet? > > do you mean TRACE_IRQFLAGS_SUPPORT ? Blackfin should ... my > understanding was that arches just needed to implement asm/irqflags.h > for it. Blackin uses own include/asm/atomic.h implementation where shouldn't be a problem with irqflags tracer. Look at my second post. Michal -- Michal Simek, Ing. (M.Eng) w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/ Microblaze U-BOOT custodian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore 2010-07-27 23:18 ` Andrew Morton 2010-07-27 23:54 ` Mike Frysinger @ 2010-07-28 5:20 ` Michal Simek 2010-07-28 5:35 ` Mike Frysinger 1 sibling, 1 reply; 11+ messages in thread From: Michal Simek @ 2010-07-28 5:20 UTC (permalink / raw) To: Andrew Morton; +Cc: rostedt, linux-kernel, tglx, mingo, linux-arch Andrew Morton wrote: > On Mon, 26 Jul 2010 10:49:50 +0200 > Michal Simek <monstr@monstr.eu> wrote: > >> start/stop_critical_timing function for preemptirqsoff, preemptoff >> and irqsoff tracers contains atomic_inc and atomic_dec operations. >> >> Atomic operations used local_irq_save/restore macros to ensure >> atomic access but they are traced by the same function which is causing >> recursion problem. >> >> The reason is when these tracers are turn ON then local_irq_save/restore >> macros are changed in include/linux/irqflags.h to call trace_hardirqs_on/off >> which call start/stop_critical_timing. >> >> Microblaze was affected because use generic atomic implementation. > > Seems that this will also affect blackfin, mn10300 and score. I guess > they aren't supporting tracing yet? If they uses asm-generic/atomic.h, then yes. It seems to me that Blackfin uses own asm/atomic.h. nm10300 and score are the same case as Microblaze. In include/linux/irqflags.h is written this commentary. /* * The local_irq_*() APIs are equal to the raw_local_irq*() * if !TRACE_IRQFLAGS. */ If architecture doesn't enable TRACE_IRQFLAGS then there is no difference in behavior. If yes and use asm-generic/atomic.h code, then IRQFLAG tracer freeze because it is traced part of irqsoff tracer because of recursion as is describe in patch description. > >> Signed-off-by: Michal Simek <monstr@monstr.eu> >> Acked-by: Steven Rostedt <rostedt@goodmis.org> >> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > > hm, I wonder how my signoff got there. Doesn't matter. http://lkml.org/lkml/2010/5/26/364 Michal -- Michal Simek, Ing. (M.Eng) w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/ Microblaze U-BOOT custodian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore 2010-07-28 5:20 ` Michal Simek @ 2010-07-28 5:35 ` Mike Frysinger 0 siblings, 0 replies; 11+ messages in thread From: Mike Frysinger @ 2010-07-28 5:35 UTC (permalink / raw) To: monstr; +Cc: Andrew Morton, rostedt, linux-kernel, tglx, mingo, linux-arch On Wed, Jul 28, 2010 at 01:20, Michal Simek wrote: > Andrew Morton wrote: >> On Mon, 26 Jul 2010 10:49:50 +0200 Michal Simek wrote: >>> Signed-off-by: Michal Simek <monstr@monstr.eu> >>> Acked-by: Steven Rostedt <rostedt@goodmis.org> >>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> >> >> hm, I wonder how my signoff got there. Doesn't matter. > > http://lkml.org/lkml/2010/5/26/364 Sighed != Signed ;) -mike ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-07-28 5:35 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1274863724-14906-1-git-send-email-monstr@monstr.eu>
[not found] ` <1274863724-14906-2-git-send-email-monstr@monstr.eu>
2010-05-26 14:42 ` [PATCH] asm-generic: Use raw_local_irq_save/restore instead local_irq_save/restore Steven Rostedt
2010-05-26 17:08 ` Andrew Morton
2010-05-26 17:11 ` Steven Rostedt
2010-05-27 7:14 ` Michal Simek
2010-05-27 9:11 ` Yong Zhang
2010-07-26 8:49 ` Michal Simek
2010-07-27 23:18 ` Andrew Morton
2010-07-27 23:54 ` Mike Frysinger
2010-07-28 5:23 ` Michal Simek
2010-07-28 5:20 ` Michal Simek
2010-07-28 5:35 ` Mike Frysinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox