Messages in this thread | | | Date | Fri, 31 Oct 2014 22:53:42 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 2/7] wait: Reimplement wait_event_freezable() |
| |
On 10/31, Oleg Nesterov wrote: > > On 10/31, Peter Zijlstra wrote: > > > > Provide better implementations of wait_event_freezable() APIs. > > > > The problem is with freezer_do_not_count(), it hides the thread from > > the freezer, even though this thread might not actually freeze/sleep > > at all. > > I agree, wait_event_freezable() is awful. But could you clarify "at all" ? > > Sure, the task can be preempted right after it sets, it can do a lot > of things before it calls schedule(), it can be woken after that and > it can run again and do something else before freezer_count() calls > try_to_freeze(), etc. > > Is this what you meant? > > > > +#define __wait_event_freezable(wq, condition) \ > > + (void)___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, 0, \ > > + schedule(); try_to_freeze()) > > I don't think this can work. wait_event_freezable() should be used by > kernel threads and thus we can't rely on TIF_SIGPENDING, freeze_task() > simply does wake_up_state(TASK_INTERRUPTIBLE) in this case. > > Just for example, suppose that try_to_freeze_tasks() calls freeze_task() > before this kthread sets current->state = TASK_INTERRUPTIBLE. In this > case __wait_event_freezable()->schedule() will happily sleep and > try_to_freeze_tasks() will fail. > > That is why I tried to suggest cmd == freezable_schedule(). Still not > good, but at least this narrows the window and (perhaps) we can improve > this freezable_schedule() later. > > But on a second thought... Probably cmd => try_to_freeze(); schedule(); > should work. Or just > > #define __wait_event_freezable(wq, condition) \ > __wait_event_interruptible(wq, ({ try_to_freeze(); (condition); })) > > which looks simpler.
Ah, but I forgot about another problem... can't we finally kill this ugly "long save = current->state" code in __refrigerator() ? Nobody seem to understand why we are doing this, and this looks just wrong.
And this doesn't allow us to do try_to_freeze() + schedule().
Oleg.
| |