Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211011172236.11223-1-mkoutny@suse.com> <CAFpoUr1KrXKdiCp-DVQLnu-c2YS91AszZB6RdZNVLBm9sGjMEw@mail.gmail.com>
 <20211013143900.GB48428@blackbody.suse.cz>
In-Reply-To: <20211013143900.GB48428@blackbody.suse.cz>
From:   Odin Ugedal <odin@uged.al>
Date:   Wed, 13 Oct 2021 19:45:59 +0100
Message-ID: <CAFpoUr1NCA+L=+FkxJ-iRU-s0ehe__zY7PCwnEsk3BxOdrvCSw@mail.gmail.com>
Subject: Re: [PATCH] sched/fair: Use rq->lock when checking cfs_rq list presence
To:     =?UTF-8?Q?Michal_Koutn=C3=BD?= <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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

ons. 13. okt. 2021 kl. 15:39 skrev Michal Koutn=C3=BD <mkoutny@suse.com>:
>
> On Mon, Oct 11, 2021 at 08:12:08PM +0100, Odin Ugedal <odin@uged.al> wrot=
e:
>
> 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 we=
ll...

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=3D2 inside that lock under your code change? If we
then treat on_list=3D2
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