MIME-Version: 1.0
References: <20211115170241.1.I94825a614577505bd1a8be9aeff208a49cb39b3d@changeid>
 <CAD=FV=UZNxt7y6bTipyvYFGMJhgp6nRozNt=iEaOYP6yc4OFpg@mail.gmail.com>
 <CAJWu+ora8F0eurnYkSXRqcT4mB8k8wy9CyKj1EGk52vEVEkT3A@mail.gmail.com>
 <20211202111127.gew4lqquvtn3lkbc@e107158-lin.cambridge.arm.com>
 <CAD=FV=V8aB7_6tEq9SbW+vNC5Y7G+8JMsWXo6+zdtF3tmJ97EQ@mail.gmail.com> <20211213130853.gdywukt2ibxnwzse@e107158-lin.cambridge.arm.com>
In-Reply-To: <20211213130853.gdywukt2ibxnwzse@e107158-lin.cambridge.arm.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Mon, 13 Dec 2021 10:32:36 -0800
Message-ID: <CAD=FV=Vt1AjtYvX-NEAomCy3piHOVa-E-9G=OtKdvDRp-E3HpQ@mail.gmail.com>
Subject: Re: [PATCH] sched/rt: Don't reschedule a throttled task even if it's
 higher priority
To:     Qais Yousef <qais.yousef@arm.com>
Cc:     Joel Fernandes <joelaf@google.com>, Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Mel Gorman <mgorman@suse.de>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Mon, Dec 13, 2021 at 5:09 AM Qais Yousef <qais.yousef@arm.com> wrote:
>
> > > Note that Peter fixed the kernel so that it produces known RT priorities (as
> > > opposed to developers setting random values in the past):
> > >
> > >   * sched_set_fifo_low() ==> not really RT but needs to be above cfs. Runs at
> > >     priority 1.
> > >   * sched_set_fifo() ==> sets priority MAX_RT_PRIO/2 ==> 50
[ ... cut ... ]
> > I would also say that with Peter's fix above the problem is perhaps
> > _more_ urgent? You just said that there's a whole bunch of kernel
>
> I can't see the problem still tbh.

I think this is the key, so let me explain more here. I don't think
it's worth nitpicking the documentation more to figure out what the
original author might have meant. Even if the current behavior is
expected, it's still a broken behavior that's worth fixing.

Here's my point of view:

1. The fact that the kernel has some of its threads running w/
sched_set_fifo_low() is not ABI to userspace, right? The kernel's
usage of this function on some tasks is an implementation detail and
not something that userspace should need to know about or worry about.

2. Presumably when the kernel is using sched_set_fifo_low() it's doing
so for tasks that are important for the running of the system. I
suppose you could say that _all_ kernel tasks are important to the
running of the system, but presumably these ones are higher priority
and thus more important.

3. If userspace is bothering with all the setup of RT_GROUP_SCHED, it
presumably is expecting it to do something useful. Presumably this
"useful" thing is to keep other parts of the system (those not in the
RT group) working normally. Specifically userspace wouldn't be
expecting the system to crash or a big chunk of kernel functionality
to just stop working if the scheduling allocation is exceeded.

4. Userspace expects priorities other than the "lowest" priority to be
useful for something. If they're not then they should be disallowed.

Maybe from the above points my argument is clear? Said another way:
Userspace is allowed to use a priority other than the lowest one.
Userspace wouldn't be setting up RT_GROUP_SCHED if it didn't think it
would be needed. Userspace doesn't want the kernel to crash / chunks
of functionality to fail when RT_GROUP_SCHED triggers. Userspace can't
know / account for kernel tasks using sched_set_fifo_low()

As an actual example, on my system (which has important kernel tasks
using sched_set_fifo_low()) a big chunk of the kernel is unusable if I
run my testcase. We can't get keyboard input nor do any other
communication to one of the important components in our system.


-Doug