On Thu, 26 Dec 2013, Tejun Heo wrote: > Hello, Rafael. > > On Thu, Dec 26, 2013 at 04:05:53PM +0100, Rafael J. Wysocki wrote: > > First, there is a removal-vs-resume deadlock which technically is related to > > the freezing, but it is not entirely clear to me that using a more fine grained > > mechanism instead of the freezing will actually help here. It may turn out to > > be necessary to defer the removal until the resume is complete anyway. In fact, some places may be using the freezer as a convenient way to implement just such a deferral. > > Second, there is the problem with the freezer being a huge sledgehammer which > > sometimes is used too eagerly and without enough understanding. I agree that > > that is a problem and probably the best way to address it would be to make > > people use more specific things instead of the freezer for workqueues/kthreads. > > I don't think the two points are separate. There's no reason for > anything above device drivers (themselves or whatever midlayer > implementing command queue) to be participating in the freezer and > this deadlock occurred only because writeback is freezable for no good > reason that I can see. I don't know why writeback was made freezable -- but that would be a reasonable way to avoid changing on-disk data during hibernation. > If we make "freezing" specific to the places > where the actual PM operations are necessary, these lockups cannot > happen, or rather, if a deadlock happens, the blame would likely be > clearly on the device driver or its subsystem implementation. In the case of hibernation, it's not so simple. We do need to perform I/O, in order to save the memory image. But we also need to avoid unnecessary I/O, in order to keep the on-disk data consistent with the data in the memory image. You probably can't accomplish this at the device driver or subsystem level. > > I actually think that khubd is somewhat analogous to the PM workqueue, so > > it needs to be stopped before we start calling suspend callbacks or we'd > > have to deal with a lot more complexity than really necessary. There may > > be more things like that, but that's hard to tell without reviewing all > > users of freezable kthreads and workqueues and analysing them all. So I > > guess that's the next step if we want to go into that direction. > > I see. I'm kinda curious how that jives with runtime PM. One thing There are some very significant differences between system sleep and runtime PM. What is appropriate for one may not be appropriate for the other. In particular, the freezer is not appropriate for runtime PM. > which bothers me about the freezer is that it's essentially a separate > entry point for suspend/resume implementation, and not a particularly > well designed one at that. Things which depend on freezer for PM ops > would need completely separate paths for runtime PM. They probably > need some deviations anyway but freezer would push it unnecessarily. Maybe it's the other way around: The separate paths are necessary, and the freezer _simplifies_ the system sleep ops. > Well, converting kthreads to workqueues are pretty easy and usually > beneficial anyway, especially given that people often get things not > completely right when mixing kthread_should_stop() and freezing > condition checks (it can be surprisingly tricky and likely to work > most of the time even when slightly broken) and workqueue is a lot > easier to get right on that respect. Taking khubd as an example, I have to agree that converting it to a workqueue would be a big simplification overall. And yet there are some things khubd does which are (as far as I know) rather difficult to accomplish with workqueues. One example in drivers/usb/core/hub.c: kick_khubd() calls usb_autopm_get_interface_no_resume() if and only if it added the hub to the event list (and it does so before releasing the list's lock). How can you do that with a workqueue? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
