Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
From:   Valentin Schneider <vschneid@redhat.com>
To:     Ingo Molnar <mingo@kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc:     linux-kernel@vger.kernel.org,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH] sched/rt: Make rt_rq->pushable_tasks updates drive
 rto_mask
In-Reply-To: <ZRFEeGKGzE+dY6n6@gmail.com>
References: <20230811112044.3302588-1-vschneid@redhat.com>
 <20230815142121.MoZplZUr@linutronix.de> <ZRFEeGKGzE+dY6n6@gmail.com>
Date:   Mon, 25 Sep 2023 14:09:00 +0200
Message-ID: <xhsmh4jjiv4ir.mognet@vschneid.remote.csb>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 25/09/23 10:27, Ingo Molnar wrote:
> * Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>
>> On 2023-08-11 12:20:44 [+0100], Valentin Schneider wrote:
>> > Sebastian noted that the rto_push_work IRQ work can be queued for a CPU
>> > that has an empty pushable_tasks list, which means nothing useful will=
 be
>> > done in the IPI other than queue the work for the next CPU on the rto_=
mask.
>> >=20
>> > rto_push_irq_work_func() only operates on tasks in the pushable_tasks =
list,
>> > but the conditions for that irq_work to be queued (and for a CPU to be
>> > added to the rto_mask) rely on rq_rt->nr_migratory instead.
>> >=20
>> > nr_migratory is increased whenever an RT task entity is enqueued and i=
t has
>> > nr_cpus_allowed > 1. Unlike the pushable_tasks list, nr_migratory incl=
udes a
>> > rt_rq's current task. This means a rt_rq can have a migratible current=
, N
>> > non-migratible queued tasks, and be flagged as overloaded / have its C=
PU
>> > set in the rto_mask, despite having an empty pushable_tasks list.
>> >=20
>> > Make an rt_rq's overload logic be driven by {enqueue,dequeue}_pushable=
_task().
>> > Since rt_rq->{rt_nr_migratory,rt_nr_total} become unused, remove them.
>> >=20
>> > Note that the case where the current task is pushed away to make way f=
or a
>> > migration-disabled task remains unchanged: the migration-disabled task=
 has
>> > to be in the pushable_tasks list in the first place, which means it has
>> > nr_cpus_allowed > 1.
>> >=20
>> > Link: http://lore.kernel.org/r/20230801152648._y603AS_@linutronix.de
>> > Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> > Signed-off-by: Valentin Schneider <vschneid@redhat.com>
>> > ---
>> > This is lightly tested, this looks to be working OK but I don't have n=
or am
>> > I aware of a test case for RT balancing, I suppose we want something t=
hat
>> > asserts we always run the N highest prio tasks for N CPUs, with a small
>> > margin for migrations?
>>=20
>> I don't see the storm of IPIs I saw before. So as far that goes:
>>    Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> I've applied Valentin's initial fix to tip:sched/core, for an eventual
> v6.7 merge, as it addresses the IPI storm bug. Let me know if merging
> this is not desirable for some reason.
>
>> What I still observe is:
>> - CPU0 is idle. CPU0 gets a task assigned from CPU1. That task receives
>>   a wakeup. CPU0 returns from idle and schedules the task.
>>   pull_rt_task() on CPU1 and sometimes on other CPU observe this, too.
>>   CPU1 sends irq_work to CPU0 while at the time rto_next_cpu() sees that
>>   has_pushable_tasks() return 0. That bit was cleared earlier (as per
>>   tracing).
>>=20
>> - CPU0 is idle. CPU0 gets a task assigned from CPU1. The task on CPU0 is
>>   woken up without an IPI (yay). But then pull_rt_task() decides that
>>   send irq_work and has_pushable_tasks() said that is has tasks left
>>   so=E2=80=A6.
>>   Now: rto_push_irq_work_func() run once once on CPU0, does nothing,
>>   rto_next_cpu() return CPU0 again and enqueues itself again on CPU0.
>>   Usually after the second or third round the scheduler on CPU0 makes
>>   enough progress to remove the task/ clear the CPU from mask.
>
> Just curious, any progress on solving this?
>

On my side not really, I need to stop getting distracted and probably get
this to reproduce on a system so I can understand-by-tracing

> Thanks,
>
> 	Ingo