Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <238f23ab72d99c3d2737a1ef0b68aebe60de8f8a.camel@redhat.com>
Subject: Re: [PATCH] nohz/full, sched/rt: Fix missed tick-reenabling bug in
 dequeue_task_rt
From:   Nicolas Saenz Julienne <nsaenzju@redhat.com>
To:     Valentin Schneider <vschneid@redhat.com>,
        linux-kernel@vger.kernel.org, fweisbec@gmail.com
Cc:     bristot@redhat.com, cmetcalf@ezchip.com, mgorman@suse.de,
        bsegall@google.com, rostedt@goodmis.org, dietmar.eggemann@arm.com,
        vincent.guittot@linaro.org, juri.lelli@redhat.com,
        peterz@infradead.org, mingo@redhat.com, mtosatti@redhat.com
Date:   Fri, 01 Jul 2022 13:35:15 +0200
In-Reply-To: <xhsmhedz55b2v.mognet@vschneid.remote.csb>
References: <20220628092259.330171-1-nsaenzju@redhat.com>
         <xhsmhedz55b2v.mognet@vschneid.remote.csb>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2022-07-01 at 12:25 +0100, Valentin Schneider wrote:
> On 28/06/22 11:22, Nicolas Saenz Julienne wrote:
> > dequeue_task_rt() only decrements 'rt_rq->rt_nr_running' after having
> > called sched_update_tick_dependency() preventing it from re-enabling th=
e
> > tick on systems that no longer have pending SCHED_RT tasks but have
> > multiple runnable SCHED_OTHER tasks:
> >=20
> >   dequeue_task_rt()
> >     dequeue_rt_entity()
> >       dequeue_rt_stack()
> >         dequeue_top_rt_rq()
> >         sub_nr_running()	// decrements rq->nr_running
> >           sched_update_tick_dependency()
> >             sched_can_stop_tick()	// checks rq->rt.rt_nr_running,
> >             ...
> >         __dequeue_rt_entity()
> >           dec_rt_tasks()	// decrements rq->rt.rt_nr_running
> >         ...
> >=20
> > Every other scheduler class performs the operation in the opposite
> > order, and sched_update_tick_dependency() expects the values to be
> > updated as such. So avoid the misbehaviour by inverting the order in
> > which the above operations are performed in the RT scheduler.
> >=20
>=20
> I can't see anything wrong with your approach, though I did have to spend
> some time re-learning RT_GROUP_SCHED. The designated Fixes: commit looks
> about right too.
>=20
> > Fixes: 76d92ac305f2 ("sched: Migrate sched to use new tick dependency m=
ask model")
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
>=20
> Reviewed-by: Valentin Schneider <vschneid@redhat.com>

Thanks!

--=20
Nicol=C3=A1s S=C3=A1enz