[PATCH 00/23] sched: Remove FIFO priorities from modules

[PATCH 00/23] sched: Remove FIFO priorities from modules

[Peter Zijlstra]

After having seen yet another patch that adds a FIFO kernel thread at a random
priority, I bit the bullet and implemented the suggestion done a while ago:

  Flat out remove the ability to set a specific FIFO priority

As argued in the patches themselves, the kernel has no clue what actual
priority it should use for various things, so it is useless (or worse, counter
productive) to even try.

[PATCH 01/23] sched: Provide sched_set_fifo()

[Peter Zijlstra]

SCHED_FIFO (or any static priority scheduler) is a broken scheduler
model; it is fundamentally incapable of resource management, the one
thing an OS is actually supposed to do.

It is impossible to compose static priority workloads. One cannot take
two well designed and functional static priority workloads and mash
them together and still expect them to work.

Therefore it doesn't make sense to expose the priority field; the
kernel is fundamentally incapable of setting a sensible value, it
needs systems knowledge that it doesn't have.

Take away sched_setschedule() / sched_setattr() from modules and
replace them with:

  - sched_set_fifo(p); create a FIFO task (at prio 50)
  - sched_set_fifo_low(p); create a task higher than NORMAL,
	which ends up being a FIFO task at prio 1.
  - sched_set_normal(p, nice); (re)set the task to normal

This stops the proliferation of randomly chosen, and irrelevant, FIFO
priorities that dont't really mean anything anyway.

The system administrator/integrator, whoever has insight into the
actual system design and requirements (userspace) can set-up
appropriate priorities if and when needed.

Cc: airlied@redhat.com
Cc: alexander.deucher@amd.com
Cc: awalls@md.metrocast.net
Cc: axboe@kernel.dk
Cc: broonie@kernel.org
Cc: daniel.lezcano@linaro.org
Cc: gregkh@linuxfoundation.org
Cc: hannes@cmpxchg.org
Cc: herbert@gondor.apana.org.au
Cc: hverkuil@xs4all.nl
Cc: john.stultz@linaro.org
Cc: nico@fluxnic.net
Cc: paulmck@kernel.org
Cc: rafael.j.wysocki@intel.com
Cc: rmk+kernel@arm.linux.org.uk
Cc: sudeep.holla@arm.com
Cc: tglx@linutronix.de
Cc: ulf.hansson@linaro.org
Cc: wim@linux-watchdog.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/sched.h |    3 +++
 kernel/sched/core.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1631,6 +1631,9 @@ extern int idle_cpu(int cpu);
 extern int available_idle_cpu(int cpu);
 extern int sched_setscheduler(struct task_struct *, int, const struct sched_param *);
 extern int sched_setscheduler_nocheck(struct task_struct *, int, const struct sched_param *);
+extern int sched_set_fifo(struct task_struct *p);
+extern int sched_set_fifo_low(struct task_struct *p);
+extern int sched_set_normal(struct task_struct *p, int nice);
 extern int sched_setattr(struct task_struct *, const struct sched_attr *);
 extern int sched_setattr_nocheck(struct task_struct *, const struct sched_attr *);
 extern struct task_struct *idle_task(int cpu);
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5055,6 +5055,8 @@ static int _sched_setscheduler(struct ta
  * @policy: new policy.
  * @param: structure containing the new RT priority.
  *
+ * Use sched_set_fifo(), read its comment.
+ *
  * Return: 0 on success. An error code otherwise.
  *
  * NOTE that the task may be already dead.
@@ -5097,6 +5099,51 @@ int sched_setscheduler_nocheck(struct ta
 }
 EXPORT_SYMBOL_GPL(sched_setscheduler_nocheck);
 
+/*
+ * 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.
+ */
+int sched_set_fifo(struct task_struct *p)
+{
+	struct sched_param sp = { .sched_priority = MAX_RT_PRIO / 2 };
+	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
+}
+EXPORT_SYMBOL_GPL(sched_set_fifo);
+
+/*
+ * For when you don't much care about FIFO, but want to be above SCHED_NORMAL.
+ */
+int sched_set_fifo_low(struct task_struct *p)
+{
+	struct sched_param sp = { .sched_priority = 1 };
+	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
+}
+EXPORT_SYMBOL_GPL(sched_set_fifo_low);
+
+int sched_set_normal(struct task_struct *p, int nice)
+{
+	struct sched_attr attr = {
+		.sched_policy = SCHED_NORMAL,
+		.sched_nice = nice,
+	};
+	return sched_setattr_nocheck(p, &attr);
+}
+EXPORT_SYMBOL_GPL(sched_set_normal);
+
 static int
 do_sched_setscheduler(pid_t pid, int policy, struct sched_param __user *param)
 {

[PATCH 02/23] sched,bL_switcher: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

In this case, use fifo_low, because it only cares about being above
SCHED_NORMAL. Effectively no change in behaviour.

Cc: nico@fluxnic.net
Cc: rmk+kernel@arm.linux.org.uk
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 arch/arm/common/bL_switcher.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/arch/arm/common/bL_switcher.c
+++ b/arch/arm/common/bL_switcher.c
@@ -270,12 +270,11 @@ static struct bL_thread bL_threads[NR_CP
 static int bL_switcher_thread(void *arg)
 {
 	struct bL_thread *t = arg;
-	struct sched_param param = { .sched_priority = 1 };
 	int cluster;
 	bL_switch_completion_handler completer;
 	void *completer_cookie;
 
-	sched_setscheduler_nocheck(current, SCHED_FIFO, &param);
+	sched_set_fifo_low(current);
 	complete(&t->started);
 
 	do {

[PATCH 03/23] sched,crypto: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Use sched_set_fifo() to request SCHED_FIFO and delegate
actual priority selection to userspace. Effectively no change in
behaviour.

Cc: herbert@gondor.apana.org.au
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 crypto/crypto_engine.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -396,7 +396,6 @@ 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 / 2 };
 	struct crypto_engine *engine;
 
 	if (!dev)
@@ -428,7 +427,7 @@ struct crypto_engine *crypto_engine_allo
 
 	if (engine->rt) {
 		dev_info(dev, "will run requests pump with realtime priority\n");
-		sched_setscheduler(engine->kworker->task, SCHED_FIFO, &param);
+		sched_set_fifo(engine->kworker->task);
 	}
 
 	return engine;

[PATCH 04/23] sched,acpi_pad: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

In this case, use fifo_low, because it only cares about being above
SCHED_NORMAL. Effectively no change in behaviour.

XXX: this driver is still complete crap; why isn't it using proper
idle injection or at the very least play_idle() ?

Cc: rafael.j.wysocki@intel.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/acpi/acpi_pad.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/drivers/acpi/acpi_pad.c
+++ b/drivers/acpi/acpi_pad.c
@@ -136,12 +136,11 @@ static unsigned int idle_pct = 5; /* per
 static unsigned int round_robin_time = 1; /* second */
 static int power_saving_thread(void *data)
 {
-	struct sched_param param = {.sched_priority = 1};
 	int do_sleep;
 	unsigned int tsk_index = (unsigned long)data;
 	u64 last_jiffies = 0;
 
-	sched_setscheduler(current, SCHED_RR, &param);
+	sched_set_fifo_low(current);
 
 	while (!kthread_should_stop()) {
 		unsigned long expire_time;

[PATCH 05/23] sched,drbd: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

In this case, use fifo_low, because it only cares about being above
SCHED_NORMAL. Effectively changes prio from 2 to 1.

Cc: axboe@kernel.dk
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/block/drbd/drbd_receiver.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/drivers/block/drbd/drbd_receiver.c
+++ b/drivers/block/drbd/drbd_receiver.c
@@ -6020,9 +6020,8 @@ int drbd_ack_receiver(struct drbd_thread
 	unsigned int header_size = drbd_header_size(connection);
 	int expect   = header_size;
 	bool ping_timeout_active = false;
-	struct sched_param param = { .sched_priority = 2 };
 
-	rv = sched_setscheduler(current, SCHED_RR, &param);
+	rv = sched_set_fifo_low(current);
 	if (rv < 0)
 		drbd_err(connection, "drbd_ack_receiver: ERROR set priority, ret=%d\n", rv);
 

[PATCH 06/23] sched,psci: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively changes prio from 99 to 50.

XXX this thing is horrific, it basically open-codes a stop-machine and
idle.

Cc: daniel.lezcano@linaro.org
Cc: sudeep.holla@arm.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/firmware/psci/psci_checker.c |   11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

--- a/drivers/firmware/psci/psci_checker.c
+++ b/drivers/firmware/psci/psci_checker.c
@@ -272,7 +272,6 @@ static int suspend_test_thread(void *arg
 {
 	int cpu = (long)arg;
 	int i, nb_suspend = 0, nb_shallow_sleep = 0, nb_err = 0;
-	struct sched_param sched_priority = { .sched_priority = MAX_RT_PRIO-1 };
 	struct cpuidle_device *dev;
 	struct cpuidle_driver *drv;
 	/* No need for an actual callback, we just want to wake up the CPU. */
@@ -282,9 +281,8 @@ static int suspend_test_thread(void *arg
 	wait_for_completion(&suspend_threads_started);
 
 	/* Set maximum priority to preempt all other threads on this CPU. */
-	if (sched_setscheduler_nocheck(current, SCHED_FIFO, &sched_priority))
-		pr_warn("Failed to set suspend thread scheduler on CPU %d\n",
-			cpu);
+	if (sched_set_fifo(current))
+		pr_warn("Failed to set suspend thread scheduler on CPU %d\n", cpu);
 
 	dev = this_cpu_read(cpuidle_devices);
 	drv = cpuidle_get_cpu_driver(dev);
@@ -349,11 +347,6 @@ static int suspend_test_thread(void *arg
 	if (atomic_dec_return_relaxed(&nb_active_threads) == 0)
 		complete(&suspend_threads_done);
 
-	/* Give up on RT scheduling and wait for termination. */
-	sched_priority.sched_priority = 0;
-	if (sched_setscheduler_nocheck(current, SCHED_NORMAL, &sched_priority))
-		pr_warn("Failed to set suspend thread scheduler on CPU %d\n",
-			cpu);
 	for (;;) {
 		/* Needs to be set first to avoid missing a wakeup. */
 		set_current_state(TASK_INTERRUPTIBLE);

[PATCH 07/23] sched,msm: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Use sched_set_fifo(); Effectively changes prio from 16 to 50.

Cc: airlied@redhat.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -388,7 +388,6 @@ static int msm_drm_init(struct device *d
 	struct msm_kms *kms;
 	struct msm_mdss *mdss;
 	int ret, i;
-	struct sched_param param;
 
 	ddev = drm_dev_alloc(drv, dev);
 	if (IS_ERR(ddev)) {
@@ -494,12 +493,6 @@ static int msm_drm_init(struct device *d
 	ddev->mode_config.funcs = &mode_config_funcs;
 	ddev->mode_config.helper_private = &mode_config_helper_funcs;
 
-	/**
-	 * this priority was found during empiric testing to have appropriate
-	 * realtime scheduling to process display updates and interact with
-	 * other real time and normal priority task
-	 */
-	param.sched_priority = 16;
 	for (i = 0; i < priv->num_crtcs; i++) {
 		/* initialize event thread */
 		priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
@@ -515,8 +508,7 @@ static int msm_drm_init(struct device *d
 			goto err_msm_uninit;
 		}
 
-		ret = sched_setscheduler(priv->event_thread[i].thread,
-					 SCHED_FIFO, &param);
+		ret = sched_set_fifo(priv->event_thread[i].thread);
 		if (ret)
 			dev_warn(dev, "event_thread set priority failed:%d\n",
 				 ret);

[PATCH 08/23] sched,drm/scheduler: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

In this case, use fifo_low, because it only cares about being above
SCHED_NORMAL. Effectively no change in behaviour.

Cc: alexander.deucher@amd.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/gpu/drm/scheduler/sched_main.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -760,11 +760,10 @@ static bool drm_sched_blocked(struct drm
  */
 static int drm_sched_main(void *param)
 {
-	struct sched_param sparam = {.sched_priority = 1};
 	struct drm_gpu_scheduler *sched = (struct drm_gpu_scheduler *)param;
 	int r;
 
-	sched_setscheduler(current, SCHED_FIFO, &sparam);
+	sched_set_fifo_low(current);
 
 	while (!kthread_should_stop()) {
 		struct drm_sched_entity *entity = NULL;

[PATCH 09/23] sched,ivtv: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively changes from 99 to 50.

Cc: hverkuil@xs4all.nl
Cc: awalls@md.metrocast.net
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/media/pci/ivtv/ivtv-driver.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

--- a/drivers/media/pci/ivtv/ivtv-driver.c
+++ b/drivers/media/pci/ivtv/ivtv-driver.c
@@ -737,8 +737,6 @@ static void ivtv_process_options(struct
  */
 static int ivtv_init_struct1(struct ivtv *itv)
 {
-	struct sched_param param = { .sched_priority = 99 };
-
 	itv->base_addr = pci_resource_start(itv->pdev, 0);
 	itv->enc_mbox.max_mbox = 2; /* the encoder has 3 mailboxes (0-2) */
 	itv->dec_mbox.max_mbox = 1; /* the decoder has 2 mailboxes (0-1) */
@@ -758,7 +756,7 @@ static int ivtv_init_struct1(struct ivtv
 		return -1;
 	}
 	/* must use the FIFO scheduler as it is realtime sensitive */
-	sched_setscheduler(itv->irq_worker_task, SCHED_FIFO, &param);
+	sched_set_fifo(itv->irq_worker_task);
 
 	kthread_init_work(&itv->irq_work, ivtv_irq_work_handler);
 

[PATCH 10/23] sched,mmc: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

In this case, use fifo_low, because it only cares about being above
SCHED_NORMAL. Effectively no change in behaviour.

Cc: ulf.hansson@linaro.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/mmc/core/sdio_irq.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -139,11 +139,10 @@ EXPORT_SYMBOL_GPL(sdio_signal_irq);
 static int sdio_irq_thread(void *_host)
 {
 	struct mmc_host *host = _host;
-	struct sched_param param = { .sched_priority = 1 };
 	unsigned long period, idle_period;
 	int ret;
 
-	sched_setscheduler(current, SCHED_FIFO, &param);
+	sched_set_fifo_low(current);
 
 	/*
 	 * We want to allow for SDIO cards to work even on non SDIO

[PATCH 11/23] sched,spi: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

No effective change.

Cc: broonie@kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/platform/chrome/cros_ec_spi.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -709,9 +709,6 @@ static void cros_ec_spi_high_pri_release
 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 / 2,
-	};
 	int err;
 
 	ec_spi->high_pri_worker =
@@ -728,8 +725,7 @@ static int cros_ec_spi_devm_high_pri_all
 	if (err)
 		return err;
 
-	err = sched_setscheduler_nocheck(ec_spi->high_pri_worker->task,
-					 SCHED_FIFO, &sched_priority);
+	err = sched_set_fifo(ec_spi->high_pri_worker->task);
 	if (err)
 		dev_err(dev, "Can't set cros_ec high pri priority: %d\n", err);
 	return err;
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1589,11 +1589,9 @@ EXPORT_SYMBOL_GPL(spi_take_timestamp_pos
  */
 static void spi_set_thread_rt(struct spi_controller *ctlr)
 {
-	struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
-
 	dev_info(&ctlr->dev,
 		"will run message pump with realtime priority\n");
-	sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
+	sched_set_fifo(ctlr->kworker_task);
 }
 
 static int spi_init_queue(struct spi_controller *ctlr)

[PATCH 12/23] sched,powercap: Convert to sched_set_fifo*()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively no change.

Cc: daniel.lezcano@linaro.org
Cc: rafael.j.wysocki@intel.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/powercap/idle_inject.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

--- a/drivers/powercap/idle_inject.c
+++ b/drivers/powercap/idle_inject.c
@@ -255,9 +255,7 @@ void idle_inject_stop(struct idle_inject
  */
 static void idle_inject_setup(unsigned int cpu)
 {
-	struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO / 2 };
-
-	sched_setscheduler(current, SCHED_FIFO, &param);
+	sched_set_fifo(current);
 }
 
 /**

[PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Peter Zijlstra]

In an attempt to take away sched_setscheduler() from modules, change
this into sched_set_normal(.nice = 19).

Cc: john.stultz@linaro.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/staging/android/ion/ion_heap.c |    3 ---
 1 file changed, 3 deletions(-)

--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -244,8 +244,6 @@ static int ion_heap_deferred_free(void *
 
 int ion_heap_init_deferred_free(struct ion_heap *heap)
 {
-	struct sched_param param = { .sched_priority = 0 };
-
 	INIT_LIST_HEAD(&heap->free_list);
 	init_waitqueue_head(&heap->waitqueue);
 	heap->task = kthread_run(ion_heap_deferred_free, heap,
@@ -255,7 +253,7 @@ int ion_heap_init_deferred_free(struct i
 		       __func__);
 		return PTR_ERR_OR_ZERO(heap->task);
 	}
-	sched_setscheduler(heap->task, SCHED_IDLE, &param);
+	sched_set_normal(heap->task, 19);
 
 	return 0;
 }

[PATCH 14/23] sched,powerclamp: Convert to sched_set_fifo()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively no change.

Cc: rafael.j.wysocki@intel.com
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/thermal/intel/intel_powerclamp.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

--- a/drivers/thermal/intel/intel_powerclamp.c
+++ b/drivers/thermal/intel/intel_powerclamp.c
@@ -70,9 +70,6 @@ static unsigned int control_cpu; /* The
 				  */
 static bool clamping;
 
-static const struct sched_param sparam = {
-	.sched_priority = MAX_USER_RT_PRIO / 2,
-};
 struct powerclamp_worker_data {
 	struct kthread_worker *worker;
 	struct kthread_work balancing_work;
@@ -488,7 +485,7 @@ static void start_power_clamp_worker(uns
 	w_data->cpu = cpu;
 	w_data->clamping = true;
 	set_bit(cpu, cpu_clamping_mask);
-	sched_setscheduler(worker->task, SCHED_FIFO, &sparam);
+	sched_set_fifo(worker->task);
 	kthread_init_work(&w_data->balancing_work, clamp_balancing_func);
 	kthread_init_delayed_work(&w_data->idle_injection_work,
 				  clamp_idle_injection_func);

[PATCH 15/23] sched,serial: Convert to sched_set_fifo()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively no change.

Cc: gregkh@linuxfoundation.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/tty/serial/sc16is7xx.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1178,7 +1178,6 @@ static int sc16is7xx_probe(struct device
 			   const struct sc16is7xx_devtype *devtype,
 			   struct regmap *regmap, int irq, unsigned long flags)
 {
-	struct sched_param sched_param = { .sched_priority = MAX_RT_PRIO / 2 };
 	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
 	u32 uartclk = 0;
 	int i, ret;
@@ -1228,7 +1227,7 @@ static int sc16is7xx_probe(struct device
 		ret = PTR_ERR(s->kworker_task);
 		goto out_clk;
 	}
-	sched_setscheduler(s->kworker_task, SCHED_FIFO, &sched_param);
+	sched_set_fifo(s->kworker_task);
 
 #ifdef CONFIG_GPIOLIB
 	if (devtype->nr_gpio) {

[PATCH 16/23] sched,watchdog: Convert to sched_set_fifo()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively changes prio from 99 to 50.

Cc: wim@linux-watchdog.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/watchdog/watchdog_dev.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -1144,14 +1144,13 @@ void watchdog_dev_unregister(struct watc
 int __init watchdog_dev_init(void)
 {
 	int err;
-	struct sched_param param = {.sched_priority = MAX_RT_PRIO - 1,};
 
 	watchdog_kworker = kthread_create_worker(0, "watchdogd");
 	if (IS_ERR(watchdog_kworker)) {
 		pr_err("Failed to create watchdog kworker\n");
 		return PTR_ERR(watchdog_kworker);
 	}
-	sched_setscheduler(watchdog_kworker->task, SCHED_FIFO, &param);
+	sched_set_fifo(watchdog_kworker->task);
 
 	err = class_register(&watchdog_class);
 	if (err < 0) {

[PATCH 17/23] sched,irq: Convert to sched_set_fifo()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively no change.

Cc: tglx@linutronix.de
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/irq/manage.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1271,9 +1271,6 @@ static int
 setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary)
 {
 	struct task_struct *t;
-	struct sched_param param = {
-		.sched_priority = MAX_USER_RT_PRIO/2,
-	};
 
 	if (!secondary) {
 		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
@@ -1281,13 +1278,12 @@ setup_irq_thread(struct irqaction *new,
 	} else {
 		t = kthread_create(irq_thread, new, "irq/%d-s-%s", irq,
 				   new->name);
-		param.sched_priority -= 1;
 	}
 
 	if (IS_ERR(t))
 		return PTR_ERR(t);
 
-	sched_setscheduler_nocheck(t, SCHED_FIFO, &param);
+	sched_set_fifo(t);
 
 	/*
 	 * We keep the reference to the task struct even if

[PATCH 18/23] sched,locktorture: Convert to sched_set_fifo()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively changes prio from 99 to 50.

Cc: paulmck@kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/locktorture.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -436,8 +436,6 @@ static int torture_rtmutex_lock(void) __
 
 static void torture_rtmutex_boost(struct torture_random_state *trsp)
 {
-	int policy;
-	struct sched_param param;
 	const unsigned int factor = 50000; /* yes, quite arbitrary */
 
 	if (!rt_task(current)) {
@@ -448,8 +446,7 @@ static void torture_rtmutex_boost(struct
 		 */
 		if (trsp && !(torture_random(trsp) %
 			      (cxt.nrealwriters_stress * factor))) {
-			policy = SCHED_FIFO;
-			param.sched_priority = MAX_RT_PRIO - 1;
+			sched_set_fifo(current);
 		} else /* common case, do nothing */
 			return;
 	} else {
@@ -462,13 +459,10 @@ static void torture_rtmutex_boost(struct
 		 */
 		if (!trsp || !(torture_random(trsp) %
 			       (cxt.nrealwriters_stress * factor * 2))) {
-			policy = SCHED_NORMAL;
-			param.sched_priority = 0;
+			sched_set_normal(current, 0);
 		} else /* common case, do nothing */
 			return;
 	}
-
-	sched_setscheduler_nocheck(current, policy, &param);
 }
 
 static void torture_rtmutex_delay(struct torture_random_state *trsp)

[PATCH 19/23] sched,rcuperf: Convert to sched_set_fifo_low()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively no change.

Cc: paulmck@kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/rcu/rcuperf.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

--- a/kernel/rcu/rcuperf.c
+++ b/kernel/rcu/rcuperf.c
@@ -353,7 +353,6 @@ rcu_perf_writer(void *arg)
 	int i_max;
 	long me = (long)arg;
 	struct rcu_head *rhp = NULL;
-	struct sched_param sp;
 	bool started = false, done = false, alldone = false;
 	u64 t;
 	u64 *wdp;
@@ -362,8 +361,7 @@ rcu_perf_writer(void *arg)
 	VERBOSE_PERFOUT_STRING("rcu_perf_writer task started");
 	WARN_ON(!wdpp);
 	set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids));
-	sp.sched_priority = 1;
-	sched_setscheduler_nocheck(current, SCHED_FIFO, &sp);
+	sched_set_fifo_low(current);
 
 	if (holdoff)
 		schedule_timeout_uninterruptible(holdoff * HZ);
@@ -419,9 +417,7 @@ rcu_perf_writer(void *arg)
 			started = true;
 		if (!done && i >= MIN_MEAS) {
 			done = true;
-			sp.sched_priority = 0;
-			sched_setscheduler_nocheck(current,
-						   SCHED_NORMAL, &sp);
+			sched_set_normal(current, 0);
 			pr_alert("%s%s rcu_perf_writer %ld has %d measurements\n",
 				 perf_type, PERF_FLAG, me, MIN_MEAS);
 			if (atomic_inc_return(&n_rcu_perf_writer_finished) >=

[PATCH 20/23] sched,rcutorture: Convert to sched_set_fifo_low()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively no change.

Cc: paulmck@kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/rcu/rcutorture.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -802,13 +802,11 @@ static int rcu_torture_boost(void *arg)
 	unsigned long endtime;
 	unsigned long oldstarttime;
 	struct rcu_boost_inflight rbi = { .inflight = 0 };
-	struct sched_param sp;
 
 	VERBOSE_TOROUT_STRING("rcu_torture_boost started");
 
 	/* Set real-time priority. */
-	sp.sched_priority = 1;
-	if (sched_setscheduler(current, SCHED_FIFO, &sp) < 0) {
+	if (sched_set_fifo_low(current) < 0) {
 		VERBOSE_TOROUT_STRING("rcu_torture_boost RT prio failed!");
 		n_rcu_torture_boost_rterror++;
 	}

[PATCH 21/23] sched,psi: Convert to sched_set_fifo_low()

[Peter Zijlstra]

Because SCHED_FIFO is a broken scheduler model (see previous patches)
take away the priority field, the kernel can't possibly make an
informed decision.

Effectively no change.

Cc: hannes@cmpxchg.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/psi.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -1100,9 +1100,6 @@ struct psi_trigger *psi_trigger_create(s
 	mutex_lock(&group->trigger_lock);
 
 	if (!rcu_access_pointer(group->poll_kworker)) {
-		struct sched_param param = {
-			.sched_priority = 1,
-		};
 		struct kthread_worker *kworker;
 
 		kworker = kthread_create_worker(0, "psimon");
@@ -1111,7 +1108,7 @@ struct psi_trigger *psi_trigger_create(s
 			mutex_unlock(&group->trigger_lock);
 			return ERR_CAST(kworker);
 		}
-		sched_setscheduler_nocheck(kworker->task, SCHED_FIFO, &param);
+		sched_set_fifo_low(kworker->task);
 		kthread_init_delayed_work(&group->poll_work,
 				psi_poll_work);
 		rcu_assign_pointer(group->poll_kworker, kworker);

[PATCH 22/23] sched: Remove sched_setscheduler*() EXPORTs

[Peter Zijlstra]

Now that nothing (modular) still uses sched_setscheduler(), remove the
exports.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/core.c |    3 ---
 1 file changed, 3 deletions(-)

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5066,13 +5066,11 @@ int sched_setscheduler(struct task_struc
 {
 	return _sched_setscheduler(p, policy, param, true);
 }
-EXPORT_SYMBOL_GPL(sched_setscheduler);
 
 int sched_setattr(struct task_struct *p, const struct sched_attr *attr)
 {
 	return __sched_setscheduler(p, attr, true, true);
 }
-EXPORT_SYMBOL_GPL(sched_setattr);
 
 int sched_setattr_nocheck(struct task_struct *p, const struct sched_attr *attr)
 {
@@ -5097,7 +5095,6 @@ int sched_setscheduler_nocheck(struct ta
 {
 	return _sched_setscheduler(p, policy, param, false);
 }
-EXPORT_SYMBOL_GPL(sched_setscheduler_nocheck);
 
 /*
  * SCHED_FIFO is a broken scheduler model; that is, it is fundamentally

[PATCH 23/23] sched: Remove sched_set_*() return value

[Peter Zijlstra]

Ingo suggested that since the new sched_set_*() functions are
implemented using the 'nocheck' variants, they really shouldn't ever
fail, so remove the return value.

Cc: axboe@kernel.dk
Cc: daniel.lezcano@linaro.org
Cc: sudeep.holla@arm.com
Cc: airlied@redhat.com
Cc: broonie@kernel.org
Cc: paulmck@kernel.org
Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/block/drbd/drbd_receiver.c    |    4 +---
 drivers/firmware/psci/psci_checker.c  |    3 +--
 drivers/gpu/drm/msm/msm_drv.c         |    5 +----
 drivers/platform/chrome/cros_ec_spi.c |    7 +++----
 include/linux/sched.h                 |    6 +++---
 kernel/rcu/rcutorture.c               |    5 +----
 kernel/sched/core.c                   |   12 ++++++------
 7 files changed, 16 insertions(+), 26 deletions(-)

--- a/drivers/block/drbd/drbd_receiver.c
+++ b/drivers/block/drbd/drbd_receiver.c
@@ -6021,9 +6021,7 @@ int drbd_ack_receiver(struct drbd_thread
 	int expect   = header_size;
 	bool ping_timeout_active = false;
 
-	rv = sched_set_fifo_low(current);
-	if (rv < 0)
-		drbd_err(connection, "drbd_ack_receiver: ERROR set priority, ret=%d\n", rv);
+	sched_set_fifo_low(current);
 
 	while (get_t_state(thi) == RUNNING) {
 		drbd_thread_current_set_cpu(thi);
--- a/drivers/firmware/psci/psci_checker.c
+++ b/drivers/firmware/psci/psci_checker.c
@@ -281,8 +281,7 @@ static int suspend_test_thread(void *arg
 	wait_for_completion(&suspend_threads_started);
 
 	/* Set maximum priority to preempt all other threads on this CPU. */
-	if (sched_set_fifo(current))
-		pr_warn("Failed to set suspend thread scheduler on CPU %d\n", cpu);
+	sched_set_fifo(current);
 
 	dev = this_cpu_read(cpuidle_devices);
 	drv = cpuidle_get_cpu_driver(dev);
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -508,10 +508,7 @@ static int msm_drm_init(struct device *d
 			goto err_msm_uninit;
 		}
 
-		ret = sched_set_fifo(priv->event_thread[i].thread);
-		if (ret)
-			dev_warn(dev, "event_thread set priority failed:%d\n",
-				 ret);
+		sched_set_fifo(priv->event_thread[i].thread);
 	}
 
 	ret = drm_vblank_init(ddev, priv->num_crtcs);
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -725,10 +725,9 @@ static int cros_ec_spi_devm_high_pri_all
 	if (err)
 		return err;
 
-	err = sched_set_fifo(ec_spi->high_pri_worker->task);
-	if (err)
-		dev_err(dev, "Can't set cros_ec high pri priority: %d\n", err);
-	return err;
+	sched_set_fifo(ec_spi->high_pri_worker->task);
+
+	return 0;
 }
 
 static int cros_ec_spi_probe(struct spi_device *spi)
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1631,9 +1631,9 @@ extern int idle_cpu(int cpu);
 extern int available_idle_cpu(int cpu);
 extern int sched_setscheduler(struct task_struct *, int, const struct sched_param *);
 extern int sched_setscheduler_nocheck(struct task_struct *, int, const struct sched_param *);
-extern int sched_set_fifo(struct task_struct *p);
-extern int sched_set_fifo_low(struct task_struct *p);
-extern int sched_set_normal(struct task_struct *p, int nice);
+extern void sched_set_fifo(struct task_struct *p);
+extern void sched_set_fifo_low(struct task_struct *p);
+extern void sched_set_normal(struct task_struct *p, int nice);
 extern int sched_setattr(struct task_struct *, const struct sched_attr *);
 extern int sched_setattr_nocheck(struct task_struct *, const struct sched_attr *);
 extern struct task_struct *idle_task(int cpu);
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -806,10 +806,7 @@ static int rcu_torture_boost(void *arg)
 	VERBOSE_TOROUT_STRING("rcu_torture_boost started");
 
 	/* Set real-time priority. */
-	if (sched_set_fifo_low(current) < 0) {
-		VERBOSE_TOROUT_STRING("rcu_torture_boost RT prio failed!");
-		n_rcu_torture_boost_rterror++;
-	}
+	sched_set_fifo_low(current);
 
 	init_rcu_head_on_stack(&rbi.rcu);
 	/* Each pass through the following loop does one boost-test cycle. */
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5114,30 +5114,30 @@ int sched_setscheduler_nocheck(struct ta
  * The administrator _MUST_ configure the system, the kernel simply doesn't
  * know enough information to make a sensible choice.
  */
-int sched_set_fifo(struct task_struct *p)
+void sched_set_fifo(struct task_struct *p)
 {
 	struct sched_param sp = { .sched_priority = MAX_RT_PRIO / 2 };
-	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
+	WARN_ON_ONCE(sched_setscheduler_nocheck(p, SCHED_FIFO, &sp) != 0);
 }
 EXPORT_SYMBOL_GPL(sched_set_fifo);
 
 /*
  * For when you don't much care about FIFO, but want to be above SCHED_NORMAL.
  */
-int sched_set_fifo_low(struct task_struct *p)
+void sched_set_fifo_low(struct task_struct *p)
 {
 	struct sched_param sp = { .sched_priority = 1 };
-	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
+	WARN_ON_ONCE(sched_setscheduler_nocheck(p, SCHED_FIFO, &sp) != 0);
 }
 EXPORT_SYMBOL_GPL(sched_set_fifo_low);
 
-int sched_set_normal(struct task_struct *p, int nice)
+void sched_set_normal(struct task_struct *p, int nice)
 {
 	struct sched_attr attr = {
 		.sched_policy = SCHED_NORMAL,
 		.sched_nice = nice,
 	};
-	return sched_setattr_nocheck(p, &attr);
+	WARN_ON_ONCE(sched_setattr_nocheck(p, &attr) != 0);
 }
 EXPORT_SYMBOL_GPL(sched_set_normal);
 

Re: [PATCH 17/23] sched,irq: Convert to sched_set_fifo()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 01:27:36PM +0200, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Effectively no change.

That's ofcourse a flat out lie; I wrote this before I noticed that -1
thing. It most definitely changes that. I also only noticed this while
reading through my own copy after sending :-/

> @@ -1281,13 +1278,12 @@ setup_irq_thread(struct irqaction *new,
>  	} else {
>  		t = kthread_create(irq_thread, new, "irq/%d-s-%s", irq,
>  				   new->name);
> -		param.sched_priority -= 1;
>  	}

Re: [PATCH 06/23] sched,psci: Convert to sched_set_fifo*()

[Valentin Schneider]

On 22/04/20 12:27, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
>
> Effectively changes prio from 99 to 50.
>
> XXX this thing is horrific, it basically open-codes a stop-machine and
> idle.
>

I went and tried to clean up the thing. I didn't find a clean way to use
stop_machine() + play_idle() (stoppers can't sleep), but I did manage to
get something usable with the existing FIFO threads, play_idle_precise()
and the cpuidle state usage stats, so the whole homebrewed idle thing can
go.

I also just tested it with 50 prio and big surprise it didn't change
anything (it ends up being sibling of idle injection). So FWIW:

Acked-by: Valentin Schneider <valentin.schneider@arm.com>

> Cc: daniel.lezcano@linaro.org
> Cc: sudeep.holla@arm.com
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>

Re: [PATCH 16/23] sched,watchdog: Convert to sched_set_fifo()

[Steven Rostedt]

On Wed, 22 Apr 2020 13:27:35 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Effectively changes prio from 99 to 50.

Hmm, this being a watchdog, and looking at commit 38a1222ae4f364d
("watchdog: core: make sure the watchdog worker always works")

I wonder if we should add a sched_set_high(), or have some other kind of
watchdog handler that is guaranteed to trigger.

-- Steve


> 
> Cc: wim@linux-watchdog.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  drivers/watchdog/watchdog_dev.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -1144,14 +1144,13 @@ void watchdog_dev_unregister(struct watc
>  int __init watchdog_dev_init(void)
>  {
>  	int err;
> -	struct sched_param param = {.sched_priority = MAX_RT_PRIO - 1,};
>  
>  	watchdog_kworker = kthread_create_worker(0, "watchdogd");
>  	if (IS_ERR(watchdog_kworker)) {
>  		pr_err("Failed to create watchdog kworker\n");
>  		return PTR_ERR(watchdog_kworker);
>  	}
> -	sched_setscheduler(watchdog_kworker->task, SCHED_FIFO, &param);
> +	sched_set_fifo(watchdog_kworker->task);
>  
>  	err = class_register(&watchdog_class);
>  	if (err < 0) {
> 

Re: [PATCH 09/23] sched,ivtv: Convert to sched_set_fifo*()

[Steven Rostedt]

On Wed, 22 Apr 2020 13:27:28 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Effectively changes from 99 to 50.

I wonder for the 99 users, we should have a sched_set_high() that would set
the task to something like 75.

That is, above default 50?


-- Steve


> 
> Cc: hverkuil@xs4all.nl
> Cc: awalls@md.metrocast.net
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  drivers/media/pci/ivtv/ivtv-driver.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> --- a/drivers/media/pci/ivtv/ivtv-driver.c
> +++ b/drivers/media/pci/ivtv/ivtv-driver.c
> @@ -737,8 +737,6 @@ static void ivtv_process_options(struct
>   */
>  static int ivtv_init_struct1(struct ivtv *itv)
>  {
> -	struct sched_param param = { .sched_priority = 99 };
> -
>  	itv->base_addr = pci_resource_start(itv->pdev, 0);
>  	itv->enc_mbox.max_mbox = 2; /* the encoder has 3 mailboxes (0-2) */
>  	itv->dec_mbox.max_mbox = 1; /* the decoder has 2 mailboxes (0-1) */
> @@ -758,7 +756,7 @@ static int ivtv_init_struct1(struct ivtv
>  		return -1;
>  	}
>  	/* must use the FIFO scheduler as it is realtime sensitive */
> -	sched_setscheduler(itv->irq_worker_task, SCHED_FIFO, &param);
> +	sched_set_fifo(itv->irq_worker_task);
>  
>  	kthread_init_work(&itv->irq_work, ivtv_irq_work_handler);
>  
> 

Re: [PATCH 01/23] sched: Provide sched_set_fifo()

[Paul E. McKenney]

On Wed, Apr 22, 2020 at 01:27:20PM +0200, Peter Zijlstra wrote:
> SCHED_FIFO (or any static priority scheduler) is a broken scheduler
> model; it is fundamentally incapable of resource management, the one
> thing an OS is actually supposed to do.
> 
> It is impossible to compose static priority workloads. One cannot take
> two well designed and functional static priority workloads and mash
> them together and still expect them to work.
> 
> Therefore it doesn't make sense to expose the priority field; the
> kernel is fundamentally incapable of setting a sensible value, it
> needs systems knowledge that it doesn't have.
> 
> Take away sched_setschedule() / sched_setattr() from modules and
> replace them with:
> 
>   - sched_set_fifo(p); create a FIFO task (at prio 50)
>   - sched_set_fifo_low(p); create a task higher than NORMAL,
> 	which ends up being a FIFO task at prio 1.
>   - sched_set_normal(p, nice); (re)set the task to normal
> 
> This stops the proliferation of randomly chosen, and irrelevant, FIFO
> priorities that dont't really mean anything anyway.
> 
> The system administrator/integrator, whoever has insight into the
> actual system design and requirements (userspace) can set-up
> appropriate priorities if and when needed.

The sched_setscheduler_nocheck() calls in rcu_spawn_gp_kthread(),
rcu_cpu_kthread_setup(), and rcu_spawn_one_boost_kthread() all stay as
is because they all use the rcutree.kthread_prio boot parameter, which is
set at boot time by the system administrator (or {who,what}ever, correct?

Or did my email reader eat a patch or two?

							Thanx, Paul

> Cc: airlied@redhat.com
> Cc: alexander.deucher@amd.com
> Cc: awalls@md.metrocast.net
> Cc: axboe@kernel.dk
> Cc: broonie@kernel.org
> Cc: daniel.lezcano@linaro.org
> Cc: gregkh@linuxfoundation.org
> Cc: hannes@cmpxchg.org
> Cc: herbert@gondor.apana.org.au
> Cc: hverkuil@xs4all.nl
> Cc: john.stultz@linaro.org
> Cc: nico@fluxnic.net
> Cc: paulmck@kernel.org
> Cc: rafael.j.wysocki@intel.com
> Cc: rmk+kernel@arm.linux.org.uk
> Cc: sudeep.holla@arm.com
> Cc: tglx@linutronix.de
> Cc: ulf.hansson@linaro.org
> Cc: wim@linux-watchdog.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  include/linux/sched.h |    3 +++
>  kernel/sched/core.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1631,6 +1631,9 @@ extern int idle_cpu(int cpu);
>  extern int available_idle_cpu(int cpu);
>  extern int sched_setscheduler(struct task_struct *, int, const struct sched_param *);
>  extern int sched_setscheduler_nocheck(struct task_struct *, int, const struct sched_param *);
> +extern int sched_set_fifo(struct task_struct *p);
> +extern int sched_set_fifo_low(struct task_struct *p);
> +extern int sched_set_normal(struct task_struct *p, int nice);
>  extern int sched_setattr(struct task_struct *, const struct sched_attr *);
>  extern int sched_setattr_nocheck(struct task_struct *, const struct sched_attr *);
>  extern struct task_struct *idle_task(int cpu);
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5055,6 +5055,8 @@ static int _sched_setscheduler(struct ta
>   * @policy: new policy.
>   * @param: structure containing the new RT priority.
>   *
> + * Use sched_set_fifo(), read its comment.
> + *
>   * Return: 0 on success. An error code otherwise.
>   *
>   * NOTE that the task may be already dead.
> @@ -5097,6 +5099,51 @@ int sched_setscheduler_nocheck(struct ta
>  }
>  EXPORT_SYMBOL_GPL(sched_setscheduler_nocheck);
>  
> +/*
> + * 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.
> + */
> +int sched_set_fifo(struct task_struct *p)
> +{
> +	struct sched_param sp = { .sched_priority = MAX_RT_PRIO / 2 };
> +	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
> +}
> +EXPORT_SYMBOL_GPL(sched_set_fifo);
> +
> +/*
> + * For when you don't much care about FIFO, but want to be above SCHED_NORMAL.
> + */
> +int sched_set_fifo_low(struct task_struct *p)
> +{
> +	struct sched_param sp = { .sched_priority = 1 };
> +	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
> +}
> +EXPORT_SYMBOL_GPL(sched_set_fifo_low);
> +
> +int sched_set_normal(struct task_struct *p, int nice)
> +{
> +	struct sched_attr attr = {
> +		.sched_policy = SCHED_NORMAL,
> +		.sched_nice = nice,
> +	};
> +	return sched_setattr_nocheck(p, &attr);
> +}
> +EXPORT_SYMBOL_GPL(sched_set_normal);
> +
>  static int
>  do_sched_setscheduler(pid_t pid, int policy, struct sched_param __user *param)
>  {
> 
> 

Re: [PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Vincent Guittot]

On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <peterz@infradead.org> wrote:
>
> In an attempt to take away sched_setscheduler() from modules, change
> this into sched_set_normal(.nice = 19).
>
> Cc: john.stultz@linaro.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  drivers/staging/android/ion/ion_heap.c |    3 ---
>  1 file changed, 3 deletions(-)
>
> --- a/drivers/staging/android/ion/ion_heap.c
> +++ b/drivers/staging/android/ion/ion_heap.c
> @@ -244,8 +244,6 @@ static int ion_heap_deferred_free(void *
>
>  int ion_heap_init_deferred_free(struct ion_heap *heap)
>  {
> -       struct sched_param param = { .sched_priority = 0 };
> -
>         INIT_LIST_HEAD(&heap->free_list);
>         init_waitqueue_head(&heap->waitqueue);
>         heap->task = kthread_run(ion_heap_deferred_free, heap,
> @@ -255,7 +253,7 @@ int ion_heap_init_deferred_free(struct i
>                        __func__);
>                 return PTR_ERR_OR_ZERO(heap->task);
>         }
> -       sched_setscheduler(heap->task, SCHED_IDLE, &param);
> +       sched_set_normal(heap->task, 19);

Would it make sense to have a sched_set_idle(task) to enable kernel
setting SCHED_IDLE task ?

SCHED_NORMAL w/ nice 19 and SCHED_IDLE tasks are not treated in the
same way when checking for preemption at  wakeup

>
>         return 0;
>  }
>
>

Re: [PATCH 16/23] sched,watchdog: Convert to sched_set_fifo()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 08:51:55AM -0400, Steven Rostedt wrote:
> On Wed, 22 Apr 2020 13:27:35 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Because SCHED_FIFO is a broken scheduler model (see previous patches)
> > take away the priority field, the kernel can't possibly make an
> > informed decision.
> > 
> > Effectively changes prio from 99 to 50.
> 
> Hmm, this being a watchdog, and looking at commit 38a1222ae4f364d
> ("watchdog: core: make sure the watchdog worker always works")
> 
> I wonder if we should add a sched_set_high(), or have some other kind of
> watchdog handler that is guaranteed to trigger.

It's FIFO, it'll never win from either a deadline or a stop-task. After
that it doesn't matter.

fifo_high() is most definitely a bad idea, because then we're back into
the whole 'fifo priority' has meaning -- it does not. At least, it
doesn't until you've got system design information.

Maybe we should rename fifo_low to get away from that. I just drew a
blank on a better name there.

Re: [PATCH 09/23] sched,ivtv: Convert to sched_set_fifo*()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 08:53:45AM -0400, Steven Rostedt wrote:
> On Wed, 22 Apr 2020 13:27:28 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Because SCHED_FIFO is a broken scheduler model (see previous patches)
> > take away the priority field, the kernel can't possibly make an
> > informed decision.
> > 
> > Effectively changes from 99 to 50.
> 
> I wonder for the 99 users, we should have a sched_set_high() that would set
> the task to something like 75.
> 
> That is, above default 50?

No. If userspace knows, userspace can fix it. We must not bother with
priority, simply because we cannot, possibly, say something useful.

Re: [PATCH 01/23] sched: Provide sched_set_fifo()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 06:11:38AM -0700, Paul E. McKenney wrote:
> On Wed, Apr 22, 2020 at 01:27:20PM +0200, Peter Zijlstra wrote:
> > SCHED_FIFO (or any static priority scheduler) is a broken scheduler
> > model; it is fundamentally incapable of resource management, the one
> > thing an OS is actually supposed to do.
> > 
> > It is impossible to compose static priority workloads. One cannot take
> > two well designed and functional static priority workloads and mash
> > them together and still expect them to work.
> > 
> > Therefore it doesn't make sense to expose the priority field; the
> > kernel is fundamentally incapable of setting a sensible value, it
> > needs systems knowledge that it doesn't have.
> > 
> > Take away sched_setschedule() / sched_setattr() from modules and
> > replace them with:
> > 
> >   - sched_set_fifo(p); create a FIFO task (at prio 50)
> >   - sched_set_fifo_low(p); create a task higher than NORMAL,
> > 	which ends up being a FIFO task at prio 1.
> >   - sched_set_normal(p, nice); (re)set the task to normal
> > 
> > This stops the proliferation of randomly chosen, and irrelevant, FIFO
> > priorities that dont't really mean anything anyway.
> > 
> > The system administrator/integrator, whoever has insight into the
> > actual system design and requirements (userspace) can set-up
> > appropriate priorities if and when needed.
> 
> The sched_setscheduler_nocheck() calls in rcu_spawn_gp_kthread(),
> rcu_cpu_kthread_setup(), and rcu_spawn_one_boost_kthread() all stay as
> is because they all use the rcutree.kthread_prio boot parameter, which is
> set at boot time by the system administrator (or {who,what}ever, correct?

Correct, also they are not modular afaict, so they escaped the dance ;-)

Re: [PATCH 02/23] sched,bL_switcher: Convert to sched_set_fifo*()

[Nicolas Pitre]

On Wed, 22 Apr 2020, Peter Zijlstra wrote:

> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> In this case, use fifo_low, because it only cares about being above
> SCHED_NORMAL. Effectively no change in behaviour.
> 
> Cc: nico@fluxnic.net
> Cc: rmk+kernel@arm.linux.org.uk
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>

Acked-by: Nicolas Pitre <nico@fluxnic.net>

> ---
>  arch/arm/common/bL_switcher.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- a/arch/arm/common/bL_switcher.c
> +++ b/arch/arm/common/bL_switcher.c
> @@ -270,12 +270,11 @@ static struct bL_thread bL_threads[NR_CP
>  static int bL_switcher_thread(void *arg)
>  {
>  	struct bL_thread *t = arg;
> -	struct sched_param param = { .sched_priority = 1 };
>  	int cluster;
>  	bL_switch_completion_handler completer;
>  	void *completer_cookie;
>  
> -	sched_setscheduler_nocheck(current, SCHED_FIFO, &param);
> +	sched_set_fifo_low(current);
>  	complete(&t->started);
>  
>  	do {
> 
> 
> 

Re: [PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > In an attempt to take away sched_setscheduler() from modules, change
> > this into sched_set_normal(.nice = 19).
> >
> > Cc: john.stultz@linaro.org
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reviewed-by: Ingo Molnar <mingo@kernel.org>
> > ---
> >  drivers/staging/android/ion/ion_heap.c |    3 ---
> >  1 file changed, 3 deletions(-)
> >
> > --- a/drivers/staging/android/ion/ion_heap.c
> > +++ b/drivers/staging/android/ion/ion_heap.c
> > @@ -244,8 +244,6 @@ static int ion_heap_deferred_free(void *
> >
> >  int ion_heap_init_deferred_free(struct ion_heap *heap)
> >  {
> > -       struct sched_param param = { .sched_priority = 0 };
> > -
> >         INIT_LIST_HEAD(&heap->free_list);
> >         init_waitqueue_head(&heap->waitqueue);
> >         heap->task = kthread_run(ion_heap_deferred_free, heap,
> > @@ -255,7 +253,7 @@ int ion_heap_init_deferred_free(struct i
> >                        __func__);
> >                 return PTR_ERR_OR_ZERO(heap->task);
> >         }
> > -       sched_setscheduler(heap->task, SCHED_IDLE, &param);
> > +       sched_set_normal(heap->task, 19);
> 
> Would it make sense to have a sched_set_idle(task) to enable kernel
> setting SCHED_IDLE task ?
> 
> SCHED_NORMAL w/ nice 19 and SCHED_IDLE tasks are not treated in the
> same way when checking for preemption at  wakeup

Yeah, but does it really matter? I did indeed consider it, but got
lazy. Is there a definite need for IDLE?

Re: [PATCH 03/23] sched,crypto: Convert to sched_set_fifo*()

[Herbert Xu]

On Wed, Apr 22, 2020 at 01:27:22PM +0200, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Use sched_set_fifo() to request SCHED_FIFO and delegate
> actual priority selection to userspace. Effectively no change in
> behaviour.
> 
> Cc: herbert@gondor.apana.org.au
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  crypto/crypto_engine.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
-- 
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

Re: [PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Vincent Guittot]

On Wed, 22 Apr 2020 at 15:29, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> > On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > In an attempt to take away sched_setscheduler() from modules, change
> > > this into sched_set_normal(.nice = 19).
> > >
> > > Cc: john.stultz@linaro.org
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > Reviewed-by: Ingo Molnar <mingo@kernel.org>
> > > ---
> > >  drivers/staging/android/ion/ion_heap.c |    3 ---
> > >  1 file changed, 3 deletions(-)
> > >
> > > --- a/drivers/staging/android/ion/ion_heap.c
> > > +++ b/drivers/staging/android/ion/ion_heap.c
> > > @@ -244,8 +244,6 @@ static int ion_heap_deferred_free(void *
> > >
> > >  int ion_heap_init_deferred_free(struct ion_heap *heap)
> > >  {
> > > -       struct sched_param param = { .sched_priority = 0 };
> > > -
> > >         INIT_LIST_HEAD(&heap->free_list);
> > >         init_waitqueue_head(&heap->waitqueue);
> > >         heap->task = kthread_run(ion_heap_deferred_free, heap,
> > > @@ -255,7 +253,7 @@ int ion_heap_init_deferred_free(struct i
> > >                        __func__);
> > >                 return PTR_ERR_OR_ZERO(heap->task);
> > >         }
> > > -       sched_setscheduler(heap->task, SCHED_IDLE, &param);
> > > +       sched_set_normal(heap->task, 19);
> >
> > Would it make sense to have a sched_set_idle(task) to enable kernel
> > setting SCHED_IDLE task ?
> >
> > SCHED_NORMAL w/ nice 19 and SCHED_IDLE tasks are not treated in the
> > same way when checking for preemption at  wakeup
>
> Yeah, but does it really matter? I did indeed consider it, but got
> lazy. Is there a definite need for IDLE?

John is the best to answer this for this driver but SCHED_IDLE will
let other tasks which might be involved in end user interaction like
on Android to run first

Re: [PATCH 11/23] sched,spi: Convert to sched_set_fifo*()

[Mark Brown]

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

On Wed, Apr 22, 2020 at 01:27:30PM +0200, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> No effective change.

Copying Doug who did this change and Guenter who reviewed it.  This
looks fine to me but I've no particular involvement with the code or
platforms that are affected here.

> Cc: broonie@kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  drivers/platform/chrome/cros_ec_spi.c |    6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -709,9 +709,6 @@ static void cros_ec_spi_high_pri_release
>  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 / 2,
> -	};
>  	int err;
>  
>  	ec_spi->high_pri_worker =
> @@ -728,8 +725,7 @@ static int cros_ec_spi_devm_high_pri_all
>  	if (err)
>  		return err;
>  
> -	err = sched_setscheduler_nocheck(ec_spi->high_pri_worker->task,
> -					 SCHED_FIFO, &sched_priority);
> +	err = sched_set_fifo(ec_spi->high_pri_worker->task);
>  	if (err)
>  		dev_err(dev, "Can't set cros_ec high pri priority: %d\n", err);
>  	return err;
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1589,11 +1589,9 @@ EXPORT_SYMBOL_GPL(spi_take_timestamp_pos
>   */
>  static void spi_set_thread_rt(struct spi_controller *ctlr)
>  {
> -	struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
> -
>  	dev_info(&ctlr->dev,
>  		"will run message pump with realtime priority\n");
> -	sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
> +	sched_set_fifo(ctlr->kworker_task);
>  }
>  
>  static int spi_init_queue(struct spi_controller *ctlr)
> 
> 

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

Re: [PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 03:36:22PM +0200, Vincent Guittot wrote:
> On Wed, 22 Apr 2020 at 15:29, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> > > On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > > In an attempt to take away sched_setscheduler() from modules, change
> > > > this into sched_set_normal(.nice = 19).
> > > >
> > > > Cc: john.stultz@linaro.org
> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > > Reviewed-by: Ingo Molnar <mingo@kernel.org>
> > > > ---
> > > >  drivers/staging/android/ion/ion_heap.c |    3 ---
> > > >  1 file changed, 3 deletions(-)
> > > >
> > > > --- a/drivers/staging/android/ion/ion_heap.c
> > > > +++ b/drivers/staging/android/ion/ion_heap.c
> > > > @@ -244,8 +244,6 @@ static int ion_heap_deferred_free(void *
> > > >
> > > >  int ion_heap_init_deferred_free(struct ion_heap *heap)
> > > >  {
> > > > -       struct sched_param param = { .sched_priority = 0 };
> > > > -
> > > >         INIT_LIST_HEAD(&heap->free_list);
> > > >         init_waitqueue_head(&heap->waitqueue);
> > > >         heap->task = kthread_run(ion_heap_deferred_free, heap,
> > > > @@ -255,7 +253,7 @@ int ion_heap_init_deferred_free(struct i
> > > >                        __func__);
> > > >                 return PTR_ERR_OR_ZERO(heap->task);
> > > >         }
> > > > -       sched_setscheduler(heap->task, SCHED_IDLE, &param);
> > > > +       sched_set_normal(heap->task, 19);
> > >
> > > Would it make sense to have a sched_set_idle(task) to enable kernel
> > > setting SCHED_IDLE task ?
> > >
> > > SCHED_NORMAL w/ nice 19 and SCHED_IDLE tasks are not treated in the
> > > same way when checking for preemption at  wakeup
> >
> > Yeah, but does it really matter? I did indeed consider it, but got
> > lazy. Is there a definite need for IDLE?
> 
> John is the best to answer this for this driver but SCHED_IDLE will
> let other tasks which might be involved in end user interaction like
> on Android to run first

So I don't much like SCHED_IDLE because it introduces some pretty
horrible tail latencies. Consider the IDLE task holding a lock, then the
lock waiter will have to wait until the task gets around to running.

It's not unbounded, like a true idle-time scheduler would be, but it can
still be pretty horrible. nice19 has some of that too of course, but
idle has it worse, esp. also because it begs others to preempt it.

I should get back to proxy execution I suppose...

Re: [PATCH 06/23] sched,psci: Convert to sched_set_fifo*()

[Sudeep Holla]

On Wed, Apr 22, 2020 at 01:27:25PM +0200, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Effectively changes prio from 99 to 50.
> 
> XXX this thing is horrific, it basically open-codes a stop-machine and
> idle.
> 
> Cc: daniel.lezcano@linaro.org
> Cc: sudeep.holla@arm.com

Tested the PSCI checker pulling the entire series. Continues to work :)

Tested-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

Re: [PATCH 23/23] sched: Remove sched_set_*() return value

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> Ingo suggested that since the new sched_set_*() functions are
> implemented using the 'nocheck' variants, they really shouldn't ever
> fail, so remove the return value.
> 
> Cc: axboe@kernel.dk
> Cc: daniel.lezcano@linaro.org
> Cc: sudeep.holla@arm.com
> Cc: airlied@redhat.com
> Cc: broonie@kernel.org
> Cc: paulmck@kernel.org
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 11/23] sched,spi: Convert to sched_set_fifo*()

[Doug Anderson]

Hi,

On Wed, Apr 22, 2020 at 6:56 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Apr 22, 2020 at 01:27:30PM +0200, Peter Zijlstra wrote:
> > Because SCHED_FIFO is a broken scheduler model (see previous patches)
> > take away the priority field, the kernel can't possibly make an
> > informed decision.
> >
> > No effective change.
>
> Copying Doug who did this change and Guenter who reviewed it.  This
> looks fine to me but I've no particular involvement with the code or
> platforms that are affected here.

Thanks!  Probably the maintainers of cros_ec_spi.c (Benson and Enric)
should be aware of it, too.  CCing them.

From my point of view, my response is pretty much identical to the one
I wrote when the priority was reduced from "MAX_RT_PRIO - 1" to
"MAX_RT_PRIO / 2" [1].  Basically, any priority that keeps us from
being preempted by tasks that are only high priority for performance
reasons (like dm crypt and loopback did when I last analyzed) is fine.
Our priority needs to be high not for performance reasons but for
correctness reasons (the other side will drop our data if we don't
respond in time).

Thus:

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


[1] https://lore.kernel.org/r/CAD=FV=UsYF1R6+XRfFFFm6PfmkTsEOfxxgCw2JxCnpyu1kGVLQ@mail.gmail.com


> > Cc: broonie@kernel.org
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reviewed-by: Ingo Molnar <mingo@kernel.org>
> > ---
> >  drivers/platform/chrome/cros_ec_spi.c |    6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > --- a/drivers/platform/chrome/cros_ec_spi.c
> > +++ b/drivers/platform/chrome/cros_ec_spi.c
> > @@ -709,9 +709,6 @@ static void cros_ec_spi_high_pri_release
> >  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 / 2,
> > -     };
> >       int err;
> >
> >       ec_spi->high_pri_worker =
> > @@ -728,8 +725,7 @@ static int cros_ec_spi_devm_high_pri_all
> >       if (err)
> >               return err;
> >
> > -     err = sched_setscheduler_nocheck(ec_spi->high_pri_worker->task,
> > -                                      SCHED_FIFO, &sched_priority);
> > +     err = sched_set_fifo(ec_spi->high_pri_worker->task);
> >       if (err)
> >               dev_err(dev, "Can't set cros_ec high pri priority: %d\n", err);
> >       return err;
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1589,11 +1589,9 @@ EXPORT_SYMBOL_GPL(spi_take_timestamp_pos
> >   */
> >  static void spi_set_thread_rt(struct spi_controller *ctlr)
> >  {
> > -     struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
> > -
> >       dev_info(&ctlr->dev,
> >               "will run message pump with realtime priority\n");
> > -     sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
> > +     sched_set_fifo(ctlr->kworker_task);
> >  }
> >
> >  static int spi_init_queue(struct spi_controller *ctlr)
> >
> >

Re: [PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Vincent Guittot]

On Wed, 22 Apr 2020 at 15:59, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Apr 22, 2020 at 03:36:22PM +0200, Vincent Guittot wrote:
> > On Wed, 22 Apr 2020 at 15:29, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> > > > On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > >
> > > > > In an attempt to take away sched_setscheduler() from modules, change
> > > > > this into sched_set_normal(.nice = 19).
> > > > >
> > > > > Cc: john.stultz@linaro.org
> > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > > > Reviewed-by: Ingo Molnar <mingo@kernel.org>
> > > > > ---
> > > > >  drivers/staging/android/ion/ion_heap.c |    3 ---
> > > > >  1 file changed, 3 deletions(-)
> > > > >
> > > > > --- a/drivers/staging/android/ion/ion_heap.c
> > > > > +++ b/drivers/staging/android/ion/ion_heap.c
> > > > > @@ -244,8 +244,6 @@ static int ion_heap_deferred_free(void *
> > > > >
> > > > >  int ion_heap_init_deferred_free(struct ion_heap *heap)
> > > > >  {
> > > > > -       struct sched_param param = { .sched_priority = 0 };
> > > > > -
> > > > >         INIT_LIST_HEAD(&heap->free_list);
> > > > >         init_waitqueue_head(&heap->waitqueue);
> > > > >         heap->task = kthread_run(ion_heap_deferred_free, heap,
> > > > > @@ -255,7 +253,7 @@ int ion_heap_init_deferred_free(struct i
> > > > >                        __func__);
> > > > >                 return PTR_ERR_OR_ZERO(heap->task);
> > > > >         }
> > > > > -       sched_setscheduler(heap->task, SCHED_IDLE, &param);
> > > > > +       sched_set_normal(heap->task, 19);
> > > >
> > > > Would it make sense to have a sched_set_idle(task) to enable kernel
> > > > setting SCHED_IDLE task ?
> > > >
> > > > SCHED_NORMAL w/ nice 19 and SCHED_IDLE tasks are not treated in the
> > > > same way when checking for preemption at  wakeup
> > >
> > > Yeah, but does it really matter? I did indeed consider it, but got
> > > lazy. Is there a definite need for IDLE?
> >
> > John is the best to answer this for this driver but SCHED_IDLE will
> > let other tasks which might be involved in end user interaction like
> > on Android to run first
>
> So I don't much like SCHED_IDLE because it introduces some pretty
> horrible tail latencies. Consider the IDLE task holding a lock, then the
> lock waiter will have to wait until the task gets around to running.

Yes one must be careful when using it
>
> It's not unbounded, like a true idle-time scheduler would be, but it can
> still be pretty horrible. nice19 has some of that too of course, but
> idle has it worse, esp. also because it begs others to preempt it.

Yeah... you have to pay the benefit of letting other tasks to preempt
faster. But both sched_idle and nice19 have the same weight and as a
result they will have the same amount of running time.  So I'm not
sure that sched-idle will have bigger problems than nice 19 but may be
more often

>
> I should get back to proxy execution I suppose...

Re: [PATCH 21/23] sched,psi: Convert to sched_set_fifo_low()

[Johannes Weiner]

On Wed, Apr 22, 2020 at 01:27:40PM +0200, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Effectively no change.
> 
> Cc: hannes@cmpxchg.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>

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

Re: [PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Juri Lelli]

On 22/04/20 15:59, Peter Zijlstra wrote:
> On Wed, Apr 22, 2020 at 03:36:22PM +0200, Vincent Guittot wrote:
> > On Wed, 22 Apr 2020 at 15:29, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> > > > On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > >
> > > > > In an attempt to take away sched_setscheduler() from modules, change
> > > > > this into sched_set_normal(.nice = 19).
> > > > >
> > > > > Cc: john.stultz@linaro.org
> > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > > > Reviewed-by: Ingo Molnar <mingo@kernel.org>
> > > > > ---
> > > > >  drivers/staging/android/ion/ion_heap.c |    3 ---
> > > > >  1 file changed, 3 deletions(-)
> > > > >
> > > > > --- a/drivers/staging/android/ion/ion_heap.c
> > > > > +++ b/drivers/staging/android/ion/ion_heap.c
> > > > > @@ -244,8 +244,6 @@ static int ion_heap_deferred_free(void *
> > > > >
> > > > >  int ion_heap_init_deferred_free(struct ion_heap *heap)
> > > > >  {
> > > > > -       struct sched_param param = { .sched_priority = 0 };
> > > > > -
> > > > >         INIT_LIST_HEAD(&heap->free_list);
> > > > >         init_waitqueue_head(&heap->waitqueue);
> > > > >         heap->task = kthread_run(ion_heap_deferred_free, heap,
> > > > > @@ -255,7 +253,7 @@ int ion_heap_init_deferred_free(struct i
> > > > >                        __func__);
> > > > >                 return PTR_ERR_OR_ZERO(heap->task);
> > > > >         }
> > > > > -       sched_setscheduler(heap->task, SCHED_IDLE, &param);
> > > > > +       sched_set_normal(heap->task, 19);
> > > >
> > > > Would it make sense to have a sched_set_idle(task) to enable kernel
> > > > setting SCHED_IDLE task ?
> > > >
> > > > SCHED_NORMAL w/ nice 19 and SCHED_IDLE tasks are not treated in the
> > > > same way when checking for preemption at  wakeup
> > >
> > > Yeah, but does it really matter? I did indeed consider it, but got
> > > lazy. Is there a definite need for IDLE?
> > 
> > John is the best to answer this for this driver but SCHED_IDLE will
> > let other tasks which might be involved in end user interaction like
> > on Android to run first
> 
> So I don't much like SCHED_IDLE because it introduces some pretty
> horrible tail latencies. Consider the IDLE task holding a lock, then the
> lock waiter will have to wait until the task gets around to running.
> 
> It's not unbounded, like a true idle-time scheduler would be, but it can
> still be pretty horrible. nice19 has some of that too of course, but
> idle has it worse, esp. also because it begs others to preempt it.
> 
> I should get back to proxy execution I suppose...

Huh, so you really think proxy exec should be default on for kernel
mutexes...

Re: [PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 05:09:15PM +0200, Vincent Guittot wrote:
> > It's not unbounded, like a true idle-time scheduler would be, but it can
> > still be pretty horrible. nice19 has some of that too of course, but
> > idle has it worse, esp. also because it begs others to preempt it.
> 
> Yeah... you have to pay the benefit of letting other tasks to preempt
> faster. But both sched_idle and nice19 have the same weight 

#define WEIGHT_IDLEPRIO		3

 /*  15 */        36,        29,        23,        18,        15,

15 != 3

Also, like said elsewhere, idle is much more eager to let itself be
preempted.

Re: [PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 05:38:20PM +0200, Juri Lelli wrote:
> On 22/04/20 15:59, Peter Zijlstra wrote:

> > I should get back to proxy execution I suppose...
> 
> Huh, so you really think proxy exec should be default on for kernel
> mutexes...

We'll see, ideally yes, but even if not, then you have something to give
to people who run into trouble.

It would also allow a true idle time class, although I'm not sure we'd
actually want to go there.

Re: [PATCH 11/23] sched,spi: Convert to sched_set_fifo*()

[Guenter Roeck]

On Wed, Apr 22, 2020 at 7:35 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Apr 22, 2020 at 6:56 AM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Wed, Apr 22, 2020 at 01:27:30PM +0200, Peter Zijlstra wrote:
> > > Because SCHED_FIFO is a broken scheduler model (see previous patches)
> > > take away the priority field, the kernel can't possibly make an
> > > informed decision.
> > >
> > > No effective change.
> >
> > Copying Doug who did this change and Guenter who reviewed it.  This
> > looks fine to me but I've no particular involvement with the code or
> > platforms that are affected here.
>
> Thanks!  Probably the maintainers of cros_ec_spi.c (Benson and Enric)
> should be aware of it, too.  CCing them.
>
> From my point of view, my response is pretty much identical to the one
> I wrote when the priority was reduced from "MAX_RT_PRIO - 1" to
> "MAX_RT_PRIO / 2" [1].  Basically, any priority that keeps us from
> being preempted by tasks that are only high priority for performance
> reasons (like dm crypt and loopback did when I last analyzed) is fine.
> Our priority needs to be high not for performance reasons but for
> correctness reasons (the other side will drop our data if we don't
> respond in time).
>
The crypto engine ends up running at the same priority level, so I am
a bit concerned that this patch series will re-introduce the problem
that Doug's initial patch tried to solve. Though I do notice that it
already _is_ running at the same priority, so maybe the problem has
already been re-introduced with the commit that set the priority to
MAX_RT_PRIO / 2, and we just haven't noticed yet. So I guess this
patch indeed doesn't make a difference.

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> Thus:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
>
> [1] https://lore.kernel.org/r/CAD=FV=UsYF1R6+XRfFFFm6PfmkTsEOfxxgCw2JxCnpyu1kGVLQ@mail.gmail.com
>
>
> > > Cc: broonie@kernel.org
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > Reviewed-by: Ingo Molnar <mingo@kernel.org>
> > > ---
> > >  drivers/platform/chrome/cros_ec_spi.c |    6 +-----
> > >  1 file changed, 1 insertion(+), 5 deletions(-)
> > >
> > > --- a/drivers/platform/chrome/cros_ec_spi.c
> > > +++ b/drivers/platform/chrome/cros_ec_spi.c
> > > @@ -709,9 +709,6 @@ static void cros_ec_spi_high_pri_release
> > >  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 / 2,
> > > -     };
> > >       int err;
> > >
> > >       ec_spi->high_pri_worker =
> > > @@ -728,8 +725,7 @@ static int cros_ec_spi_devm_high_pri_all
> > >       if (err)
> > >               return err;
> > >
> > > -     err = sched_setscheduler_nocheck(ec_spi->high_pri_worker->task,
> > > -                                      SCHED_FIFO, &sched_priority);
> > > +     err = sched_set_fifo(ec_spi->high_pri_worker->task);
> > >       if (err)
> > >               dev_err(dev, "Can't set cros_ec high pri priority: %d\n", err);
> > >       return err;
> > > --- a/drivers/spi/spi.c
> > > +++ b/drivers/spi/spi.c
> > > @@ -1589,11 +1589,9 @@ EXPORT_SYMBOL_GPL(spi_take_timestamp_pos
> > >   */
> > >  static void spi_set_thread_rt(struct spi_controller *ctlr)
> > >  {
> > > -     struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
> > > -
> > >       dev_info(&ctlr->dev,
> > >               "will run message pump with realtime priority\n");
> > > -     sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
> > > +     sched_set_fifo(ctlr->kworker_task);
> > >  }
> > >
> > >  static int spi_init_queue(struct spi_controller *ctlr)
> > >
> > >

Re: [PATCH 01/23] sched: Provide sched_set_fifo()

[Paul E. McKenney]

On Wed, Apr 22, 2020 at 03:26:48PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 22, 2020 at 06:11:38AM -0700, Paul E. McKenney wrote:
> > On Wed, Apr 22, 2020 at 01:27:20PM +0200, Peter Zijlstra wrote:
> > > SCHED_FIFO (or any static priority scheduler) is a broken scheduler
> > > model; it is fundamentally incapable of resource management, the one
> > > thing an OS is actually supposed to do.
> > > 
> > > It is impossible to compose static priority workloads. One cannot take
> > > two well designed and functional static priority workloads and mash
> > > them together and still expect them to work.
> > > 
> > > Therefore it doesn't make sense to expose the priority field; the
> > > kernel is fundamentally incapable of setting a sensible value, it
> > > needs systems knowledge that it doesn't have.
> > > 
> > > Take away sched_setschedule() / sched_setattr() from modules and
> > > replace them with:
> > > 
> > >   - sched_set_fifo(p); create a FIFO task (at prio 50)
> > >   - sched_set_fifo_low(p); create a task higher than NORMAL,
> > > 	which ends up being a FIFO task at prio 1.
> > >   - sched_set_normal(p, nice); (re)set the task to normal
> > > 
> > > This stops the proliferation of randomly chosen, and irrelevant, FIFO
> > > priorities that dont't really mean anything anyway.
> > > 
> > > The system administrator/integrator, whoever has insight into the
> > > actual system design and requirements (userspace) can set-up
> > > appropriate priorities if and when needed.
> > 
> > The sched_setscheduler_nocheck() calls in rcu_spawn_gp_kthread(),
> > rcu_cpu_kthread_setup(), and rcu_spawn_one_boost_kthread() all stay as
> > is because they all use the rcutree.kthread_prio boot parameter, which is
> > set at boot time by the system administrator (or {who,what}ever, correct?
> 
> Correct, also they are not modular afaict, so they escaped the dance ;-)

Indeed, an extreme form of insanity would be required to try to make core
RCU be a module.  Not that such a form of insanity is a bad thing in and
of itself, but it might best be directed towards less futile ventures.  ;-)

							Thanx, Paul

Re: [PATCH 01/23] sched: Provide sched_set_fifo()

[Paul E. McKenney]

On Wed, Apr 22, 2020 at 01:27:20PM +0200, Peter Zijlstra wrote:
> SCHED_FIFO (or any static priority scheduler) is a broken scheduler
> model; it is fundamentally incapable of resource management, the one
> thing an OS is actually supposed to do.
> 
> It is impossible to compose static priority workloads. One cannot take
> two well designed and functional static priority workloads and mash
> them together and still expect them to work.
> 
> Therefore it doesn't make sense to expose the priority field; the
> kernel is fundamentally incapable of setting a sensible value, it
> needs systems knowledge that it doesn't have.
> 
> Take away sched_setschedule() / sched_setattr() from modules and
> replace them with:
> 
>   - sched_set_fifo(p); create a FIFO task (at prio 50)
>   - sched_set_fifo_low(p); create a task higher than NORMAL,
> 	which ends up being a FIFO task at prio 1.
>   - sched_set_normal(p, nice); (re)set the task to normal
> 
> This stops the proliferation of randomly chosen, and irrelevant, FIFO
> priorities that dont't really mean anything anyway.
> 
> The system administrator/integrator, whoever has insight into the
> actual system design and requirements (userspace) can set-up
> appropriate priorities if and when needed.
> 
> Cc: airlied@redhat.com
> Cc: alexander.deucher@amd.com
> Cc: awalls@md.metrocast.net
> Cc: axboe@kernel.dk
> Cc: broonie@kernel.org
> Cc: daniel.lezcano@linaro.org
> Cc: gregkh@linuxfoundation.org
> Cc: hannes@cmpxchg.org
> Cc: herbert@gondor.apana.org.au
> Cc: hverkuil@xs4all.nl
> Cc: john.stultz@linaro.org
> Cc: nico@fluxnic.net
> Cc: paulmck@kernel.org
> Cc: rafael.j.wysocki@intel.com
> Cc: rmk+kernel@arm.linux.org.uk
> Cc: sudeep.holla@arm.com
> Cc: tglx@linutronix.de
> Cc: ulf.hansson@linaro.org
> Cc: wim@linux-watchdog.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>

Tested-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  include/linux/sched.h |    3 +++
>  kernel/sched/core.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1631,6 +1631,9 @@ extern int idle_cpu(int cpu);
>  extern int available_idle_cpu(int cpu);
>  extern int sched_setscheduler(struct task_struct *, int, const struct sched_param *);
>  extern int sched_setscheduler_nocheck(struct task_struct *, int, const struct sched_param *);
> +extern int sched_set_fifo(struct task_struct *p);
> +extern int sched_set_fifo_low(struct task_struct *p);
> +extern int sched_set_normal(struct task_struct *p, int nice);
>  extern int sched_setattr(struct task_struct *, const struct sched_attr *);
>  extern int sched_setattr_nocheck(struct task_struct *, const struct sched_attr *);
>  extern struct task_struct *idle_task(int cpu);
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5055,6 +5055,8 @@ static int _sched_setscheduler(struct ta
>   * @policy: new policy.
>   * @param: structure containing the new RT priority.
>   *
> + * Use sched_set_fifo(), read its comment.
> + *
>   * Return: 0 on success. An error code otherwise.
>   *
>   * NOTE that the task may be already dead.
> @@ -5097,6 +5099,51 @@ int sched_setscheduler_nocheck(struct ta
>  }
>  EXPORT_SYMBOL_GPL(sched_setscheduler_nocheck);
>  
> +/*
> + * 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.
> + */
> +int sched_set_fifo(struct task_struct *p)
> +{
> +	struct sched_param sp = { .sched_priority = MAX_RT_PRIO / 2 };
> +	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
> +}
> +EXPORT_SYMBOL_GPL(sched_set_fifo);
> +
> +/*
> + * For when you don't much care about FIFO, but want to be above SCHED_NORMAL.
> + */
> +int sched_set_fifo_low(struct task_struct *p)
> +{
> +	struct sched_param sp = { .sched_priority = 1 };
> +	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
> +}
> +EXPORT_SYMBOL_GPL(sched_set_fifo_low);
> +
> +int sched_set_normal(struct task_struct *p, int nice)
> +{
> +	struct sched_attr attr = {
> +		.sched_policy = SCHED_NORMAL,
> +		.sched_nice = nice,
> +	};
> +	return sched_setattr_nocheck(p, &attr);
> +}
> +EXPORT_SYMBOL_GPL(sched_set_normal);
> +
>  static int
>  do_sched_setscheduler(pid_t pid, int policy, struct sched_param __user *param)
>  {
> 
> 

Re: [PATCH 18/23] sched,locktorture: Convert to sched_set_fifo()

[Paul E. McKenney]

On Wed, Apr 22, 2020 at 01:27:37PM +0200, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Effectively changes prio from 99 to 50.
> 
> Cc: paulmck@kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  kernel/locking/locktorture.c |   10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> --- a/kernel/locking/locktorture.c
> +++ b/kernel/locking/locktorture.c
> @@ -436,8 +436,6 @@ static int torture_rtmutex_lock(void) __
>  
>  static void torture_rtmutex_boost(struct torture_random_state *trsp)
>  {
> -	int policy;
> -	struct sched_param param;
>  	const unsigned int factor = 50000; /* yes, quite arbitrary */
>  
>  	if (!rt_task(current)) {
> @@ -448,8 +446,7 @@ static void torture_rtmutex_boost(struct
>  		 */
>  		if (trsp && !(torture_random(trsp) %
>  			      (cxt.nrealwriters_stress * factor))) {
> -			policy = SCHED_FIFO;
> -			param.sched_priority = MAX_RT_PRIO - 1;
> +			sched_set_fifo(current);
>  		} else /* common case, do nothing */
>  			return;
>  	} else {
> @@ -462,13 +459,10 @@ static void torture_rtmutex_boost(struct
>  		 */
>  		if (!trsp || !(torture_random(trsp) %
>  			       (cxt.nrealwriters_stress * factor * 2))) {
> -			policy = SCHED_NORMAL;
> -			param.sched_priority = 0;
> +			sched_set_normal(current, 0);
>  		} else /* common case, do nothing */
>  			return;
>  	}
> -
> -	sched_setscheduler_nocheck(current, policy, &param);
>  }
>  
>  static void torture_rtmutex_delay(struct torture_random_state *trsp)
> 
> 

Re: [PATCH 19/23] sched,rcuperf: Convert to sched_set_fifo_low()

[Paul E. McKenney]

On Wed, Apr 22, 2020 at 01:27:38PM +0200, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Effectively no change.
> 
> Cc: paulmck@kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  kernel/rcu/rcuperf.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> --- a/kernel/rcu/rcuperf.c
> +++ b/kernel/rcu/rcuperf.c
> @@ -353,7 +353,6 @@ rcu_perf_writer(void *arg)
>  	int i_max;
>  	long me = (long)arg;
>  	struct rcu_head *rhp = NULL;
> -	struct sched_param sp;
>  	bool started = false, done = false, alldone = false;
>  	u64 t;
>  	u64 *wdp;
> @@ -362,8 +361,7 @@ rcu_perf_writer(void *arg)
>  	VERBOSE_PERFOUT_STRING("rcu_perf_writer task started");
>  	WARN_ON(!wdpp);
>  	set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids));
> -	sp.sched_priority = 1;
> -	sched_setscheduler_nocheck(current, SCHED_FIFO, &sp);
> +	sched_set_fifo_low(current);
>  
>  	if (holdoff)
>  		schedule_timeout_uninterruptible(holdoff * HZ);
> @@ -419,9 +417,7 @@ rcu_perf_writer(void *arg)
>  			started = true;
>  		if (!done && i >= MIN_MEAS) {
>  			done = true;
> -			sp.sched_priority = 0;
> -			sched_setscheduler_nocheck(current,
> -						   SCHED_NORMAL, &sp);
> +			sched_set_normal(current, 0);
>  			pr_alert("%s%s rcu_perf_writer %ld has %d measurements\n",
>  				 perf_type, PERF_FLAG, me, MIN_MEAS);
>  			if (atomic_inc_return(&n_rcu_perf_writer_finished) >=
> 
> 

Re: [PATCH 20/23] sched,rcutorture: Convert to sched_set_fifo_low()

[Paul E. McKenney]

On Wed, Apr 22, 2020 at 01:27:39PM +0200, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Effectively no change.
> 
> Cc: paulmck@kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  kernel/rcu/rcutorture.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> --- a/kernel/rcu/rcutorture.c
> +++ b/kernel/rcu/rcutorture.c
> @@ -802,13 +802,11 @@ static int rcu_torture_boost(void *arg)
>  	unsigned long endtime;
>  	unsigned long oldstarttime;
>  	struct rcu_boost_inflight rbi = { .inflight = 0 };
> -	struct sched_param sp;
>  
>  	VERBOSE_TOROUT_STRING("rcu_torture_boost started");
>  
>  	/* Set real-time priority. */
> -	sp.sched_priority = 1;
> -	if (sched_setscheduler(current, SCHED_FIFO, &sp) < 0) {
> +	if (sched_set_fifo_low(current) < 0) {
>  		VERBOSE_TOROUT_STRING("rcu_torture_boost RT prio failed!");
>  		n_rcu_torture_boost_rterror++;
>  	}
> 
> 

Re: [PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Vincent Guittot]

On Wed, 22 Apr 2020 at 17:39, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Apr 22, 2020 at 05:09:15PM +0200, Vincent Guittot wrote:
> > > It's not unbounded, like a true idle-time scheduler would be, but it can
> > > still be pretty horrible. nice19 has some of that too of course, but
> > > idle has it worse, esp. also because it begs others to preempt it.
> >
> > Yeah... you have to pay the benefit of letting other tasks to preempt
> > faster. But both sched_idle and nice19 have the same weight
>
> #define WEIGHT_IDLEPRIO         3
>
>  /*  15 */        36,        29,        23,        18,        15,
>
> 15 != 3

Good point
Don't know why I thought they had same weight

>
> Also, like said elsewhere, idle is much more eager to let itself be
> preempted.

Re: [PATCH 23/23] sched: Remove sched_set_*() return value

[Paul E. McKenney]

On Wed, Apr 22, 2020 at 01:27:42PM +0200, Peter Zijlstra wrote:
> Ingo suggested that since the new sched_set_*() functions are
> implemented using the 'nocheck' variants, they really shouldn't ever
> fail, so remove the return value.
> 
> Cc: axboe@kernel.dk
> Cc: daniel.lezcano@linaro.org
> Cc: sudeep.holla@arm.com
> Cc: airlied@redhat.com
> Cc: broonie@kernel.org
> Cc: paulmck@kernel.org
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  drivers/block/drbd/drbd_receiver.c    |    4 +---
>  drivers/firmware/psci/psci_checker.c  |    3 +--
>  drivers/gpu/drm/msm/msm_drv.c         |    5 +----
>  drivers/platform/chrome/cros_ec_spi.c |    7 +++----
>  include/linux/sched.h                 |    6 +++---
>  kernel/rcu/rcutorture.c               |    5 +----
>  kernel/sched/core.c                   |   12 ++++++------
>  7 files changed, 16 insertions(+), 26 deletions(-)

[ . . . ]

> --- a/kernel/rcu/rcutorture.c
> +++ b/kernel/rcu/rcutorture.c
> @@ -806,10 +806,7 @@ static int rcu_torture_boost(void *arg)
>  	VERBOSE_TOROUT_STRING("rcu_torture_boost started");
>  
>  	/* Set real-time priority. */
> -	if (sched_set_fifo_low(current) < 0) {
> -		VERBOSE_TOROUT_STRING("rcu_torture_boost RT prio failed!");
> -		n_rcu_torture_boost_rterror++;
> -	}
> +	sched_set_fifo_low(current);
>  
>  	init_rcu_head_on_stack(&rbi.rcu);
>  	/* Each pass through the following loop does one boost-test cycle. */

This is the only update of n_rcu_torture_boost_rterror, so it can
be eliminated entirely, for example as shown below.

Other than that, looks good to me!

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index ee27b57..61b0c4f 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -171,7 +171,6 @@ static atomic_t n_rcu_torture_mberror;
 static atomic_t n_rcu_torture_error;
 static long n_rcu_torture_barrier_error;
 static long n_rcu_torture_boost_ktrerror;
-static long n_rcu_torture_boost_rterror;
 static long n_rcu_torture_boost_failure;
 static long n_rcu_torture_boosts;
 static atomic_long_t n_rcu_torture_timers;
@@ -893,10 +892,7 @@ static int rcu_torture_boost(void *arg)
 	VERBOSE_TOROUT_STRING("rcu_torture_boost started");
 
 	/* Set real-time priority. */
-	if (sched_set_fifo_low(current) < 0) {
-		VERBOSE_TOROUT_STRING("rcu_torture_boost RT prio failed!");
-		n_rcu_torture_boost_rterror++;
-	}
+	sched_set_fifo_low(current);
 
 	init_rcu_head_on_stack(&rbi.rcu);
 	/* Each pass through the following loop does one boost-test cycle. */
@@ -1527,11 +1523,10 @@ rcu_torture_stats_print(void)
 		atomic_read(&n_rcu_torture_alloc),
 		atomic_read(&n_rcu_torture_alloc_fail),
 		atomic_read(&n_rcu_torture_free));
-	pr_cont("rtmbe: %d rtbe: %ld rtbke: %ld rtbre: %ld ",
+	pr_cont("rtmbe: %d rtbe: %ld rtbke: %ld ",
 		atomic_read(&n_rcu_torture_mberror),
 		n_rcu_torture_barrier_error,
-		n_rcu_torture_boost_ktrerror,
-		n_rcu_torture_boost_rterror);
+		n_rcu_torture_boost_ktrerror);
 	pr_cont("rtbf: %ld rtb: %ld nt: %ld ",
 		n_rcu_torture_boost_failure,
 		n_rcu_torture_boosts,
@@ -1545,14 +1540,12 @@ rcu_torture_stats_print(void)
 	pr_alert("%s%s ", torture_type, TORTURE_FLAG);
 	if (atomic_read(&n_rcu_torture_mberror) ||
 	    n_rcu_torture_barrier_error || n_rcu_torture_boost_ktrerror ||
-	    n_rcu_torture_boost_rterror || n_rcu_torture_boost_failure ||
-	    i > 1) {
+	    n_rcu_torture_boost_failure || i > 1) {
 		pr_cont("%s", "!!! ");
 		atomic_inc(&n_rcu_torture_error);
 		WARN_ON_ONCE(atomic_read(&n_rcu_torture_mberror));
 		WARN_ON_ONCE(n_rcu_torture_barrier_error);  // rcu_barrier()
 		WARN_ON_ONCE(n_rcu_torture_boost_ktrerror); // no boost kthread
-		WARN_ON_ONCE(n_rcu_torture_boost_rterror); // can't set RT prio
 		WARN_ON_ONCE(n_rcu_torture_boost_failure); // RCU boost failed
 		WARN_ON_ONCE(i > 1); // Too-short grace period
 	}
@@ -2569,7 +2562,6 @@ rcu_torture_init(void)
 	atomic_set(&n_rcu_torture_error, 0);
 	n_rcu_torture_barrier_error = 0;
 	n_rcu_torture_boost_ktrerror = 0;
-	n_rcu_torture_boost_rterror = 0;
 	n_rcu_torture_boost_failure = 0;
 	n_rcu_torture_boosts = 0;
 	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)

Re: [PATCH 01/23] sched: Provide sched_set_fifo()

[Steven Rostedt]

On Wed, 22 Apr 2020 08:50:06 -0700
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> Indeed, an extreme form of insanity would be required to try to make core
> RCU be a module.  Not that such a form of insanity is a bad thing in and
> of itself, but it might best be directed towards less futile ventures.  ;-)

That's like making the core of mutexes a module. How would that ever work
(without becoming a microkernel).

-- Steve

Re: [PATCH 01/23] sched: Provide sched_set_fifo()

[Paul E. McKenney]

On Wed, Apr 22, 2020 at 12:33:31PM -0400, Steven Rostedt wrote:
> On Wed, 22 Apr 2020 08:50:06 -0700
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > Indeed, an extreme form of insanity would be required to try to make core
> > RCU be a module.  Not that such a form of insanity is a bad thing in and
> > of itself, but it might best be directed towards less futile ventures.  ;-)
> 
> That's like making the core of mutexes a module. How would that ever work
> (without becoming a microkernel).

Someone somewhere has probably done it.  Perhaps you really could make
mutexes be a module within the Linux kernel using kpatch or similar.
Sort of, anyway.

Not saying it is not insane, mind you!  ;-)

							Thanx, Paul

Re: [PATCH 11/23] sched,spi: Convert to sched_set_fifo*()

[Doug Anderson]

Hi,

On Wed, Apr 22, 2020 at 8:48 AM Guenter Roeck <groeck@google.com> wrote:
>
> On Wed, Apr 22, 2020 at 7:35 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Apr 22, 2020 at 6:56 AM Mark Brown <broonie@kernel.org> wrote:
> > >
> > > On Wed, Apr 22, 2020 at 01:27:30PM +0200, Peter Zijlstra wrote:
> > > > Because SCHED_FIFO is a broken scheduler model (see previous patches)
> > > > take away the priority field, the kernel can't possibly make an
> > > > informed decision.
> > > >
> > > > No effective change.
> > >
> > > Copying Doug who did this change and Guenter who reviewed it.  This
> > > looks fine to me but I've no particular involvement with the code or
> > > platforms that are affected here.
> >
> > Thanks!  Probably the maintainers of cros_ec_spi.c (Benson and Enric)
> > should be aware of it, too.  CCing them.
> >
> > From my point of view, my response is pretty much identical to the one
> > I wrote when the priority was reduced from "MAX_RT_PRIO - 1" to
> > "MAX_RT_PRIO / 2" [1].  Basically, any priority that keeps us from
> > being preempted by tasks that are only high priority for performance
> > reasons (like dm crypt and loopback did when I last analyzed) is fine.
> > Our priority needs to be high not for performance reasons but for
> > correctness reasons (the other side will drop our data if we don't
> > respond in time).
> >
> The crypto engine ends up running at the same priority level, so I am
> a bit concerned that this patch series will re-introduce the problem
> that Doug's initial patch tried to solve.

Do you have a pointer to the code you're looking at?  Digging through
my old investigation for dm-crypt showed the problem to be the code
touched by commit a1b89132dc4f ("dm crypt: use
WQ_HIGHPRI for the IO and crypt workqueues").  Interestingly enough,
that's been reverted in commit f612b2132db5 ("Revert "dm crypt: use
WQ_HIGHPRI for the IO and crypt workqueues"").

Even if something is using WQ_HIGHPRI, last I checked WQ_HIGHPRI was
the highest non-relatime priority.  Looking quickly I see
"HIGHPRI_NICE_LEVEL" which is MIN_NICE.  I don't think that implies
realtime, but I assume sched_fifo() still does.

-Doug

Re: [PATCH 04/23] sched,acpi_pad: Convert to sched_set_fifo*()

[Dietmar Eggemann]

On 22/04/2020 13:27, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> In this case, use fifo_low, because it only cares about being above
> SCHED_NORMAL. Effectively no change in behaviour.
> 
> XXX: this driver is still complete crap; why isn't it using proper
> idle injection or at the very least play_idle() ?
> 
> Cc: rafael.j.wysocki@intel.com
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  drivers/acpi/acpi_pad.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- a/drivers/acpi/acpi_pad.c
> +++ b/drivers/acpi/acpi_pad.c
> @@ -136,12 +136,11 @@ static unsigned int idle_pct = 5; /* per
>  static unsigned int round_robin_time = 1; /* second */
>  static int power_saving_thread(void *data)
>  {
> -	struct sched_param param = {.sched_priority = 1};
>  	int do_sleep;
>  	unsigned int tsk_index = (unsigned long)data;
>  	u64 last_jiffies = 0;
>  
> -	sched_setscheduler(current, SCHED_RR, &param);

I was wondering what happened to the SCHED_RR cases but as I can see now
they are handled here and in the next patch.

> +	sched_set_fifo_low(current);
>  
>  	while (!kthread_should_stop()) {
>  		unsigned long expire_time;
> 
> 

Re: [PATCH 01/23] sched: Provide sched_set_fifo()

[Steven Rostedt]

On Wed, 22 Apr 2020 09:40:30 -0700
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> Not saying it is not insane, mind you!  ;-)

I sometimes wonder about your sanity ;-)

-- Steve

Re: [PATCH 10/23] sched,mmc: Convert to sched_set_fifo*()

[Ulf Hansson]

On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
>
> In this case, use fifo_low, because it only cares about being above
> SCHED_NORMAL. Effectively no change in behaviour.
>
> Cc: ulf.hansson@linaro.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

FYI: I am slowly moving towards removing the entire kthread for the
sdio_irq_thread(). It shouldn't be too far off to be posted, one or
two kernel releases or so.

Kind regards
Uffe

> ---
>  drivers/mmc/core/sdio_irq.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> --- a/drivers/mmc/core/sdio_irq.c
> +++ b/drivers/mmc/core/sdio_irq.c
> @@ -139,11 +139,10 @@ EXPORT_SYMBOL_GPL(sdio_signal_irq);
>  static int sdio_irq_thread(void *_host)
>  {
>         struct mmc_host *host = _host;
> -       struct sched_param param = { .sched_priority = 1 };
>         unsigned long period, idle_period;
>         int ret;
>
> -       sched_setscheduler(current, SCHED_FIFO, &param);
> +       sched_set_fifo_low(current);
>
>         /*
>          * We want to allow for SDIO cards to work even on non SDIO
>
>

Re: [PATCH 01/23] sched: Provide sched_set_fifo()

[Paul E. McKenney]

On Wed, Apr 22, 2020 at 12:46:51PM -0400, Steven Rostedt wrote:
> On Wed, 22 Apr 2020 09:40:30 -0700
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > Not saying it is not insane, mind you!  ;-)
> 
> I sometimes wonder about your sanity ;-)

;-) ;-) ;-)

							Thanx, Paul

Re: [PATCH 11/23] sched,spi: Convert to sched_set_fifo*()

[Guenter Roeck]

On Wed, Apr 22, 2020 at 9:41 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Apr 22, 2020 at 8:48 AM Guenter Roeck <groeck@google.com> wrote:
> >
> > On Wed, Apr 22, 2020 at 7:35 AM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Apr 22, 2020 at 6:56 AM Mark Brown <broonie@kernel.org> wrote:
> > > >
> > > > On Wed, Apr 22, 2020 at 01:27:30PM +0200, Peter Zijlstra wrote:
> > > > > Because SCHED_FIFO is a broken scheduler model (see previous patches)
> > > > > take away the priority field, the kernel can't possibly make an
> > > > > informed decision.
> > > > >
> > > > > No effective change.
> > > >
> > > > Copying Doug who did this change and Guenter who reviewed it.  This
> > > > looks fine to me but I've no particular involvement with the code or
> > > > platforms that are affected here.
> > >
> > > Thanks!  Probably the maintainers of cros_ec_spi.c (Benson and Enric)
> > > should be aware of it, too.  CCing them.
> > >
> > > From my point of view, my response is pretty much identical to the one
> > > I wrote when the priority was reduced from "MAX_RT_PRIO - 1" to
> > > "MAX_RT_PRIO / 2" [1].  Basically, any priority that keeps us from
> > > being preempted by tasks that are only high priority for performance
> > > reasons (like dm crypt and loopback did when I last analyzed) is fine.
> > > Our priority needs to be high not for performance reasons but for
> > > correctness reasons (the other side will drop our data if we don't
> > > respond in time).
> > >
> > The crypto engine ends up running at the same priority level, so I am
> > a bit concerned that this patch series will re-introduce the problem
> > that Doug's initial patch tried to solve.
>
> Do you have a pointer to the code you're looking at?  Digging through

I was looking at crypto/crypto_engine.c:crypto_engine_alloc_init().
Maybe that is different code and I misunderstand its use, though.

Guenter

> my old investigation for dm-crypt showed the problem to be the code
> touched by commit a1b89132dc4f ("dm crypt: use
> WQ_HIGHPRI for the IO and crypt workqueues").  Interestingly enough,
> that's been reverted in commit f612b2132db5 ("Revert "dm crypt: use
> WQ_HIGHPRI for the IO and crypt workqueues"").
>
> Even if something is using WQ_HIGHPRI, last I checked WQ_HIGHPRI was
> the highest non-relatime priority.  Looking quickly I see
> "HIGHPRI_NICE_LEVEL" which is MIN_NICE.  I don't think that implies
> realtime, but I assume sched_fifo() still does.
>
> -Doug

Re: [PATCH 04/23] sched,acpi_pad: Convert to sched_set_fifo*()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 06:45:36PM +0200, Dietmar Eggemann wrote:
> On 22/04/2020 13:27, Peter Zijlstra wrote:
> > Because SCHED_FIFO is a broken scheduler model (see previous patches)
> > take away the priority field, the kernel can't possibly make an
> > informed decision.
> > 
> > In this case, use fifo_low, because it only cares about being above
> > SCHED_NORMAL. Effectively no change in behaviour.
> > 
> > XXX: this driver is still complete crap; why isn't it using proper
> > idle injection or at the very least play_idle() ?
> > 
> > Cc: rafael.j.wysocki@intel.com
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reviewed-by: Ingo Molnar <mingo@kernel.org>
> > ---
> >  drivers/acpi/acpi_pad.c |    3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > --- a/drivers/acpi/acpi_pad.c
> > +++ b/drivers/acpi/acpi_pad.c
> > @@ -136,12 +136,11 @@ static unsigned int idle_pct = 5; /* per
> >  static unsigned int round_robin_time = 1; /* second */
> >  static int power_saving_thread(void *data)
> >  {
> > -	struct sched_param param = {.sched_priority = 1};
> >  	int do_sleep;
> >  	unsigned int tsk_index = (unsigned long)data;
> >  	u64 last_jiffies = 0;
> >  
> > -	sched_setscheduler(current, SCHED_RR, &param);
> 
> I was wondering what happened to the SCHED_RR cases but as I can see now
> they are handled here and in the next patch.

Oh right; I completely forgot to mention that in the Changelog didn't I
:-(

In this case, this driver is a broken piece of crap and doing fake idle
with RR is just plain idiotic. Also note the WARNs in play_idle().

Also, rjw, what was the point of renmaing play_idle() to
play_idle_precise() if there is only one anyway? The changelog talks
about adding play_idle_precise() but the patch (rightfully) doesn't add
another version but replaces the existing one. But why change the name?!

Re: [PATCH 05/23] sched,drbd: Convert to sched_set_fifo*()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 01:27:24PM +0200, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> In this case, use fifo_low, because it only cares about being above
> SCHED_NORMAL. Effectively changes prio from 2 to 1.
> 
> Cc: axboe@kernel.dk
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  drivers/block/drbd/drbd_receiver.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- a/drivers/block/drbd/drbd_receiver.c
> +++ b/drivers/block/drbd/drbd_receiver.c
> @@ -6020,9 +6020,8 @@ int drbd_ack_receiver(struct drbd_thread
>  	unsigned int header_size = drbd_header_size(connection);
>  	int expect   = header_size;
>  	bool ping_timeout_active = false;
> -	struct sched_param param = { .sched_priority = 2 };
>  
> -	rv = sched_setscheduler(current, SCHED_RR, &param);
> +	rv = sched_set_fifo_low(current);

As noted by Dietmar, I forgot to mention loosing RR in the changelog,
bad me.

In this case I'm not actually 100% sure, but there was no comment with
it that justified it being RR, and RR-SMP is not optimal (in fact it's
rather buggered).

In general RR is even more 'interesting' to get right thatn FIFO and
therefore I figured it probably didn't want to be RR, but given there
can be multiple of such threads, it might have been an attempt at
providing some sort of fairness between the multiple threads.

At the same time, if you're running the threads so hard that RR makes a
difference, it's unlikely there is any actual NORMAL time left and
things will be unhappy anyway.

Therefore, and me being lazy, make it FIFO.

Re: [PATCH 10/23] sched,mmc: Convert to sched_set_fifo*()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 06:59:35PM +0200, Ulf Hansson wrote:
> On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Because SCHED_FIFO is a broken scheduler model (see previous patches)
> > take away the priority field, the kernel can't possibly make an
> > informed decision.
> >
> > In this case, use fifo_low, because it only cares about being above
> > SCHED_NORMAL. Effectively no change in behaviour.
> >
> > Cc: ulf.hansson@linaro.org
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reviewed-by: Ingo Molnar <mingo@kernel.org>
> 
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Thanks!

> FYI: I am slowly moving towards removing the entire kthread for the
> sdio_irq_thread(). It shouldn't be too far off to be posted, one or
> two kernel releases or so.

Moving over to regular threaded interrupts? Anyway, cool, if these
series collide it's easy enough to drop this patch on the floor if it
turns out obsolete.

Re: [PATCH 10/23] sched,mmc: Convert to sched_set_fifo*()

[Ulf Hansson]

On Thu, 23 Apr 2020 at 10:59, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Apr 22, 2020 at 06:59:35PM +0200, Ulf Hansson wrote:
> > On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > Because SCHED_FIFO is a broken scheduler model (see previous patches)
> > > take away the priority field, the kernel can't possibly make an
> > > informed decision.
> > >
> > > In this case, use fifo_low, because it only cares about being above
> > > SCHED_NORMAL. Effectively no change in behaviour.
> > >
> > > Cc: ulf.hansson@linaro.org
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > Reviewed-by: Ingo Molnar <mingo@kernel.org>
> >
> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Thanks!
>
> > FYI: I am slowly moving towards removing the entire kthread for the
> > sdio_irq_thread(). It shouldn't be too far off to be posted, one or
> > two kernel releases or so.
>
> Moving over to regular threaded interrupts? Anyway, cool, if these
> series collide it's easy enough to drop this patch on the floor if it
> turns out obsolete.

In principle, the only reason for the kthread is that we need it for
polling - for hosts that don't support SDIO irqs. So, I am thinking of
replacing the kthread with a workqueue, as we already have one for
hosts that are using non-threaded IRQs.

Kind regards
Uffe

Re: [PATCH 09/23] sched,ivtv: Convert to sched_set_fifo*()

[Hans Verkuil]

On 22/04/2020 13:27, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Effectively changes from 99 to 50.
> 
> Cc: hverkuil@xs4all.nl
> Cc: awalls@md.metrocast.net
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>

Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

I didn't see any difference in behavior when I changed the prio from 99 to 50,
so it doesn't seem to be a harmful change :-)

Regards,

	Hans

> ---
>  drivers/media/pci/ivtv/ivtv-driver.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> --- a/drivers/media/pci/ivtv/ivtv-driver.c
> +++ b/drivers/media/pci/ivtv/ivtv-driver.c
> @@ -737,8 +737,6 @@ static void ivtv_process_options(struct
>   */
>  static int ivtv_init_struct1(struct ivtv *itv)
>  {
> -	struct sched_param param = { .sched_priority = 99 };
> -
>  	itv->base_addr = pci_resource_start(itv->pdev, 0);
>  	itv->enc_mbox.max_mbox = 2; /* the encoder has 3 mailboxes (0-2) */
>  	itv->dec_mbox.max_mbox = 1; /* the decoder has 2 mailboxes (0-1) */
> @@ -758,7 +756,7 @@ static int ivtv_init_struct1(struct ivtv
>  		return -1;
>  	}
>  	/* must use the FIFO scheduler as it is realtime sensitive */
> -	sched_setscheduler(itv->irq_worker_task, SCHED_FIFO, &param);
> +	sched_set_fifo(itv->irq_worker_task);
>  
>  	kthread_init_work(&itv->irq_work, ivtv_irq_work_handler);
>  
> 
> 

Re: [PATCH 06/23] sched,psci: Convert to sched_set_fifo*()

[Qais Yousef]

On 04/22/20 13:27, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> Effectively changes prio from 99 to 50.
> 
> XXX this thing is horrific, it basically open-codes a stop-machine and
> idle.
> 
> Cc: daniel.lezcano@linaro.org
> Cc: sudeep.holla@arm.com
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  drivers/firmware/psci/psci_checker.c |   11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> --- a/drivers/firmware/psci/psci_checker.c
> +++ b/drivers/firmware/psci/psci_checker.c
> @@ -272,7 +272,6 @@ static int suspend_test_thread(void *arg
>  {
>  	int cpu = (long)arg;
>  	int i, nb_suspend = 0, nb_shallow_sleep = 0, nb_err = 0;
> -	struct sched_param sched_priority = { .sched_priority = MAX_RT_PRIO-1 };
>  	struct cpuidle_device *dev;
>  	struct cpuidle_driver *drv;
>  	/* No need for an actual callback, we just want to wake up the CPU. */
> @@ -282,9 +281,8 @@ static int suspend_test_thread(void *arg
>  	wait_for_completion(&suspend_threads_started);
>  
>  	/* Set maximum priority to preempt all other threads on this CPU. */
> -	if (sched_setscheduler_nocheck(current, SCHED_FIFO, &sched_priority))
> -		pr_warn("Failed to set suspend thread scheduler on CPU %d\n",
> -			cpu);
> +	if (sched_set_fifo(current))
> +		pr_warn("Failed to set suspend thread scheduler on CPU %d\n", cpu);
>  
>  	dev = this_cpu_read(cpuidle_devices);
>  	drv = cpuidle_get_cpu_driver(dev);
> @@ -349,11 +347,6 @@ static int suspend_test_thread(void *arg
>  	if (atomic_dec_return_relaxed(&nb_active_threads) == 0)
>  		complete(&suspend_threads_done);
>  
> -	/* Give up on RT scheduling and wait for termination. */
> -	sched_priority.sched_priority = 0;
> -	if (sched_setscheduler_nocheck(current, SCHED_NORMAL, &sched_priority))
> -		pr_warn("Failed to set suspend thread scheduler on CPU %d\n",
> -			cpu);

No need for sched_set_normal() here before the busy loop?

Thanks

--
Qais Yousef

>  	for (;;) {
>  		/* Needs to be set first to avoid missing a wakeup. */
>  		set_current_state(TASK_INTERRUPTIBLE);
> 
> 

Re: [PATCH 06/23] sched,psci: Convert to sched_set_fifo*()

[Valentin Schneider]

On 27/04/20 17:35, Qais Yousef wrote:
>>      drv = cpuidle_get_cpu_driver(dev);
>> @@ -349,11 +347,6 @@ static int suspend_test_thread(void *arg
>>      if (atomic_dec_return_relaxed(&nb_active_threads) == 0)
>>              complete(&suspend_threads_done);
>>
>> -	/* Give up on RT scheduling and wait for termination. */
>> -	sched_priority.sched_priority = 0;
>> -	if (sched_setscheduler_nocheck(current, SCHED_NORMAL, &sched_priority))
>> -		pr_warn("Failed to set suspend thread scheduler on CPU %d\n",
>> -			cpu);
>
> No need for sched_set_normal() here before the busy loop?
>

Given the tasks become TASK_INTERRUPTIBLE, and the only extra thing they'll
do is exit (barring the parking weirdness), changing them back to CFS
seems superfluous.

> Thanks

Re: [PATCH 01/23] sched: Provide sched_set_fifo()

[Qais Yousef]

On 04/22/20 13:27, Peter Zijlstra wrote:
> SCHED_FIFO (or any static priority scheduler) is a broken scheduler
> model; it is fundamentally incapable of resource management, the one
> thing an OS is actually supposed to do.
> 
> It is impossible to compose static priority workloads. One cannot take
> two well designed and functional static priority workloads and mash
> them together and still expect them to work.
> 
> Therefore it doesn't make sense to expose the priority field; the
> kernel is fundamentally incapable of setting a sensible value, it
> needs systems knowledge that it doesn't have.
> 
> Take away sched_setschedule() / sched_setattr() from modules and
> replace them with:
> 
>   - sched_set_fifo(p); create a FIFO task (at prio 50)
>   - sched_set_fifo_low(p); create a task higher than NORMAL,
> 	which ends up being a FIFO task at prio 1.
>   - sched_set_normal(p, nice); (re)set the task to normal
> 
> This stops the proliferation of randomly chosen, and irrelevant, FIFO
> priorities that dont't really mean anything anyway.
> 
> The system administrator/integrator, whoever has insight into the
> actual system design and requirements (userspace) can set-up
> appropriate priorities if and when needed.
> 
> Cc: airlied@redhat.com
> Cc: alexander.deucher@amd.com
> Cc: awalls@md.metrocast.net
> Cc: axboe@kernel.dk
> Cc: broonie@kernel.org
> Cc: daniel.lezcano@linaro.org
> Cc: gregkh@linuxfoundation.org
> Cc: hannes@cmpxchg.org
> Cc: herbert@gondor.apana.org.au
> Cc: hverkuil@xs4all.nl
> Cc: john.stultz@linaro.org
> Cc: nico@fluxnic.net
> Cc: paulmck@kernel.org
> Cc: rafael.j.wysocki@intel.com
> Cc: rmk+kernel@arm.linux.org.uk
> Cc: sudeep.holla@arm.com
> Cc: tglx@linutronix.de
> Cc: ulf.hansson@linaro.org
> Cc: wim@linux-watchdog.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---

The random priorities were horrible indeed.

FWIW

Reviewed-by: Qais Yousef <qais.yousef@arm.com>

Thanks

--
Qais Yousef

>  include/linux/sched.h |    3 +++
>  kernel/sched/core.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1631,6 +1631,9 @@ extern int idle_cpu(int cpu);
>  extern int available_idle_cpu(int cpu);
>  extern int sched_setscheduler(struct task_struct *, int, const struct sched_param *);
>  extern int sched_setscheduler_nocheck(struct task_struct *, int, const struct sched_param *);
> +extern int sched_set_fifo(struct task_struct *p);
> +extern int sched_set_fifo_low(struct task_struct *p);
> +extern int sched_set_normal(struct task_struct *p, int nice);
>  extern int sched_setattr(struct task_struct *, const struct sched_attr *);
>  extern int sched_setattr_nocheck(struct task_struct *, const struct sched_attr *);
>  extern struct task_struct *idle_task(int cpu);
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5055,6 +5055,8 @@ static int _sched_setscheduler(struct ta
>   * @policy: new policy.
>   * @param: structure containing the new RT priority.
>   *
> + * Use sched_set_fifo(), read its comment.
> + *
>   * Return: 0 on success. An error code otherwise.
>   *
>   * NOTE that the task may be already dead.
> @@ -5097,6 +5099,51 @@ int sched_setscheduler_nocheck(struct ta
>  }
>  EXPORT_SYMBOL_GPL(sched_setscheduler_nocheck);
>  
> +/*
> + * 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.
> + */
> +int sched_set_fifo(struct task_struct *p)
> +{
> +	struct sched_param sp = { .sched_priority = MAX_RT_PRIO / 2 };
> +	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
> +}
> +EXPORT_SYMBOL_GPL(sched_set_fifo);
> +
> +/*
> + * For when you don't much care about FIFO, but want to be above SCHED_NORMAL.
> + */
> +int sched_set_fifo_low(struct task_struct *p)
> +{
> +	struct sched_param sp = { .sched_priority = 1 };
> +	return sched_setscheduler_nocheck(p, SCHED_FIFO, &sp);
> +}
> +EXPORT_SYMBOL_GPL(sched_set_fifo_low);
> +
> +int sched_set_normal(struct task_struct *p, int nice)
> +{
> +	struct sched_attr attr = {
> +		.sched_policy = SCHED_NORMAL,
> +		.sched_nice = nice,
> +	};
> +	return sched_setattr_nocheck(p, &attr);
> +}
> +EXPORT_SYMBOL_GPL(sched_set_normal);
> +
>  static int
>  do_sched_setscheduler(pid_t pid, int policy, struct sched_param __user *param)
>  {
> 
>