lkml.org 
[lkml]   [2007]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] Remove process freezer from suspend to RAM pathway
    From
    Date

    > You agree that drivers need to block various activities during suspend.
    > Principally I/O requests, but other things as well. So when one of
    > these requests arrives, the driver has to make it wait somehow and then
    > has to allow it to proceed at the appropriate time.

    Yes. The main thing I see here is that here is nothing common among
    drivers in what a "request" is and how it's processed.

    For example, for block drivers, it's actually fairly simple to just stop
    processing the request queue and wait for pending ones to complete.

    For network, in a similar vein, we can mostly just tell the network
    stack to stop sending us packets.

    That's what I call the "main path". This is often the trivial part to
    deal with, mostly because for a whole lot of drivers, it can be done via
    a couple of helpers in the subsystem that the driver provides a service
    too, via a helper, asking that subsystem to stop calling into the said
    driver (the asking should be done by the driver itself of course, for
    ordering reasons).

    We have some helpers, but I think not enough, and that's where we should
    focus imho. For example, I added fb_set_suspend() so that fbdev's can
    request fbcon to stop accessing them (it doesn't solve the problem of
    userland mmap's, that will have to be done, if we want to do it, in a
    more sneaky way, using VM tricks, but the DRM nowadays has the
    infrastructure to do it).

    But that's only the "main" path. Aside for that, almost all drivers also
    have sideband "request" input and some driver don't actually live behind
    a subsystem. That ranges from ioctl, to direct read/write on a char dev
    from userland.

    I think many of those cases can fairly well deal with just taking a PM
    semaphore, that's how I did for a couple of things in the past, provided
    that the request path isn't deadlocking with the semaphore held because
    of the system suspending of course.

    But in a whole lot of cases, it's, I beleive, perfectly kosher to just
    return an error. You're trying to capture frame from your camera while
    the machine is suspended ? error. At worst, your capture app will be
    unhappy when you wakeup, nothing terrible and totally fixable in
    userland if it's a problem.

    In some cases, we could use a little bit more help from the subsystem.
    Network for example, could have some explicit knowledge of the suspend
    state, and in addition to stopping the queue would also stop calling
    into things like change_mtu or set_multicast, provided it's agreed that
    the driver will account for those changes on resume (the actual MTU
    values or multicast lists are still updated in the netdev).

    > Normally a waitqueue or a struct completion would be used for this
    > purpose.

    I think there's no "normal" scenario, each driver or family of drivers
    will do things very differently.

    > But either one puts the burden on the driver of defining a
    > data structure and signalling it at the right time.

    Yes.

    > That time is generally when the device is resumed, but there's nothing
    > wrong with delaying it slightly, to after all the devices have been resumed (i.e.,
    > the time when the current PM code takes everything out of the freezer).

    In fact, the best is to have parallel suspend/resume of drivers and asynchronous resume but that's out of topic :-) (For the record, I did some bits like ADB resume like that, that is asynchronous, to speed up wakeup time).
    >
    > In fact, we definitely don't want to unblock plug events until this
    > later time.

    There are two things I believe. There's a generic issue with usermode helpers that make no sense to call between pre-suspend and post-resume, and there's the specific issue of adding/removing devices.

    I believe that "bus" drivers such as USB should indeed get a first round of notifications to tell them to stop performing bus plug/unplug operations (it's debatable whether we want to keep unplug going provided we can stack up the usermode events and re-send them later though, but let's say no for the sake of simplicity).

    > So instead, why not have the PM core take care of all this? There
    > could be a block_task_until_suspend_is_over() routine available for all
    > drivers to use. Its effect would be exactly the same as sending the
    > current task into the freezer, but it wouldn't be the freezer that
    > exists now. It would just be some routine that blocks until the system
    > suspend is over. We could call it "the icebox" instead of "the
    > freezer". :-)

    I'm not totally sure about that. I like some of it, but I think it's
    fairly different conceptually from the freezer (and the implementation
    could be as trivial as a single system wide wait queue).

    Basically it has a very big difference to the current freezer, and I
    like that, which is that we don't have some 3rd party trying to find out
    what to freeze and what not (the freezer), but instead, we have
    explicitely drivers or kernel threads sending -themselves- to the
    "icebox" when they think it's a good idea. Think of it as lazy freezing
    -> you only freeze lazy tasks that are trying to do something that
    cannot be done because of suspend.

    > Does that make you happier?

    I think it's a fairly significant change from the current freezer and I
    also think it's a very good idea. The more I think about it, the more I
    like it, in the sense that it's a simple drop-in that you could put in a
    lot of the ioctl path of drivers to just block tasks that are trying to
    call in while suspending, and could be used selectively by things like
    the USB hub threads.

    > User tasks can cause driver binding by writing to sysfs. Binding
    > _can't_ be blocked in the driver; by then it's already too late. If
    > it is going to be blocked at all, it has to be blocked earlier. One
    > possibility is in the sysfs attribute code; another is to block all
    > sysfs access.
    >
    > Of course, another possibility is simply to fail the bind. But that's
    > not very satisfying, since suspends should be transparent.

    I don't think suspend has to be -that- transparent (though there is some
    debate on whether it should be if we're gonna do some kind of fast
    "light" suspend for things like OLPC) but overall, I agree that a bind
    operation on sysfs should probably block until resume, and it does make
    sense to have the logic to do that in sysfs itself. It could perfectly
    use the above icebox thingy you came up with.

    > Ben, you haven't given enough thought to the work needed to avoid
    > locking problems.
    >
    > For instance, you agree that during suspend we must not allow device or
    > driver registration or unregistration, right?

    Registration is fine, binding/ubinding is not (no problem putting the
    driver on/off a list, though of course, you can't unregister a bound
    driver without unbinding, it's prefectly find to unregister an unbound
    driver).

    > And we must not allow driver binding or unbinding.

    That's where the meat is.

    > But these events generally involve acquiring a device semaphore, in the
    > driver core and quite often in the core's caller. Since that semaphore
    > is also needed for calling the suspend and resume methods, we have to be
    > very careful about blocking binding/unbinding/registration/unregistration.

    > It has to be done at a time when no device semaphores are held.

    I think that should indeed be handled within the driver core. Though I
    tend to think our driver core is a bit of a locking mess at the
    moment :-) (Who says it could use more RCU-like constructs ?).

    Regarding unbinding, that's debatable, it might be perfectly allright to
    unbind while suspended, though then, there is the question of a driver
    being later on bound to a piece of HW that is suspended (though I
    suppose that could happen today... some machines have their firmware
    leave some devices off at boot).

    As a general matter, If we have those pre-suspend/post-resume notifiers
    and we adapt the few (there isn't that much) bus drivers so that they
    stop all probe/unprobe operations before the suspend dance starts, we
    avoid a lot of that problem. At this point, it becomes fair enough for
    bind/unbind, in the core, to return an error (and maybe even stack trace
    in dmesg to catch the culprits) while suspend in in progress.

    The only remaining annoying case is manual bind/unbind via sysfs which
    is a good candidate for the above described icebox.

    > You also agree that kernel threads and workqueues must be allowed to
    > operate during suspend.

    Yes, unless kernel threads explicitely decide to stop themselves (for
    example, khubd is a good candidate for that). Again, not a 3rd party
    trying to decide what to freeze and what not, but the drivers or kernel
    threads themselves deciding it.

    > But consider this: By writing the appropriate
    > sysfs attribute, a user task can cause a workqueue item to be queued to
    > keventd that tries to unregister a device. That really puts you on the
    > spot: Unregistration can't be allowed to fail, it can't be allowed to
    > succeed during a suspend, and keventd can't be blocked! So what should
    > we do?

    We can either stop it at the sysfs write level, or we can have the
    workqueue task reschedule itself later until we are resumed. In fact,
    worqueue items being what they are (queue items), we could imagine
    having a special list where they enqueue themselves to rescheduled after
    resume.

    Don't get me wrong, I never said we don't need generic infrastructure
    and utilities, such as your proposed icebox scheme, or some of those
    workqueue bits, helpers in subsystems, etc...

    I just think that the freezer approach, as it is, is backward. We can't
    have a 3rd party try to discriminate what to freeze and what not, it
    will always get something wrong, and in some cases with the wrong timing
    or ordering.

    Cheers,
    Ben.


    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-07-08 07:17    [W:0.033 / U:29.872 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site