* Re: PROBLEM: in_atomic() misuse all over the place
[not found] <200901280010.37633.roger.larsson@e-gatan.se>
@ 2009-01-28 12:18 ` Andi Kleen
2009-01-31 0:03 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Andi Kleen @ 2009-01-28 12:18 UTC (permalink / raw)
To: Roger Larsson
Cc: linux-kernel, Ingo Molnar, Robert Love, Pavel Machek, netdev
Roger Larsson <roger.larsson@e-gatan.se> writes:
[cc netdev since network code is discussed]
> [2.] Full description of the problem/report:
>
> The problem is that in_atomic() only works on preemptive kernels...
That's not fully correct. It works in tasklets/softirqs/spin_lock_bh
on all kernels.
> file: include/net/sock.h
>
> static inline gfp_t gfp_any(void)
> {
> return in_atomic() ? GFP_ATOMIC : GFP_KERNEL;
> }
That's typically for softirq vs non softirq, which is important
for the network stack.
That said its undoubtedly possible that some users get
it wrong and assume it applies to spinlocks (without _bh) too.
But at least the classical uses in the network stack should be ok.
I personally would consider anyone using it inside a normal
spinlock dubious anyways, because GFP_ATOMIC is the wrong
thing to use here. The correct way is to either switch to a mutex/sem
or move the allocation outside the lock.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PROBLEM: in_atomic() misuse all over the place
2009-01-28 12:18 ` PROBLEM: in_atomic() misuse all over the place Andi Kleen
@ 2009-01-31 0:03 ` Andrew Morton
2009-01-31 5:55 ` Andi Kleen
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2009-01-31 0:03 UTC (permalink / raw)
To: Andi Kleen; +Cc: roger.larsson, linux-kernel, mingo, rml, pavel, netdev
On Wed, 28 Jan 2009 13:18:50 +0100
Andi Kleen <andi@firstfloor.org> wrote:
> > file: include/net/sock.h
> >
> > static inline gfp_t gfp_any(void)
> > {
> > return in_atomic() ? GFP_ATOMIC : GFP_KERNEL;
> > }
>
> That's typically for softirq vs non softirq, which is important
> for the network stack.
>
There's a bit of a problem here. If someone accidentally uses
gfp_any() inside a spinlock, it will do a sleeping allocation on
non-preempt kernels and will do an atomic allocation on preemptible
kernels, so we won't get to see the warning which would allow us to fix
the bug.
Would using irq_count() work? If so, that would fix this up.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PROBLEM: in_atomic() misuse all over the place
2009-01-31 5:55 ` Andi Kleen
@ 2009-01-31 5:49 ` Andrew Morton
2009-01-31 8:48 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2009-01-31 5:49 UTC (permalink / raw)
To: Andi Kleen; +Cc: roger.larsson, linux-kernel, mingo, rml, pavel, netdev
On Sat, 31 Jan 2009 06:55:08 +0100 Andi Kleen <andi@firstfloor.org> wrote:
> > There's a bit of a problem here. If someone accidentally uses
> > gfp_any() inside a spinlock, it will do a sleeping allocation on
> > non-preempt kernels and will do an atomic allocation on preemptible
> > kernels, so we won't get to see the warning which would allow us to fix
> > the bug.
>
> Yes exporting the function to drivers is dangerous I agree because
> it's easy to abuse.
>
> > Would using irq_count() work? If so, that would fix this up.
>
> There's nothing that works reliably to detect spinlocks on non
> preempt kernels.
Hang on. You said
That's typically for softirq vs non softirq, which is important for
the network stack.
that's what in_softirq() does.
Now, if networking is indeed using in_atomic() to detect
are-we-inside-a-spinlock then networking is buggy.
If networking is _not_ doing that then we can safely switch it to
in_sortirq() or in_interrupt(). And this would reenable the bug
detection which networking's use of in_atomic() accidentally
suppressed.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PROBLEM: in_atomic() misuse all over the place
2009-01-31 0:03 ` Andrew Morton
@ 2009-01-31 5:55 ` Andi Kleen
2009-01-31 5:49 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Andi Kleen @ 2009-01-31 5:55 UTC (permalink / raw)
To: Andrew Morton
Cc: Andi Kleen, roger.larsson, linux-kernel, mingo, rml, pavel,
netdev
> There's a bit of a problem here. If someone accidentally uses
> gfp_any() inside a spinlock, it will do a sleeping allocation on
> non-preempt kernels and will do an atomic allocation on preemptible
> kernels, so we won't get to see the warning which would allow us to fix
> the bug.
Yes exporting the function to drivers is dangerous I agree because
it's easy to abuse.
> Would using irq_count() work? If so, that would fix this up.
There's nothing that works reliably to detect spinlocks on non
preempt kernels.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PROBLEM: in_atomic() misuse all over the place
2009-01-31 5:49 ` Andrew Morton
@ 2009-01-31 8:48 ` David Miller
2009-01-31 8:58 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2009-01-31 8:48 UTC (permalink / raw)
To: akpm; +Cc: andi, roger.larsson, linux-kernel, mingo, rml, pavel, netdev
From: Andrew Morton <akpm@linux-foundation.org>
Date: Fri, 30 Jan 2009 21:49:33 -0800
> Hang on. You said
>
> That's typically for softirq vs non softirq, which is important for
> the network stack.
>
> that's what in_softirq() does.
>
> Now, if networking is indeed using in_atomic() to detect
> are-we-inside-a-spinlock then networking is buggy.
>
> If networking is _not_ doing that then we can safely switch it to
> in_sortirq() or in_interrupt(). And this would reenable the bug
> detection which networking's use of in_atomic() accidentally
> suppressed.
I think this is a reasonable conclusion, looking at the
gfp_any() users.
Feel free to change it to use in_softirq() and see what
explodes in -mm. Report to me your findings :-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PROBLEM: in_atomic() misuse all over the place
2009-01-31 8:48 ` David Miller
@ 2009-01-31 8:58 ` Andrew Morton
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2009-01-31 8:58 UTC (permalink / raw)
To: David Miller; +Cc: andi, roger.larsson, linux-kernel, mingo, rml, pavel, netdev
On Sat, 31 Jan 2009 00:48:43 -0800 (PST) David Miller <davem@davemloft.net> wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Fri, 30 Jan 2009 21:49:33 -0800
>
> > Hang on. You said
> >
> > That's typically for softirq vs non softirq, which is important for
> > the network stack.
> >
> > that's what in_softirq() does.
> >
> > Now, if networking is indeed using in_atomic() to detect
> > are-we-inside-a-spinlock then networking is buggy.
> >
> > If networking is _not_ doing that then we can safely switch it to
> > in_sortirq() or in_interrupt(). And this would reenable the bug
> > detection which networking's use of in_atomic() accidentally
> > suppressed.
>
> I think this is a reasonable conclusion, looking at the
> gfp_any() users.
>
> Feel free to change it to use in_softirq() and see what
> explodes in -mm. Report to me your findings :-)
I don't get much network coverage in my testing...
I went for in_interrupt(), which is in_softirq()||in_hardirq(). I
guess that was a bit of a cop-out if the design decision is that this
is purely for are-we-in-softirq decision making.
I'll set it to in_softirq() and shall see what happens..
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-01-31 8:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200901280010.37633.roger.larsson@e-gatan.se>
2009-01-28 12:18 ` PROBLEM: in_atomic() misuse all over the place Andi Kleen
2009-01-31 0:03 ` Andrew Morton
2009-01-31 5:55 ` Andi Kleen
2009-01-31 5:49 ` Andrew Morton
2009-01-31 8:48 ` David Miller
2009-01-31 8:58 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).