[PATCH 1/2] sched/deadline: introduce start_hrtick_dl for !CONFIG_SCHED_HRTICK

[PATCH 1/2] sched/deadline: introduce start_hrtick_dl for !CONFIG_SCHED_HRTICK

[Wanpeng Li]

Introduce start_hrtick_dl for !CONFIG_SCHED_HRTICK to align with
fair class.

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
---
 kernel/sched/deadline.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 04c2cbb..56674f6 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1013,6 +1013,10 @@ static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
 {
 	hrtick_start(rq, p->dl.runtime);
 }
+#else /* !CONFIG_SCHED_HRTICK */
+static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
+{
+}
 #endif
 
 static struct sched_dl_entity *pick_next_dl_entity(struct rq *rq,
@@ -1066,10 +1070,8 @@ struct task_struct *pick_next_task_dl(struct rq *rq, struct task_struct *prev)
 	/* Running task will never be pushed. */
        dequeue_pushable_dl_task(rq, p);
 
-#ifdef CONFIG_SCHED_HRTICK
 	if (hrtick_enabled(rq))
 		start_hrtick_dl(rq, p);
-#endif
 
 	set_post_schedule(rq);
 
@@ -1088,10 +1090,8 @@ static void task_tick_dl(struct rq *rq, struct task_struct *p, int queued)
 {
 	update_curr_dl(rq);
 
-#ifdef CONFIG_SCHED_HRTICK
 	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0)
 		start_hrtick_dl(rq, p);
-#endif
 }
 
 static void task_fork_dl(struct task_struct *p)
-- 
1.9.1

[PATCH 2/2] sched/deadline: fix start high-res preemption tick for a non-running task

[Wanpeng Li]

Queued ticks are scheduled to match the budget, which means the budget 
is overall consumed and the dl task should be throttled. Dl task will 
be replenished immediately if fail to start a dl timer. 

However, the curr maybe not the left most dl task in the rb tree any 
more after this immediately replenished and reschedule is needed. 
Start high-res preemption tick for this upcoming rescheduled dl task 
is not correct. 

This patch fix it by not starting high-res preemption tick for a 
non-running dl task.

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
---
 kernel/sched/deadline.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 56674f6..2a6a5bb 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1090,7 +1090,8 @@ static void task_tick_dl(struct rq *rq, struct task_struct *p, int queued)
 {
 	update_curr_dl(rq);
 
-	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0)
+	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0 &&
+	    is_leftmost(p, &rq->dl))
 		start_hrtick_dl(rq, p);
 }
 
-- 
1.9.1

Re: [PATCH 1/2] sched/deadline: introduce start_hrtick_dl for !CONFIG_SCHED_HRTICK

[Juri Lelli]

Hi,

On 11/11/14 01:52, Wanpeng Li wrote:
> Introduce start_hrtick_dl for !CONFIG_SCHED_HRTICK to align with
> fair class.
> 
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> ---
>  kernel/sched/deadline.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 04c2cbb..56674f6 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -1013,6 +1013,10 @@ static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
>  {
>  	hrtick_start(rq, p->dl.runtime);
>  }
> +#else /* !CONFIG_SCHED_HRTICK */
> +static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
> +{
> +}
>  #endif
>  
>  static struct sched_dl_entity *pick_next_dl_entity(struct rq *rq,
> @@ -1066,10 +1070,8 @@ struct task_struct *pick_next_task_dl(struct rq *rq, struct task_struct *prev)
>  	/* Running task will never be pushed. */
>         dequeue_pushable_dl_task(rq, p);
>  
> -#ifdef CONFIG_SCHED_HRTICK
>  	if (hrtick_enabled(rq))
>  		start_hrtick_dl(rq, p);
> -#endif
>  
>  	set_post_schedule(rq);
>  
> @@ -1088,10 +1090,8 @@ static void task_tick_dl(struct rq *rq, struct task_struct *p, int queued)
>  {
>  	update_curr_dl(rq);
>  
> -#ifdef CONFIG_SCHED_HRTICK
>  	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0)
>  		start_hrtick_dl(rq, p);
> -#endif
>  }
>  
>  static void task_fork_dl(struct task_struct *p)
> 

Looks good!

Acked-by: Juri Lelli <juri.lelli@arm.com>

Thanks,

- Juri

Re: [PATCH 2/2] sched/deadline: fix start high-res preemption tick for a non-running task

[Juri Lelli]

Hi,

not sure I understand what the problem is here.

On 11/11/14 01:52, Wanpeng Li wrote:
> Queued ticks are scheduled to match the budget, which means the budget 
> is overall consumed and the dl task should be throttled.

... enforce the budget? It means that when the budget is consumed the
task has to be throttled...

> Dl task will 
> be replenished immediately if fail to start a dl timer. 
> 
> However, the curr maybe not the left most dl task in the rb tree any 
> more after this immediately replenished and reschedule is needed. 
> Start high-res preemption tick for this upcoming rescheduled dl task 
> is not correct. 
> 

So, the task that is going to preempt curr is picked by
pick_next_task_dl(), that correctly starts the hrtick for this new task.

Maybe you can add more information about what you are seeing? A callpath
maybe?

Thanks,

- Juri

> This patch fix it by not starting high-res preemption tick for a 
> non-running dl task.
> 
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> ---
>  kernel/sched/deadline.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 56674f6..2a6a5bb 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -1090,7 +1090,8 @@ static void task_tick_dl(struct rq *rq, struct task_struct *p, int queued)
>  {
>  	update_curr_dl(rq);
>  
> -	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0)
> +	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0 &&
> +	    is_leftmost(p, &rq->dl))
>  		start_hrtick_dl(rq, p);
>  }
>  
> 

Re: [PATCH 2/2] sched/deadline: fix start high-res preemption tick for a non-running task

[Wanpeng Li]

Hi Juri,
On 11/13/14, 5:37 PM, Juri Lelli wrote:
> Hi,
>
> not sure I understand what the problem is here.
>
> On 11/11/14 01:52, Wanpeng Li wrote:
>> Queued ticks are scheduled to match the budget, which means the budget
>> is overall consumed and the dl task should be throttled.
> ... enforce the budget? It means that when the budget is consumed the
> task has to be throttled...

Agreed.

>
>> Dl task will
>> be replenished immediately if fail to start a dl timer.
>>
>> However, the curr maybe not the left most dl task in the rb tree any
>> more after this immediately replenished and reschedule is needed.
>> Start high-res preemption tick for this upcoming rescheduled dl task
>> is not correct.
>>
> So, the task that is going to preempt curr is picked by
> pick_next_task_dl(), that correctly starts the hrtick for this new task.
>
> Maybe you can add more information about what you are seeing? A callpath
> maybe?

The parameter of task_tick_dl() queued == 1 means that hrtick is fired. 
hrtick() => task_tick_dl( , ,1), so p->dl.runtime should be <= 0 if 
queued == 1. What I see is queued == 1 && p->dl.runtime > 0 && p is not 
the left most task and hrtick is start for this task.

Regards,
Wanpeng Li

>
> Thanks,
>
> - Juri
>
>> This patch fix it by not starting high-res preemption tick for a
>> non-running dl task.
>>
>> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
>> ---
>>   kernel/sched/deadline.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>> index 56674f6..2a6a5bb 100644
>> --- a/kernel/sched/deadline.c
>> +++ b/kernel/sched/deadline.c
>> @@ -1090,7 +1090,8 @@ static void task_tick_dl(struct rq *rq, struct task_struct *p, int queued)
>>   {
>>   	update_curr_dl(rq);
>>   
>> -	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0)
>> +	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0 &&
>> +	    is_leftmost(p, &rq->dl))
>>   		start_hrtick_dl(rq, p);
>>   }
>>   
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[tip:sched/core] sched/deadline: Introduce start_hrtick_dl() for !CONFIG_SCHED_HRTICK

[tip-bot for Wanpeng Li]

Commit-ID:  36ce98818a4df66c8134c31fd6e768b4119c7a90
Gitweb:     http://git.kernel.org/tip/36ce98818a4df66c8134c31fd6e768b4119c7a90
Author:     Wanpeng Li <wanpeng.li@linux.intel.com>
AuthorDate: Tue, 11 Nov 2014 09:52:26 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sun, 16 Nov 2014 10:59:03 +0100

sched/deadline: Introduce start_hrtick_dl() for !CONFIG_SCHED_HRTICK

Introduce start_hrtick_dl for !CONFIG_SCHED_HRTICK to align with
the fair class.

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@arm.com>
Cc: Kirill Tkhai <ktkhai@parallels.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1415670747-58726-1-git-send-email-wanpeng.li@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/deadline.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 9594c12..e5db8c6 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1013,6 +1013,10 @@ static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
 {
 	hrtick_start(rq, p->dl.runtime);
 }
+#else /* !CONFIG_SCHED_HRTICK */
+static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
+{
+}
 #endif
 
 static struct sched_dl_entity *pick_next_dl_entity(struct rq *rq,
@@ -1066,10 +1070,8 @@ struct task_struct *pick_next_task_dl(struct rq *rq, struct task_struct *prev)
 	/* Running task will never be pushed. */
        dequeue_pushable_dl_task(rq, p);
 
-#ifdef CONFIG_SCHED_HRTICK
 	if (hrtick_enabled(rq))
 		start_hrtick_dl(rq, p);
-#endif
 
 	set_post_schedule(rq);
 
@@ -1088,10 +1090,8 @@ static void task_tick_dl(struct rq *rq, struct task_struct *p, int queued)
 {
 	update_curr_dl(rq);
 
-#ifdef CONFIG_SCHED_HRTICK
 	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0)
 		start_hrtick_dl(rq, p);
-#endif
 }
 
 static void task_fork_dl(struct task_struct *p)

Re: [PATCH 2/2] sched/deadline: fix start high-res preemption tick for a non-running task

[Juri Lelli]

Hi,

On 13/11/14 10:30, Wanpeng Li wrote:
> Hi Juri,
> On 11/13/14, 5:37 PM, Juri Lelli wrote:
>> Hi,
>>
>> not sure I understand what the problem is here.
>>
>> On 11/11/14 01:52, Wanpeng Li wrote:
>>> Queued ticks are scheduled to match the budget, which means the budget
>>> is overall consumed and the dl task should be throttled.
>> ... enforce the budget? It means that when the budget is consumed the
>> task has to be throttled...
> 
> Agreed.
> 
>>
>>> Dl task will
>>> be replenished immediately if fail to start a dl timer.
>>>
>>> However, the curr maybe not the left most dl task in the rb tree any
>>> more after this immediately replenished and reschedule is needed.
>>> Start high-res preemption tick for this upcoming rescheduled dl task
>>> is not correct.
>>>
>> So, the task that is going to preempt curr is picked by
>> pick_next_task_dl(), that correctly starts the hrtick for this new task.
>>
>> Maybe you can add more information about what you are seeing? A callpath
>> maybe?
> 
> The parameter of task_tick_dl() queued == 1 means that hrtick is fired. 
> hrtick() => task_tick_dl( , ,1), so p->dl.runtime should be <= 0 if 
> queued == 1. What I see is queued == 1 && p->dl.runtime > 0 && p is not 
> the left most task and hrtick is start for this task.
> 

Oh, right, now it makes sense. Good catch! :)

Could you please post a second version with a more explanatory changelog
and maybe a comment just above the check?

Thanks,

- Juri

> Regards,
> Wanpeng Li
> 
>>
>> Thanks,
>>
>> - Juri
>>
>>> This patch fix it by not starting high-res preemption tick for a
>>> non-running dl task.
>>>
>>> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
>>> ---
>>>   kernel/sched/deadline.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>>> index 56674f6..2a6a5bb 100644
>>> --- a/kernel/sched/deadline.c
>>> +++ b/kernel/sched/deadline.c
>>> @@ -1090,7 +1090,8 @@ static void task_tick_dl(struct rq *rq, struct task_struct *p, int queued)
>>>   {
>>>   	update_curr_dl(rq);
>>>   
>>> -	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0)
>>> +	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0 &&
>>> +	    is_leftmost(p, &rq->dl))
>>>   		start_hrtick_dl(rq, p);
>>>   }
>>>   
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH 2/2] sched/deadline: fix start high-res preemption tick for a non-running task

[Wanpeng Li]

Hi Juri,
On 11/17/14, 5:50 PM, Juri Lelli wrote:
> Hi,
>
> On 13/11/14 10:30, Wanpeng Li wrote:
>> Hi Juri,
>> On 11/13/14, 5:37 PM, Juri Lelli wrote:
>>> Hi,
>>>
>>> not sure I understand what the problem is here.
>>>
>>> On 11/11/14 01:52, Wanpeng Li wrote:
>>>> Queued ticks are scheduled to match the budget, which means the budget
>>>> is overall consumed and the dl task should be throttled.
>>> ... enforce the budget? It means that when the budget is consumed the
>>> task has to be throttled...
>> Agreed.
>>
>>>> Dl task will
>>>> be replenished immediately if fail to start a dl timer.
>>>>
>>>> However, the curr maybe not the left most dl task in the rb tree any
>>>> more after this immediately replenished and reschedule is needed.
>>>> Start high-res preemption tick for this upcoming rescheduled dl task
>>>> is not correct.
>>>>
>>> So, the task that is going to preempt curr is picked by
>>> pick_next_task_dl(), that correctly starts the hrtick for this new task.
>>>
>>> Maybe you can add more information about what you are seeing? A callpath
>>> maybe?
>> The parameter of task_tick_dl() queued == 1 means that hrtick is fired.
>> hrtick() => task_tick_dl( , ,1), so p->dl.runtime should be <= 0 if
>> queued == 1. What I see is queued == 1 && p->dl.runtime > 0 && p is not
>> the left most task and hrtick is start for this task.
>>
> Oh, right, now it makes sense. Good catch! :)
>
> Could you please post a second version with a more explanatory changelog
> and maybe a comment just above the check?

Ok, I will do it and send out a new version tomorrow. ;-) Btw, could you 
review this patch? https://lkml.org/lkml/2014/11/13/51 The bug is 
introduced by deadline.

Regards,
Wanpeng Li

>   
> Thanks,
>
> - Juri
>
>> Regards,
>> Wanpeng Li
>>
>>> Thanks,
>>>
>>> - Juri
>>>
>>>> This patch fix it by not starting high-res preemption tick for a
>>>> non-running dl task.
>>>>
>>>> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
>>>> ---
>>>>    kernel/sched/deadline.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>>>> index 56674f6..2a6a5bb 100644
>>>> --- a/kernel/sched/deadline.c
>>>> +++ b/kernel/sched/deadline.c
>>>> @@ -1090,7 +1090,8 @@ static void task_tick_dl(struct rq *rq, struct task_struct *p, int queued)
>>>>    {
>>>>    	update_curr_dl(rq);
>>>>    
>>>> -	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0)
>>>> +	if (hrtick_enabled(rq) && queued && p->dl.runtime > 0 &&
>>>> +	    is_leftmost(p, &rq->dl))
>>>>    		start_hrtick_dl(rq, p);
>>>>    }
>>>>    
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>