Re: Re: [PATCH] Remove process freezer from suspend to RAM pathway

Rafael J. Wysocki
2007-07-07T20:48:05Z

On Saturday, 7 July 2007 04:44, Benjamin Herrenschmidt wrote:
> On Fri, 2007-07-06 at 11:31 +0200, Oliver Neukum wrote:
> > Am Freitag, 6. Juli 2007 schrieb Benjamin Herrenschmidt:
> > > On Fri, 2007-07-06 at 09:13 +0200, Rafael J. Wysocki wrote:
> > > >
> > > > The only reason (I know of) why we don't handle uninterruptible tasks in the
> > > > freezer is that we're afraid of the suspend process deadlocking with an
> > > > uninterruptible task holding a lock, but AFAICS the probability of such an
> > > > event is extremely small.
> > >
> > > What would deadlock specifically ? One of the drivers trying to acquire
> > > that lock ? It would be a driver bug then.
> >
> > Your driver's write method looks like:
> >
> > mutex_lock();
> > poke_some_hardware();
> > wait_event_uninterruptible(); //for result
> > res = evaluate_result();
> > mutex_unlock();
> > return res;
> >
> > If you put a task into the refrigerator at wait_event_interruptible()
> > you will deadlock if you need this lock for the driver to go to suspend.
> > The suspend method then must not take the lock _and_ it must be
> > aware that there may be an ongoing operation.
>
> Well... 2 things here. Either you have a freezer in which case the
> chances of the above scenario are increased,

How so? :-)

> or you don't, in which case
> your suspend method will just sleep on the lock until outstanding HW
> accesses that have that lock are completed, and everything is fine.
>
> You need to be careful with one thing though, whether you have a freezer
> or not. If you driver, in some code path, whatever it is (ioctl, kernel
> thread, workqueue, ...) does something like:
>
> mutex_lock
> kmalloc(...,GFP_KERNEL);
> mutex_unlock
>
> And it's suspend callback then does:
>
> mutex_lock
>
> The problem here is that the disks might already have been suspended
> prior to your driver being called. Thus, any attempt at pushing things
> out to swap or dirty mmap'ings back to storage will hang, thus kmalloc
> can potentially hang (afaik), and you will deadlock.
>
> That's what I've been talking about earlier when I said that we should
> have some security in SLAB/SLUB/Buddy allocators, to silently turn
> GFP_KERNEL to at least GFP_NOIO or even ATOMIC before we start
> suspending drivers.
>
> Now, another way to deal with that would have to use
> pre-suspend/post-resume notifications, and have drivers avoid doing the
> above between those, but that's much harder. (Essentially, drivers would
> have to either make sure they don't do things like blocking allocations,
> even implicitely, or possibly fall back to a degraded synchronous mode
> or that sort of thing).
>
> I think it's much simpler to tweak slab/slub/buddy instead :-)
>
> Note that the above issue is orthogonal to our freezer discussion, it's
> just one of the potential deadlock cause we have with suspend that needs
> to be fixed.

Agreed.

Greetings,
Rafael

--
"Premature optimization is the root of all evil." - Donald Knuth