lkml.org 
[lkml]   [2008]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: acpi_evaluate_integer broken by design
    On Wed, 26 Nov 2008 17:37:29 -0500 (EST)
    Len Brown <lenb@kernel.org> wrote:

    >
    >
    > > > Now I know why I had strange "scheduling in atomic" problems:
    > > > acpi_evaluate_integer() does malloc(..., irqs_disabled() ? GFP_ATOMIC
    > > > : GFP_KERNEL)... which is (of course) broken.
    > >
    > > That is kinda weird. When did this all start happening?
    >
    > > > There's no way to reliably tell if we need GFP_ATOMIC or not from
    > > > code, this one for example fails to detect spinlocks held.
    >
    > > Len, this looks like 2.6.28 material. But given the poor quality of
    > > the changelog it is hard to be sure about this. Why isn't everyone
    > > seeing these warnings? What did Pavel do to provoke these alleged
    > > warnings? Nobody knows...
    >
    > I don't know know why pavel sees this and nobody else --
    > maybe something unusual he's doing with suspend?
    >
    > The reason that the ACPI code is littered with bogus
    > irqs_disabled() ? GFP_ATOMIC : GFP_KERNEL)
    > is because, like boot, resume starts life with interrupts off.

    yes, suspend's disabling of interrupts causes problems all over the place.

    If we really do need to inspect global state to handle this, it would
    be much better to create a new

    bool resume_is_running_so_you_cant_sleep;

    and to test that. Something which is clear, highly specific and which
    cannot be accidentally triggered via other means.

    > I would prefer that resume and boot handle this the same way,
    > with system_state. However, a few years ago when I suggested
    > using system_state for resume, Andrew thought that was a very
    > bad idea. Andrew, do you still feel that way?

    yep. We've had problems in the past with system_state, where people add
    new states, and old code breaks. Plus as we add more dependencies on
    system_state, that reduces our ability to add new states and makes it
    harder to alter (ie: fix) system_state transitions, etc. Just run
    away!

    As I said above, if we have a piece of code which wants to know when a
    separate piece of code is in a particualr state, it is better to add a
    new state flag just for that application, rather than trying to infer
    things from the heavily overloaded system_state.


    Obviously, it is even better to do neither. It is a basic and
    oft-reoccurring design mistake for a low-level piece of code to
    hardwire its GFP_foo decision. The gfp_t should be passed in from the
    callers, all the way down the chain from the top-level code which
    actually knows what state this thread of control is in.

    > -Len
    >
    > ps. I'll put this particular fix in my tree now.

    bewdy. It's a good change.


    \
     
     \ /
      Last update: 2008-11-27 00:05    [W:5.249 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site