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: [email protected]
Cc: [email protected]
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Ingo Molnar <[email protected]>
---
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);
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 <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Reviewed-by: Ingo Molnar <[email protected]>
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: [email protected]
> Cc: [email protected]
Tested the PSCI checker pulling the entire series. Continues to work :)
Tested-by: Sudeep Holla <[email protected]>
--
Regards,
Sudeep
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: [email protected]
> Cc: [email protected]
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Reviewed-by: Ingo Molnar <[email protected]>
> ---
> 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);
>
>
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