Re: [PATCH] sched/fair: Use rq->lock when checking cfs_rq list presence

From: Odin Ugedal <odin@uged.al>
To: "Michal Koutný" <mkoutny@suse.com>
Cc: Odin Ugedal <odin@uged.al>,
	open list <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
	Daniel Bristot de Oliveira <bristot@redhat.com>
Subject: Re: [PATCH] sched/fair: Use rq->lock when checking cfs_rq list presence
Date: Wed, 13 Oct 2021 19:45:59 +0100	[thread overview]
Message-ID: <CAFpoUr1NCA+L=+FkxJ-iRU-s0ehe__zY7PCwnEsk3BxOdrvCSw@mail.gmail.com> (raw)
In-Reply-To: <20211013143900.GB48428@blackbody.suse.cz>

ons. 13. okt. 2021 kl. 15:39 skrev Michal Koutný <mkoutny@suse.com>:
>
> On Mon, Oct 11, 2021 at 08:12:08PM +0100, Odin Ugedal <odin@uged.al> wrote:
>
> Yes, that's my working hypothesis but Vincent found a loophole in the
> proposed fix under this assumption.

Yes, I started to think about the same thing late yesterday afternoon as well...

Ref. your comment about reverting a7b359fc6a37
("sched/fair: Correctly insert cfs_rq's to list on unthrottle"), I
think that is fine as
long as we revert the commit it fixes as well, to avoid a regression
of that (but yeah,
that regression itself is less bad than your discovery). If we do so,
we know that the
only time we remove it from the list is when it is fully decayed,
creating way less
edge cases

In regards to the race, would a simple fix for that be to, in addition
to your patch,
set cfs_rq->on_list=2 inside that lock under your code change? If we
then treat on_list=2
as "not on list, and do not add"? We can then make constants for them.
In that case, we
would know that the cfs_rq will never again be inserted into the list,
even if it has load. Would
something like that work properly?

Thanks
Odin