In an attempt to take away sched_setscheduler() from modules, change
this into sched_set_normal(.nice = 19).
Cc: [email protected]
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Ingo Molnar <[email protected]>
---
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, ¶m);
+ sched_set_normal(heap->task, 19);
return 0;
}
On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <[email protected]> wrote:
>
> In an attempt to take away sched_setscheduler() from modules, change
> this into sched_set_normal(.nice = 19).
>
> Cc: [email protected]
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Reviewed-by: Ingo Molnar <[email protected]>
> ---
> 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, ¶m);
> + 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;
> }
>
>
On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <[email protected]> wrote:
> >
> > In an attempt to take away sched_setscheduler() from modules, change
> > this into sched_set_normal(.nice = 19).
> >
> > Cc: [email protected]
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > Reviewed-by: Ingo Molnar <[email protected]>
> > ---
> > 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, ¶m);
> > + 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?
On Wed, 22 Apr 2020 at 15:29, Peter Zijlstra <[email protected]> wrote:
>
> On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> > On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <[email protected]> wrote:
> > >
> > > In an attempt to take away sched_setscheduler() from modules, change
> > > this into sched_set_normal(.nice = 19).
> > >
> > > Cc: [email protected]
> > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > > Reviewed-by: Ingo Molnar <[email protected]>
> > > ---
> > > 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, ¶m);
> > > + 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
On Wed, Apr 22, 2020 at 03:36:22PM +0200, Vincent Guittot wrote:
> On Wed, 22 Apr 2020 at 15:29, Peter Zijlstra <[email protected]> wrote:
> >
> > On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> > > On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <[email protected]> wrote:
> > > >
> > > > In an attempt to take away sched_setscheduler() from modules, change
> > > > this into sched_set_normal(.nice = 19).
> > > >
> > > > Cc: [email protected]
> > > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > > > Reviewed-by: Ingo Molnar <[email protected]>
> > > > ---
> > > > 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, ¶m);
> > > > + 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...
On Wed, 22 Apr 2020 at 15:59, Peter Zijlstra <[email protected]> wrote:
>
> On Wed, Apr 22, 2020 at 03:36:22PM +0200, Vincent Guittot wrote:
> > On Wed, 22 Apr 2020 at 15:29, Peter Zijlstra <[email protected]> wrote:
> > >
> > > On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> > > > On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <[email protected]> wrote:
> > > > >
> > > > > In an attempt to take away sched_setscheduler() from modules, change
> > > > > this into sched_set_normal(.nice = 19).
> > > > >
> > > > > Cc: [email protected]
> > > > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > > > > Reviewed-by: Ingo Molnar <[email protected]>
> > > > > ---
> > > > > 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, ¶m);
> > > > > + 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...
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.
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 <[email protected]> wrote:
> > >
> > > On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> > > > On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <[email protected]> wrote:
> > > > >
> > > > > In an attempt to take away sched_setscheduler() from modules, change
> > > > > this into sched_set_normal(.nice = 19).
> > > > >
> > > > > Cc: [email protected]
> > > > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > > > > Reviewed-by: Ingo Molnar <[email protected]>
> > > > > ---
> > > > > 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, ¶m);
> > > > > + 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...
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.
On Wed, 22 Apr 2020 at 17:39, Peter Zijlstra <[email protected]> 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.