Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 22 Jan 2019 17:20:54 -0500
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Hugo Lefeuvre <hle@owl.eu.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Greg Hartman <ghartman@google.com>,
        Alistair Strachan <strachan@google.com>,
        Arve =?iso-8859-1?B?SGr4bm5lduVn?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>,
        Martijn Coenen <maco@android.com>,
        Christian Brauner <christian@brauner.io>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout
Message-ID: <20190122222054.GA37126@google.com>
References: <20190117224135.GC8100@hle-laptop.local>
 <20190118151941.GB187589@google.com>
 <20190118170801.GA4537@hle-laptop.local>
 <20190119015355.GA115342@google.com>
 <20190119102912.GA2647@hle-laptop.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190119102912.GA2647@hle-laptop.local>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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