linux-kernel - Re: [PATCH] sched/wait: introduce wait_event_freezable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190119102912.GA2647@hle-laptop.local>
Date:   Sat, 19 Jan 2019 11:29:12 +0100
From:   Hugo Lefeuvre <hle@....eu.com>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Greg Hartman <ghartman@...gle.com>,
        Alistair Strachan <strachan@...gle.com>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Christian Brauner <christian@...uner.io>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

> > 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.

> > 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 !

regards,
 Hugo

[0] https://elixir.bootlin.com/linux/latest/source/include/linux/freezer.h#L103
[1] https://elixir.bootlin.com/linux/latest/source/Documentation/power/freezing-of-tasks.txt#L90
[2] https://elixir.bootlin.com/linux/latest/source/kernel/kthread.c#L569

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)