Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Joel Fernandes <joel@joelfernandes.org>]

On Sat, Jan 19, 2019 at 11:29:12AM +0100, Hugo Lefeuvre wrote:
> > > as far as I understand this code, freezable_schedule() avoids blocking the
> > > freezer during the schedule() call, but in the end try_to_freeze() is still
> > > called so the result is the same, right?
> > > I wonder why wait_event_freezable is not calling freezable_schedule().
> > 
> > It could be something subtle in my view. freezable_schedule() actually makes
> > the freezer code not send a wake up to the sleeping task if a freeze happens,
> > because the PF_FREEZER_SKIP flag is set, as you pointed.
> > 
> > Whereas wait_event_freezable() which uses try_to_freeze() does not seem to have
> > this behavior and the task will enter the freezer. So I'm a bit skeptical if
> > your API will behave as expected (or at least consistently with other wait
> > APIs).
> 
> oh right, now it is clear to me:
> 
> - schedule(); try_to_freeze()
> 
> schedule() is called and the task enters sleep. Since PF_FREEZER_SKIP is
> not set, the task wakes up as soon as try_to_freeze_tasks() is called.
> Right after waking up the task calls try_to_freeze() which freezes it.
> 
> - freezable_schedule() 
> 
> schedule() is called and the task enters sleep. Since PF_FREEZER_SKIP is
> set, the task does not wake up when try_to_freeze_tasks() is called. This
> is not a problem, the task can't "do anything which isn't allowed for a
> frozen task" while sleeping[0]. 
> 
> When the task wakes up (timeout, or whatever other reason) it calls
> try_to_freeze() which freezes it if the freeze is still underway.
> 
> So if a freeze is triggered while the task is sleeping, a task executing
> freezable_schedule() might or might not notice the freeze depending on how
> long it sleeps. A task executing schedule(); try_to_freeze() will always
> notice it.
> 
> I might be wrong on that, but freezable_schedule() just seems like a
> performance improvement to me.
> 
> Now I fully agree with you that there should be a uniform definition of
> "freezable" between wait_event_freezable and wait_event_freezable_hrtimeout.
> This leaves me to the question: should I modify my definition of
> wait_event_freezable_hrtimeout, or prepare a patch for wait_event_freezable ?
> 
> If I am right with the performance thing, the latter might be worth
> considering?
> 
> Either way, this will be fixed in the v2.

I agree, it is probably better to use freezable_schedule() for all freeze
related wait APIs, and keep it consistent. Your analysis is convincing.

Peter, what do you think?

> > > That being said, I am not sure that the try_to_freeze() call does anything
> > > in the vsoc case because there is no call to set_freezable() so the thread
> > > still has PF_NOFREEZE...
> > 
> > I traced this, and PF_NOFREEZE is not set by default for tasks.
> 
> Well, I did not check this in practice and might be confused somewhere but
> the documentation[1] says "kernel threads are not freezable by default.
> However, a kernel thread may clear PF_NOFREEZE for itself by calling
> set_freezable()".
> 
> Looking at the kthreadd() definition it seems like new tasks have
> PF_NOFREEZE set by default[2].
> 
> I'll take some time to check this in practice in the next days.
> 
> Anyways, thanks for your time !

Yeah, no problem.

thanks,

 - Joel