[PATCH 0/5] Fix FIFO-99 abuse

[PATCH 0/5] Fix FIFO-99 abuse

[Peter Zijlstra]

I noticed a bunch of kthreads defaulted to FIFO-99, fix them.

The generic default is FIFO-50, the admin will have to configure the system
anyway.

For some the purpose is to be above OTHER and then FIFO-1 really is sufficient.

[PATCH 1/5] sched/pci: Reduce psimon FIFO priority

[Peter Zijlstra]

PSI defaults to a FIFO-99 thread, reduce this to FIFO-1.

FIFO-99 is the very highest priority available to SCHED_FIFO and
it not a suitable default; it would indicate the psi work is the
most important work on the machine.

Since Real-Time tasks will have pre-allocated memory and locked it in
place, Real-Time tasks do not care about PSI. All it needs is to be
above OTHER.

Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/psi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -1051,7 +1051,7 @@ struct psi_trigger *psi_trigger_create(s
 
 	if (!rcu_access_pointer(group->poll_kworker)) {
 		struct sched_param param = {
-			.sched_priority = MAX_RT_PRIO - 1,
+			.sched_priority = 1,
 		};
 		struct kthread_worker *kworker;
 

Re: [PATCH 1/5] sched/pci: Reduce psimon FIFO priority

[Johannes Weiner]

On Thu, Aug 01, 2019 at 01:13:49PM +0200, Peter Zijlstra wrote:
> PSI defaults to a FIFO-99 thread, reduce this to FIFO-1.
> 
> FIFO-99 is the very highest priority available to SCHED_FIFO and
> it not a suitable default; it would indicate the psi work is the
> most important work on the machine.
> 
> Since Real-Time tasks will have pre-allocated memory and locked it in
> place, Real-Time tasks do not care about PSI. All it needs is to be
> above OTHER.
> 
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Subject should be s/pci/psi/

Re: [PATCH 1/5] sched/pci: Reduce psimon FIFO priority

[Suren Baghdasaryan]

On Thu, Aug 1, 2019 at 10:49 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Thu, Aug 01, 2019 at 01:13:49PM +0200, Peter Zijlstra wrote:
> > PSI defaults to a FIFO-99 thread, reduce this to FIFO-1.
> >
> > FIFO-99 is the very highest priority available to SCHED_FIFO and
> > it not a suitable default; it would indicate the psi work is the
> > most important work on the machine.
> >
> > Since Real-Time tasks will have pre-allocated memory and locked it in
> > place, Real-Time tasks do not care about PSI. All it needs is to be
> > above OTHER.
> >
> > Cc: Suren Baghdasaryan <surenb@google.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>
> Subject should be s/pci/psi/

Thanks for the patch. Please give me a couple hours for testing to
ensure no regressions before merging it.

Re: [PATCH 1/5] sched/pci: Reduce psimon FIFO priority

[Suren Baghdasaryan]

On Thu, Aug 1, 2019 at 11:31 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Thu, Aug 1, 2019 at 10:49 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Thu, Aug 01, 2019 at 01:13:49PM +0200, Peter Zijlstra wrote:
> > > PSI defaults to a FIFO-99 thread, reduce this to FIFO-1.

nit: above "PSI defaults" is more accurately "PSI polling defaults".
The core PSI thread that handles regular updates is a regular kthread.

> > >
> > > FIFO-99 is the very highest priority available to SCHED_FIFO and
> > > it not a suitable default; it would indicate the psi work is the
> > > most important work on the machine.
> > >
> > > Since Real-Time tasks will have pre-allocated memory and locked it in
> > > place, Real-Time tasks do not care about PSI. All it needs is to be
> > > above OTHER.
> > >
> > > Cc: Suren Baghdasaryan <surenb@google.com>
> > > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> >
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> >
> > Subject should be s/pci/psi/
>
> Thanks for the patch. Please give me a couple hours for testing to
> ensure no regressions before merging it.

Tested-by: Suren Baghdasaryan <surenb@google.com>

Re: [PATCH 1/5] sched/pci: Reduce psimon FIFO priority

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 01:49:07PM -0400, Johannes Weiner wrote:
> On Thu, Aug 01, 2019 at 01:13:49PM +0200, Peter Zijlstra wrote:
> > PSI defaults to a FIFO-99 thread, reduce this to FIFO-1.
> > 
> > FIFO-99 is the very highest priority available to SCHED_FIFO and
> > it not a suitable default; it would indicate the psi work is the
> > most important work on the machine.
> > 
> > Since Real-Time tasks will have pre-allocated memory and locked it in
> > place, Real-Time tasks do not care about PSI. All it needs is to be
> > above OTHER.
> > 
> > Cc: Suren Baghdasaryan <surenb@google.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks!

> Subject should be s/pci/psi/

Yeah, already fixed that.. obviously one sees such typoes only after
sending ;-)

[PATCH 2/5] rcu/tree: Fix SCHED_FIFO params

[Peter Zijlstra]

A rather embarrasing mistake had us call sched_setscheduler() before
initializing the parameters passed to it.

Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Fixes: 1a763fd7c633 ("rcu/tree: Call setschedule() gp ktread to SCHED_FIFO outside of atomic region")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/rcu/tree.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3234,13 +3234,13 @@ static int __init rcu_spawn_gp_kthread(v
 	t = kthread_create(rcu_gp_kthread, NULL, "%s", rcu_state.name);
 	if (WARN_ONCE(IS_ERR(t), "%s: Could not start grace-period kthread, OOM is now expected behavior\n", __func__))
 		return 0;
-	if (kthread_prio)
+	if (kthread_prio) {
+		sp.sched_priority = kthread_prio;
 		sched_setscheduler_nocheck(t, SCHED_FIFO, &sp);
+	}
 	rnp = rcu_get_root();
 	raw_spin_lock_irqsave_rcu_node(rnp, flags);
 	rcu_state.gp_kthread = t;
-	if (kthread_prio)
-		sp.sched_priority = kthread_prio;
 	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
 	wake_up_process(t);
 	rcu_spawn_nocb_kthreads();

Re: [PATCH 2/5] rcu/tree: Fix SCHED_FIFO params

[Paul E. McKenney]

On Thu, Aug 01, 2019 at 01:13:50PM +0200, Peter Zijlstra wrote:
> A rather embarrasing mistake had us call sched_setscheduler() before
> initializing the parameters passed to it.
> 
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>

Thank you for having CCed me this time.  ;-)

Reviewed-by: Paul E. McKenney <paulmck@linux.ibm.com>

> Fixes: 1a763fd7c633 ("rcu/tree: Call setschedule() gp ktread to SCHED_FIFO outside of atomic region")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/rcu/tree.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3234,13 +3234,13 @@ static int __init rcu_spawn_gp_kthread(v
>  	t = kthread_create(rcu_gp_kthread, NULL, "%s", rcu_state.name);
>  	if (WARN_ONCE(IS_ERR(t), "%s: Could not start grace-period kthread, OOM is now expected behavior\n", __func__))
>  		return 0;
> -	if (kthread_prio)
> +	if (kthread_prio) {
> +		sp.sched_priority = kthread_prio;
>  		sched_setscheduler_nocheck(t, SCHED_FIFO, &sp);
> +	}
>  	rnp = rcu_get_root();
>  	raw_spin_lock_irqsave_rcu_node(rnp, flags);
>  	rcu_state.gp_kthread = t;
> -	if (kthread_prio)
> -		sp.sched_priority = kthread_prio;
>  	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>  	wake_up_process(t);
>  	rcu_spawn_nocb_kthreads();
> 
> 

Re: [PATCH 2/5] rcu/tree: Fix SCHED_FIFO params

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 06:51:03AM -0700, Paul E. McKenney wrote:
> On Thu, Aug 01, 2019 at 01:13:50PM +0200, Peter Zijlstra wrote:
> > A rather embarrasing mistake had us call sched_setscheduler() before
> > initializing the parameters passed to it.
> > 
> > Cc: Juri Lelli <juri.lelli@redhat.com>
> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> 
> Thank you for having CCed me this time.  ;-)

Yeah, pretty much everything about that last time seems to have gone
wrong...

Re: [PATCH 2/5] rcu/tree: Fix SCHED_FIFO params

[Paul E. McKenney]

On Thu, Aug 01, 2019 at 04:43:27PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 01, 2019 at 06:51:03AM -0700, Paul E. McKenney wrote:
> > On Thu, Aug 01, 2019 at 01:13:50PM +0200, Peter Zijlstra wrote:
> > > A rather embarrasing mistake had us call sched_setscheduler() before
> > > initializing the parameters passed to it.
> > > 
> > > Cc: Juri Lelli <juri.lelli@redhat.com>
> > > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> > 
> > Thank you for having CCed me this time.  ;-)
> 
> Yeah, pretty much everything about that last time seems to have gone
> wrong...

Not that I necessarily would have spotted the bug, mind you...

							Thanx, Paul

[PATCH 3/5] crypto: Reduce default RT priority

[Peter Zijlstra]

The crypto engine initializes its kworker thread to FIFO-99 (when
requesting RT priority), reduce this to FIFO-50.

FIFO-99 is the very highest priority available to SCHED_FIFO and
it not a suitable default; it would indicate the crypto work is the
most important work on the machine.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 crypto/crypto_engine.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -425,7 +425,7 @@ EXPORT_SYMBOL_GPL(crypto_engine_stop);
  */
 struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt)
 {
-	struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
+	struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
 	struct crypto_engine *engine;
 
 	if (!dev)

Re: [PATCH 3/5] crypto: Reduce default RT priority

[Herbert Xu]

On Thu, Aug 01, 2019 at 01:13:51PM +0200, Peter Zijlstra wrote:
> The crypto engine initializes its kworker thread to FIFO-99 (when
> requesting RT priority), reduce this to FIFO-50.
> 
> FIFO-99 is the very highest priority available to SCHED_FIFO and
> it not a suitable default; it would indicate the crypto work is the
> most important work on the machine.
> 
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  crypto/crypto_engine.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 4/5] media/ivtv: Reduce default FIFO priority

[Peter Zijlstra]

The ivtv driver creates a FIFO-99 thread by default, reduce this to
FIFO-1.

FIFO-99 is the very highest priority available to SCHED_FIFO and
it not a suitable default; it would indicate the ivtv work is the
most important work on the machine.

FIFO-1 gets it above all OTHER tasks, which seems high enough lacking
better justification.

Cc: Andy Walls <awalls@md.metrocast.net>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/media/pci/ivtv/ivtv-driver.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/media/pci/ivtv/ivtv-driver.c
+++ b/drivers/media/pci/ivtv/ivtv-driver.c
@@ -738,7 +738,7 @@ static void ivtv_process_options(struct
  */
 static int ivtv_init_struct1(struct ivtv *itv)
 {
-	struct sched_param param = { .sched_priority = 99 };
+	struct sched_param param = { .sched_priority = 1 };
 
 	itv->base_addr = pci_resource_start(itv->pdev, 0);
 	itv->enc_mbox.max_mbox = 2; /* the encoder has 3 mailboxes (0-2) */

Re: [PATCH 4/5] media/ivtv: Reduce default FIFO priority

[Andy Walls]

Hi Peter:

On Thu, 2019-08-01 at 13:13 +0200, Peter Zijlstra wrote:
> The ivtv driver creates a FIFO-99 thread by default, reduce this to
> FIFO-1.
> 
> FIFO-99 is the very highest priority available to SCHED_FIFO and
> it not a suitable default; it would indicate the ivtv work is the
> most important work on the machine.

ivtv based boards are legacy, convential PCI boards.  At this point,
these old boards are generally installed in boxes dedicated to video
capture (e.g. MythTV setups) or boxes dedicated to capturing VBI
information, like closed captioning, for business intelligence.

For boxes dedicated to video or VBI capture, the ivtv work may very
well be close to the most important work on the machine, to avoid
dropping video frames or VBI data.


> FIFO-1 gets it above all OTHER tasks, which seems high enough lacking
> better justification.

I agree that FIFO-99 is the wrong default level.

However, in my opinion, threads responsible for real time data
acquisition should have higher priority than the other kernel driver
threads normally running at FIFO-50.

How about FIFO-51 as the default?

Regards,
Andy

> Cc: Andy Walls <awalls@md.metrocast.net>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-media@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  drivers/media/pci/ivtv/ivtv-driver.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/media/pci/ivtv/ivtv-driver.c
> +++ b/drivers/media/pci/ivtv/ivtv-driver.c
> @@ -738,7 +738,7 @@ static void ivtv_process_options(struct
>   */
>  static int ivtv_init_struct1(struct ivtv *itv)
>  {
> -	struct sched_param param = { .sched_priority = 99 };
> +	struct sched_param param = { .sched_priority = 1 };
>  
>  	itv->base_addr = pci_resource_start(itv->pdev, 0);
>  	itv->enc_mbox.max_mbox = 2; /* the encoder has 3 mailboxes (0-
> 2) */
> 
> 

Re: [PATCH 4/5] media/ivtv: Reduce default FIFO priority

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 08:24:22AM -0400, Andy Walls wrote:
> Hi Peter:
> 
> On Thu, 2019-08-01 at 13:13 +0200, Peter Zijlstra wrote:
> > The ivtv driver creates a FIFO-99 thread by default, reduce this to
> > FIFO-1.
> > 
> > FIFO-99 is the very highest priority available to SCHED_FIFO and
> > it not a suitable default; it would indicate the ivtv work is the
> > most important work on the machine.
> 
> ivtv based boards are legacy, convential PCI boards.  At this point,
> these old boards are generally installed in boxes dedicated to video
> capture (e.g. MythTV setups) or boxes dedicated to capturing VBI
> information, like closed captioning, for business intelligence.
> 
> For boxes dedicated to video or VBI capture, the ivtv work may very
> well be close to the most important work on the machine, to avoid
> dropping video frames or VBI data.
> 
> 
> > FIFO-1 gets it above all OTHER tasks, which seems high enough lacking
> > better justification.
> 
> I agree that FIFO-99 is the wrong default level.
> 
> However, in my opinion, threads responsible for real time data
> acquisition should have higher priority than the other kernel driver
> threads normally running at FIFO-50.
> 
> How about FIFO-51 as the default?

If the consumer of the data are RT tasks as well (I hadn't expected that
from a TV capture device) then I'd propose to use FIFO-50 as default.

The thing is, the moment you're doing actual proper RT, the admin needs
to configure things anyway, which then very much includes setting the
priority of interrupt threads and the like.

(that is exacty why pretty much everything defaults to FIFO-50)

Re: [PATCH 4/5] media/ivtv: Reduce default FIFO priority

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 02:38:06PM +0200, Peter Zijlstra wrote:
> If the consumer of the data are RT tasks as well (I hadn't expected that
> from a TV capture device) then I'd propose to use FIFO-50 as default.
> 
> The thing is, the moment you're doing actual proper RT, the admin needs
> to configure things anyway, which then very much includes setting the
> priority of interrupt threads and the like.
> 
> (that is exacty why pretty much everything defaults to FIFO-50)

Is the below acceptible?

---
Subject: media/ivtv: Reduce default FIFO priority
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Aug  1 12:56:40 CEST 2019

The ivtv driver creates a FIFO-99 thread by default, reduce this to
FIFO-50.

FIFO-99 is the very highest priority available to SCHED_FIFO and
it not a suitable default; it would indicate the ivtv work is the
most important work on the machine.

Cc: Andy Walls <awalls@md.metrocast.net>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/media/pci/ivtv/ivtv-driver.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/media/pci/ivtv/ivtv-driver.c
+++ b/drivers/media/pci/ivtv/ivtv-driver.c
@@ -738,7 +738,7 @@ static void ivtv_process_options(struct
  */
 static int ivtv_init_struct1(struct ivtv *itv)
 {
-	struct sched_param param = { .sched_priority = 99 };
+	struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
 
 	itv->base_addr = pci_resource_start(itv->pdev, 0);
 	itv->enc_mbox.max_mbox = 2; /* the encoder has 3 mailboxes (0-2) */

Re: [PATCH 4/5] media/ivtv: Reduce default FIFO priority

[Hans Verkuil]

On 8/2/19 10:58 AM, Peter Zijlstra wrote:
> On Thu, Aug 01, 2019 at 02:38:06PM +0200, Peter Zijlstra wrote:
>> If the consumer of the data are RT tasks as well (I hadn't expected that
>> from a TV capture device) then I'd propose to use FIFO-50 as default.
>>
>> The thing is, the moment you're doing actual proper RT, the admin needs
>> to configure things anyway, which then very much includes setting the
>> priority of interrupt threads and the like.
>>
>> (that is exacty why pretty much everything defaults to FIFO-50)
> 
> Is the below acceptible?

I think this should be OK. ivtv is real-time sensitive since certain
tasks have to happen within (if I remember correctly) 1/60th of a second
(the time it takes to capture a single video field). Data is lost if it
can't be done within that time.

Using FIFO-50 means that it competes with other irq threads, and since
irq threads shouldn't take up much time anyway this should be OK.

Andy, what do you think?

Regards,

	Hans

> 
> ---
> Subject: media/ivtv: Reduce default FIFO priority
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Thu Aug  1 12:56:40 CEST 2019
> 
> The ivtv driver creates a FIFO-99 thread by default, reduce this to
> FIFO-50.
> 
> FIFO-99 is the very highest priority available to SCHED_FIFO and
> it not a suitable default; it would indicate the ivtv work is the
> most important work on the machine.
> 
> Cc: Andy Walls <awalls@md.metrocast.net>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-media@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  drivers/media/pci/ivtv/ivtv-driver.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/media/pci/ivtv/ivtv-driver.c
> +++ b/drivers/media/pci/ivtv/ivtv-driver.c
> @@ -738,7 +738,7 @@ static void ivtv_process_options(struct
>   */
>  static int ivtv_init_struct1(struct ivtv *itv)
>  {
> -	struct sched_param param = { .sched_priority = 99 };
> +	struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
>  
>  	itv->base_addr = pci_resource_start(itv->pdev, 0);
>  	itv->enc_mbox.max_mbox = 2; /* the encoder has 3 mailboxes (0-2) */
> 

[PATCH 5/5] spi: Reduce kthread priority

[Peter Zijlstra]

The SPI thingies request FIFO-99 by default, reduce this to FIFO-50.

FIFO-99 is the very highest priority available to SCHED_FIFO and
it not a suitable default; it would indicate the SPI work is the
most important work on the machine.

Cc: Benson Leung <bleung@chromium.org>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-spi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/platform/chrome/cros_ec_spi.c |    2 +-
 drivers/spi/spi.c                     |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -706,7 +706,7 @@ static int cros_ec_spi_devm_high_pri_all
 					   struct cros_ec_spi *ec_spi)
 {
 	struct sched_param sched_priority = {
-		.sched_priority = MAX_RT_PRIO - 1,
+		.sched_priority = MAX_RT_PRIO / 2,
 	};
 	int err;
 
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1434,7 +1434,7 @@ static void spi_pump_messages(struct kth
  */
 static void spi_set_thread_rt(struct spi_controller *ctlr)
 {
-	struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
+	struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
 
 	dev_info(&ctlr->dev,
 		"will run message pump with realtime priority\n");

Re: [PATCH 5/5] spi: Reduce kthread priority

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 227 bytes --]

On Thu, Aug 01, 2019 at 01:13:53PM +0200, Peter Zijlstra wrote:

> The SPI thingies request FIFO-99 by default, reduce this to FIFO-50.

I don't have a cover letter or any other context, what's going on here
with dependencies?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/5] spi: Reduce kthread priority

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 12:26:55PM +0100, Mark Brown wrote:
> On Thu, Aug 01, 2019 at 01:13:53PM +0200, Peter Zijlstra wrote:
> 
> > The SPI thingies request FIFO-99 by default, reduce this to FIFO-50.
> 
> I don't have a cover letter or any other context, what's going on here
> with dependencies?

Patch stands on it own. I just went through the tree looking for FIFO
(ab)users.

Re: [PATCH 5/5] spi: Reduce kthread priority

[Geert Uytterhoeven]

Hi Peter,

On Thu, Aug 1, 2019 at 1:18 PM Peter Zijlstra <peterz@infradead.org> wrote:
> The SPI thingies request FIFO-99 by default, reduce this to FIFO-50.
>
> FIFO-99 is the very highest priority available to SCHED_FIFO and
> it not a suitable default; it would indicate the SPI work is the
> most important work on the machine.
>
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: linux-spi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  drivers/platform/chrome/cros_ec_spi.c |    2 +-
>  drivers/spi/spi.c                     |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -706,7 +706,7 @@ static int cros_ec_spi_devm_high_pri_all
>                                            struct cros_ec_spi *ec_spi)
>  {
>         struct sched_param sched_priority = {
> -               .sched_priority = MAX_RT_PRIO - 1,
> +               .sched_priority = MAX_RT_PRIO / 2,

include/linux/sched/prio.h says:

 * Priority of a process goes from 0..MAX_PRIO-1, valid RT
 * priority is 0..MAX_RT_PRIO-1, and SCHED_NORMAL/SCHED_BATCH
 * tasks are in the range MAX_RT_PRIO..MAX_PRIO-1. Priority
 * values are inverted: lower p->prio value means higher priority.

So the new 50 is actually a higher priority than the old 99?

Given I'm far from an RT expert, I must be missing something?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 5/5] spi: Reduce kthread priority

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 01:27:03PM +0200, Geert Uytterhoeven wrote:
> Hi Peter,
> 
> On Thu, Aug 1, 2019 at 1:18 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > The SPI thingies request FIFO-99 by default, reduce this to FIFO-50.
> >
> > FIFO-99 is the very highest priority available to SCHED_FIFO and
> > it not a suitable default; it would indicate the SPI work is the
> > most important work on the machine.
> >
> > Cc: Benson Leung <bleung@chromium.org>
> > Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > Cc: Guenter Roeck <groeck@chromium.org>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: linux-spi@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  drivers/platform/chrome/cros_ec_spi.c |    2 +-
> >  drivers/spi/spi.c                     |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > --- a/drivers/platform/chrome/cros_ec_spi.c
> > +++ b/drivers/platform/chrome/cros_ec_spi.c
> > @@ -706,7 +706,7 @@ static int cros_ec_spi_devm_high_pri_all
> >                                            struct cros_ec_spi *ec_spi)
> >  {
> >         struct sched_param sched_priority = {
> > -               .sched_priority = MAX_RT_PRIO - 1,
> > +               .sched_priority = MAX_RT_PRIO / 2,
> 
> include/linux/sched/prio.h says:
> 
>  * Priority of a process goes from 0..MAX_PRIO-1, valid RT
>  * priority is 0..MAX_RT_PRIO-1, and SCHED_NORMAL/SCHED_BATCH
>  * tasks are in the range MAX_RT_PRIO..MAX_PRIO-1. Priority
>  * values are inverted: lower p->prio value means higher priority.
> 
> So the new 50 is actually a higher priority than the old 99?
> 
> Given I'm far from an RT expert, I must be missing something?
> Thanks!

Ah; you found the confusion ;-)

https://lkml.kernel.org/20190617122448.GA3436@hirez.programming.kicks-ass.net

But basically, user-space prio is [1-99], while in-kernel prio is
[0-98]. The above is user prio (it basically uses the
sched_setscheduler() syscall).

So 50 really is lower than 99.

Re: [PATCH 5/5] spi: Reduce kthread priority

[Geert Uytterhoeven]

Hi Peter,

On Thu, Aug 1, 2019 at 2:12 PM Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Aug 01, 2019 at 01:27:03PM +0200, Geert Uytterhoeven wrote:
> > On Thu, Aug 1, 2019 at 1:18 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > > The SPI thingies request FIFO-99 by default, reduce this to FIFO-50.
> > >
> > > FIFO-99 is the very highest priority available to SCHED_FIFO and
> > > it not a suitable default; it would indicate the SPI work is the
> > > most important work on the machine.
> > >
> > > Cc: Benson Leung <bleung@chromium.org>
> > > Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > Cc: Guenter Roeck <groeck@chromium.org>
> > > Cc: Mark Brown <broonie@kernel.org>
> > > Cc: linux-spi@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > >  drivers/platform/chrome/cros_ec_spi.c |    2 +-
> > >  drivers/spi/spi.c                     |    2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > --- a/drivers/platform/chrome/cros_ec_spi.c
> > > +++ b/drivers/platform/chrome/cros_ec_spi.c
> > > @@ -706,7 +706,7 @@ static int cros_ec_spi_devm_high_pri_all
> > >                                            struct cros_ec_spi *ec_spi)
> > >  {
> > >         struct sched_param sched_priority = {
> > > -               .sched_priority = MAX_RT_PRIO - 1,
> > > +               .sched_priority = MAX_RT_PRIO / 2,
> >
> > include/linux/sched/prio.h says:
> >
> >  * Priority of a process goes from 0..MAX_PRIO-1, valid RT
> >  * priority is 0..MAX_RT_PRIO-1, and SCHED_NORMAL/SCHED_BATCH
> >  * tasks are in the range MAX_RT_PRIO..MAX_PRIO-1. Priority
> >  * values are inverted: lower p->prio value means higher priority.
> >
> > So the new 50 is actually a higher priority than the old 99?
> >
> > Given I'm far from an RT expert, I must be missing something?
> > Thanks!
>
> Ah; you found the confusion ;-)
>
> https://lkml.kernel.org/20190617122448.GA3436@hirez.programming.kicks-ass.net

/me adds /r after org/
Thanks!

> But basically, user-space prio is [1-99], while in-kernel prio is
> [0-98]. The above is user prio (it basically uses the
> sched_setscheduler() syscall).
>
> So 50 really is lower than 99.

IC.

BTW, what about having a #define for MAX_RT_PRIO / 2?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 5/5] spi: Reduce kthread priority

[Enric Balletbo i Serra]

cc'ing Doug as he might be really interested on this patch

On 1/8/19 13:13, Peter Zijlstra wrote:
> The SPI thingies request FIFO-99 by default, reduce this to FIFO-50.
> 

You say below that is not a suitable default but there is any other reason? Did
you observed problems with this?

> FIFO-99 is the very highest priority available to SCHED_FIFO and
> it not a suitable default; it would indicate the SPI work is the
> most important work on the machine.
> 
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: linux-spi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  drivers/platform/chrome/cros_ec_spi.c |    2 +-
>  drivers/spi/spi.c                     |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -706,7 +706,7 @@ static int cros_ec_spi_devm_high_pri_all
>  					   struct cros_ec_spi *ec_spi)
>  {
>  	struct sched_param sched_priority = {
> -		.sched_priority = MAX_RT_PRIO - 1,
> +		.sched_priority = MAX_RT_PRIO / 2,
>  	};
>  	int err;
>  
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1434,7 +1434,7 @@ static void spi_pump_messages(struct kth
>   */
>  static void spi_set_thread_rt(struct spi_controller *ctlr)
>  {
> -	struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
> +	struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
>  
>  	dev_info(&ctlr->dev,
>  		"will run message pump with realtime priority\n");
> 
> 

Re: [PATCH 5/5] spi: Reduce kthread priority

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 01:27:04PM +0200, Enric Balletbo i Serra wrote:
> 
> cc'ing Doug as he might be really interested on this patch
> 
> On 1/8/19 13:13, Peter Zijlstra wrote:
> > The SPI thingies request FIFO-99 by default, reduce this to FIFO-50.
> > 
> 
> You say below that is not a suitable default but there is any other reason? Did
> you observed problems with this?

I didn't observe any problems with it personally. But imagine your
device is used to control something critical, like something leathal,
say a bandsaw. And just as you stick your hand through the laser
guarding it, your SPI device chirps and preempts the task that was about
to pull the emergency break and save your hand.

FIFO-99 is the highest possible prio (for SCHED_FIFO) and by using it
you say your task is the utmost imporant task on the system.

I'm thinking that isn't true 99% of the time, except of course when that
bandsaw emergency break is attached through SPI, but in that case the
admin can very well chrt the prio of this thread.

Re: [PATCH 5/5] spi: Reduce kthread priority

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

On Thu, Aug 01, 2019 at 02:17:18PM +0200, Peter Zijlstra wrote:

> I'm thinking that isn't true 99% of the time, except of course when that
> bandsaw emergency break is attached through SPI, but in that case the
> admin can very well chrt the prio of this thread.

The SPI thread isn't usually RT, it's only made RT if something in the
system asks for it - the reason the ChromeOS people got CCed in is that
some of their embedded controllers are very fragile and need super tight
timing on some of the interactions over their control interface so
they're one of the users here.  Of course everyone is then going to
claim that their usage is the most critical usage in the system, and
they may well even be right, but I do tend to agree that just any old RT
priority is probably a sensible default since for most cases there will
be few if any other RT tasks anyway.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/5] spi: Reduce kthread priority

[Doug Anderson]

Hi,

On Thu, Aug 1, 2019 at 5:35 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Aug 01, 2019 at 02:17:18PM +0200, Peter Zijlstra wrote:
>
> > I'm thinking that isn't true 99% of the time, except of course when that
> > bandsaw emergency break is attached through SPI, but in that case the
> > admin can very well chrt the prio of this thread.
>
> The SPI thread isn't usually RT, it's only made RT if something in the
> system asks for it - the reason the ChromeOS people got CCed in is that
> some of their embedded controllers are very fragile and need super tight
> timing on some of the interactions over their control interface so
> they're one of the users here.  Of course everyone is then going to
> claim that their usage is the most critical usage in the system, and
> they may well even be right, but I do tend to agree that just any old RT
> priority is probably a sensible default since for most cases there will
> be few if any other RT tasks anyway.

For the Chrome OS case I believe that "MAX_RT_PRIO / 2" should be just
fine.  In fact in an earlier version of my work to make CrOS EC work
better at <https://crrev.com/c/1603464> I had said "We'll arbitrarily
pick a priority of "MAX_RT_PRIO / 4 - 1", AKA 24.  This seems to work
fine in practice."  I only switched to "MAX_RT_PRIO - 1" to match the
SPI code.

Mostly we just need to be a bit higher than things that request the
highest non-realtime priority, notably DM Crypt and loopback which
both schedule a bunch of work on the high priority system workqueue.
Those two things in particular seem to want high priority for
performance reasons but not for correctness reasons.  As mentioned
earlier our EC will actually fail transfers if there is too much
delay.

Thus:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Applied "spi: Reduce kthread priority" to the spi tree

[Mark Brown]

The patch

   spi: Reduce kthread priority

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.4

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 4ff13d00ebd5533947b083c66d44da3243a2d559 Mon Sep 17 00:00:00 2001
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu, 1 Aug 2019 13:13:53 +0200
Subject: [PATCH] spi: Reduce kthread priority

The SPI thingies request FIFO-99 by default, reduce this to FIFO-50.

FIFO-99 is the very highest priority available to SCHED_FIFO and
it not a suitable default; it would indicate the SPI work is the
most important work on the machine.

Cc: Benson Leung <bleung@chromium.org>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-spi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20190801111541.917256884@infradead.org
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/platform/chrome/cros_ec_spi.c | 2 +-
 drivers/spi/spi.c                     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 006a8ff64057..714306bc3f79 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -706,7 +706,7 @@ static int cros_ec_spi_devm_high_pri_alloc(struct device *dev,
 					   struct cros_ec_spi *ec_spi)
 {
 	struct sched_param sched_priority = {
-		.sched_priority = MAX_RT_PRIO - 1,
+		.sched_priority = MAX_RT_PRIO / 2,
 	};
 	int err;
 
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9fd7926e80c0..8e83c9567353 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1434,7 +1434,7 @@ static void spi_pump_messages(struct kthread_work *work)
  */
 static void spi_set_thread_rt(struct spi_controller *ctlr)
 {
-	struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
+	struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
 
 	dev_info(&ctlr->dev,
 		"will run message pump with realtime priority\n");
-- 
2.20.1

Re: [PATCH 0/5] Fix FIFO-99 abuse

[Qais Yousef]

On 08/01/19 13:13, Peter Zijlstra wrote:
> I noticed a bunch of kthreads defaulted to FIFO-99, fix them.
> 
> The generic default is FIFO-50, the admin will have to configure the system
> anyway.
> 
> For some the purpose is to be above OTHER and then FIFO-1 really is sufficient.

I was looking in this area too and was thinking of a way to consolidate the
creation of RT/DL tasks in the kernel and the way we set the priority.

Does it make sense to create a new header for RT priorities for kthreads
created in the kernel so that we can easily track and rationale about the
relative priorities of in-kernel RT tasks?

When working in the FW world such a header helped a lot in understanding what
runs at each priority level and how to reason about what priority level makes
sense for a new item. It could be a nice single point of reference; even for
admins.

Cheers

--
Qais Yousef

Re: [PATCH 0/5] Fix FIFO-99 abuse

[Peter Zijlstra]

On Thu, Aug 01, 2019 at 02:17:07PM +0100, Qais Yousef wrote:
> On 08/01/19 13:13, Peter Zijlstra wrote:
> > I noticed a bunch of kthreads defaulted to FIFO-99, fix them.
> > 
> > The generic default is FIFO-50, the admin will have to configure the system
> > anyway.
> > 
> > For some the purpose is to be above OTHER and then FIFO-1 really is sufficient.
> 
> I was looking in this area too and was thinking of a way to consolidate the
> creation of RT/DL tasks in the kernel and the way we set the priority.
> 
> Does it make sense to create a new header for RT priorities for kthreads
> created in the kernel so that we can easily track and rationale about the
> relative priorities of in-kernel RT tasks?
> 
> When working in the FW world such a header helped a lot in understanding what
> runs at each priority level and how to reason about what priority level makes
> sense for a new item. It could be a nice single point of reference; even for
> admins.

Well, SCHED_FIFO is a broken scheduler model; that is, it is
fundamentally incapable of resource management, which is the one thing
an OS really should be doing.

This is of course the reason it is limited to privileged users only.

Worse still; it is fundamentally impossible to compose static priority
workloads. You cannot take two correctly working static prio workloads
and smash them together and still expect them to work.

For this reason 'all' FIFO tasks the kernel creates are basically at:

  MAX_RT_PRIO / 2

The administrator _MUST_ configure the system, the kernel simply doesn't
know enough information to make a sensible choice.

Now, Geert suggested so make make a define for that, but how about we do
something like:

/*
 * ${the above explanation}
 */
int kernel_setscheduler_fifo(struct task_struct *p)
{
	struct sched_param sp = { .sched_priority = MAX_RT_PRIO / 2 };
	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
}

And then take away sched_setscheduler*().

Re: [PATCH 0/5] Fix FIFO-99 abuse

[Thomas Gleixner]

On Fri, 2 Aug 2019, Peter Zijlstra wrote:
> On Thu, Aug 01, 2019 at 02:17:07PM +0100, Qais Yousef wrote:
> > On 08/01/19 13:13, Peter Zijlstra wrote:
> > > I noticed a bunch of kthreads defaulted to FIFO-99, fix them.
> > > 
> > > The generic default is FIFO-50, the admin will have to configure the system
> > > anyway.
> > > 
> > > For some the purpose is to be above OTHER and then FIFO-1 really is sufficient.
> > 
> > I was looking in this area too and was thinking of a way to consolidate the
> > creation of RT/DL tasks in the kernel and the way we set the priority.
> > 
> > Does it make sense to create a new header for RT priorities for kthreads
> > created in the kernel so that we can easily track and rationale about the
> > relative priorities of in-kernel RT tasks?
> > 
> > When working in the FW world such a header helped a lot in understanding what
> > runs at each priority level and how to reason about what priority level makes
> > sense for a new item. It could be a nice single point of reference; even for
> > admins.
> 
> Well, SCHED_FIFO is a broken scheduler model; that is, it is
> fundamentally incapable of resource management, which is the one thing
> an OS really should be doing.
> 
> This is of course the reason it is limited to privileged users only.
> 
> Worse still; it is fundamentally impossible to compose static priority
> workloads. You cannot take two correctly working static prio workloads
> and smash them together and still expect them to work.
> 
> For this reason 'all' FIFO tasks the kernel creates are basically at:
> 
>   MAX_RT_PRIO / 2
> 
> The administrator _MUST_ configure the system, the kernel simply doesn't
> know enough information to make a sensible choice.
> 
> Now, Geert suggested so make make a define for that, but how about we do
> something like:
> 
> /*
>  * ${the above explanation}
>  */
> int kernel_setscheduler_fifo(struct task_struct *p)
> {
> 	struct sched_param sp = { .sched_priority = MAX_RT_PRIO / 2 };
> 	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
> }
> 
> And then take away sched_setscheduler*().

Yes, please.

     tglx

Re: [PATCH 0/5] Fix FIFO-99 abuse

[Qais Yousef]

On 08/02/19 11:32, Peter Zijlstra wrote:
> On Thu, Aug 01, 2019 at 02:17:07PM +0100, Qais Yousef wrote:
> > On 08/01/19 13:13, Peter Zijlstra wrote:
> > > I noticed a bunch of kthreads defaulted to FIFO-99, fix them.
> > > 
> > > The generic default is FIFO-50, the admin will have to configure the system
> > > anyway.
> > > 
> > > For some the purpose is to be above OTHER and then FIFO-1 really is sufficient.
> > 
> > I was looking in this area too and was thinking of a way to consolidate the
> > creation of RT/DL tasks in the kernel and the way we set the priority.
> > 
> > Does it make sense to create a new header for RT priorities for kthreads
> > created in the kernel so that we can easily track and rationale about the
> > relative priorities of in-kernel RT tasks?
> > 
> > When working in the FW world such a header helped a lot in understanding what
> > runs at each priority level and how to reason about what priority level makes
> > sense for a new item. It could be a nice single point of reference; even for
> > admins.
> 
> Well, SCHED_FIFO is a broken scheduler model; that is, it is
> fundamentally incapable of resource management, which is the one thing
> an OS really should be doing.
> 
> This is of course the reason it is limited to privileged users only.
> 
> Worse still; it is fundamentally impossible to compose static priority
> workloads. You cannot take two correctly working static prio workloads
> and smash them together and still expect them to work.
> 
> For this reason 'all' FIFO tasks the kernel creates are basically at:
> 
>   MAX_RT_PRIO / 2
> 
> The administrator _MUST_ configure the system, the kernel simply doesn't
> know enough information to make a sensible choice.
> 
> Now, Geert suggested so make make a define for that, but how about we do
> something like:
> 
> /*
>  * ${the above explanation}
>  */
> int kernel_setscheduler_fifo(struct task_struct *p)
> {
> 	struct sched_param sp = { .sched_priority = MAX_RT_PRIO / 2 };
> 	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
> }
> 
> And then take away sched_setscheduler*().

Yes a somewhat enforced default makes more sense to me. I assume you no longer
want to put the kthreads that just need to be above OTHER in FIFO-1?

While at it, since we will cram all kthreads on the same priority, isn't
a SCHED_RR a better choice now? I think the probability of a clash is pretty
low, but when it happens, shouldn't we try to guarantee some fairness?

--
Qais Yousef

Re: [PATCH 0/5] Fix FIFO-99 abuse

[Peter Zijlstra]

On Fri, Aug 02, 2019 at 11:26:12AM +0100, Qais Yousef wrote:

> Yes a somewhat enforced default makes more sense to me. I assume you no longer
> want to put the kthreads that just need to be above OTHER in FIFO-1?

I'm not sure, maybe, there's not that many of them, but possibly we add
another interface for them.

> While at it, since we will cram all kthreads on the same priority, isn't
> a SCHED_RR a better choice now? I think the probability of a clash is pretty
> low, but when it happens, shouldn't we try to guarantee some fairness?

It's never been a problem, and aside from these few straggler threads,
everybody has effectively been there already for years, so if it were a
problem someone would've complained by now.

Also; like said before, the admin had better configure.

Also also, RR-SMP is actually broken (and nobody has cared enough to
bother fixing it).

Re: [PATCH 0/5] Fix FIFO-99 abuse

[Qais Yousef]

On 08/02/19 14:41, Peter Zijlstra wrote:
> On Fri, Aug 02, 2019 at 11:26:12AM +0100, Qais Yousef wrote:
> 
> > Yes a somewhat enforced default makes more sense to me. I assume you no longer
> > want to put the kthreads that just need to be above OTHER in FIFO-1?
> 
> I'm not sure, maybe, there's not that many of them, but possibly we add
> another interface for them.

By the way, did you see this one which is set to priority 16?

https://elixir.bootlin.com/linux/v5.3-rc2/source/drivers/gpu/drm/msm/msm_drv.c#L523

> 
> > While at it, since we will cram all kthreads on the same priority, isn't
> > a SCHED_RR a better choice now? I think the probability of a clash is pretty
> > low, but when it happens, shouldn't we try to guarantee some fairness?
> 
> It's never been a problem, and aside from these few straggler threads,
> everybody has effectively been there already for years, so if it were a
> problem someone would've complained by now.

Usually they can run on enough CPUs so a real clash is definitely hard.

I'm trying to collect data on that, if I find something interesting I'll share
it.

> 
> Also; like said before, the admin had better configure.

I agree. But I don't think an 'admin' is an easily defined entity for all
systems. On mobile, is it the SoC vendor, Android framework, or the
handset/platform vendor/integrator?

In a *real* realtime system I think things are better defined. But usage of RT
tasks on generic systems is the confusing part. There's no real ownership and
things are more ad-hoc.

> 
> Also also, RR-SMP is actually broken (and nobody has cared enough to
> bother fixing it).

If you can give me enough pointers to understand the problem I might be able to
bother with it :-)

Thanks

--
Qais Yousef

Re: [PATCH 0/5] Fix FIFO-99 abuse

[Peter Zijlstra]

On Fri, Aug 02, 2019 at 03:08:54PM +0100, Qais Yousef wrote:
> On 08/02/19 14:41, Peter Zijlstra wrote:
> > On Fri, Aug 02, 2019 at 11:26:12AM +0100, Qais Yousef wrote:
> > 
> > > Yes a somewhat enforced default makes more sense to me. I assume you no longer
> > > want to put the kthreads that just need to be above OTHER in FIFO-1?
> > 
> > I'm not sure, maybe, there's not that many of them, but possibly we add
> > another interface for them.
> 
> By the way, did you see this one which is set to priority 16?
> 
> https://elixir.bootlin.com/linux/v5.3-rc2/source/drivers/gpu/drm/msm/msm_drv.c#L523

I did, I ignored it because it wasn't 99 or something silly like that,
but I'd definitely mop it up when doing the proposed.

> > Also; like said before, the admin had better configure.
> 
> I agree. But I don't think an 'admin' is an easily defined entity for all
> systems. On mobile, is it the SoC vendor, Android framework, or the
> handset/platform vendor/integrator?

Mostly Android I suspect, but if SoC specific drivers have RT threads,
it's their responsibility to integrate properly with the rest of Android
of course.

> > Also also, RR-SMP is actually broken (and nobody has cared enough to
> > bother fixing it).
> 
> If you can give me enough pointers to understand the problem I might be able to
> bother with it :-)

So the push-pull balancer we have (designed for FIFO but also applied to
RR) will only move a task if the destination CPU has a lower prio.  In
the case where one CPU has 3 tasks and the other 1, and they're all the
same prio, it does nothing.  For FIFO that is fine, for RR, not so much.

Because then the one CPU will RR between 3 tasks, giving each task
1/3rd, while the other will only run the one task.

Re: [PATCH 0/5] Fix FIFO-99 abuse

[Qais Yousef]

On 08/02/19 16:33, Peter Zijlstra wrote:
> > > Also also, RR-SMP is actually broken (and nobody has cared enough to
> > > bother fixing it).
> > 
> > If you can give me enough pointers to understand the problem I might be able to
> > bother with it :-)
> 
> So the push-pull balancer we have (designed for FIFO but also applied to
> RR) will only move a task if the destination CPU has a lower prio.  In
> the case where one CPU has 3 tasks and the other 1, and they're all the
> same prio, it does nothing.  For FIFO that is fine, for RR, not so much.
> 
> Because then the one CPU will RR between 3 tasks, giving each task
> 1/3rd, while the other will only run the one task.

Okay so what we need to do is keep a count of number of RR tasks on each CPU
(which we already do IIRC) and take that into account in
find_loweset_rq()/cpupri_find().

Let me see if I can create a test case then hack something.

Thanks

--
Qais Yousef