[PATCH] sched/deadline: overrun could happen in start_hrtick_dl

[PATCH] sched/deadline: overrun could happen in start_hrtick_dl

[xiaofeng.yan]

It could be wrong for the precision of runtime and deadline
when the precision is within microsecond level. For example:
Task runtime deadline period
 P1   200us   500us   500us

This case need enbale HRTICK feature by the next command
PC#echo "HRTICK" > /sys/kernel/debug/sched_features
PC#trace-cmd record -e sched_switch &
PC#./schedtool -E -t 200000:500000 -e ./test

Some of runtime and deadline run with millisecond level by
reading kernershark. Some pieces of trace.dat are as follows:
(remove some irrelevant information)
<idle>-0   157.603157: sched_switch: :R ==> 2481:4294967295: test
test-2481  157.603203: sched_switch:  2481:R ==> 0:120: swapper/2
<idle>-0   157.605657: sched_switch:  :R ==> 2481:4294967295: test
test-2481  157.608183: sched_switch:  2481:R ==> 2483:120: trace-cmd
trace-cmd-2483 157.609656: sched_switch:2483:R==>2481:4294967295: test

We can get the runtime from the information at some point.
runtime = 157.605657 - 157.608183
runtime = 0.002526(2.526ms)
The correct runtime should be less than or equal to 200us at some point.

The problem is caused by a conditional judgment "delta > 10000".
Because no hrtimer start up to control the runtime when runtime is less than 10us.
So the process will continue to run until tick-period coming.
For fixing this problem, Let delta is equal to 10us when it is less than 10us.
So the hrtimer will start up to control the end of process.

Signed-off-by: Xiaofeng Yan <xiaofeng.yan@huawei.com>
---
 kernel/sched/deadline.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fc4f98b1..b71c229 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -997,10 +997,8 @@ static void check_preempt_curr_dl(struct rq *rq, struct task_struct *p,
 #ifdef CONFIG_SCHED_HRTICK
 static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
 {
-	s64 delta = p->dl.dl_runtime - p->dl.runtime;
-
-	if (delta > 10000)
-		hrtick_start(rq, p->dl.runtime);
+	delta = max_t(s64, 10000LL, delta);
+	hrtick_start(rq, delta);
 }
 #endif
 
-- 
1.7.9.5

Re: [PATCH] sched/deadline: overrun could happen in start_hrtick_dl

[xiaofeng.yan]

On 2014/7/8 16:53, xiaofeng.yan wrote:

Sorry, I send a old patch and send a new one later.

Thanks
Yan
> It could be wrong for the precision of runtime and deadline
> when the precision is within microsecond level. For example:
> Task runtime deadline period
>   P1   200us   500us   500us
>
> This case need enbale HRTICK feature by the next command
> PC#echo "HRTICK" > /sys/kernel/debug/sched_features
> PC#trace-cmd record -e sched_switch &
> PC#./schedtool -E -t 200000:500000 -e ./test
>
> Some of runtime and deadline run with millisecond level by
> reading kernershark. Some pieces of trace.dat are as follows:
> (remove some irrelevant information)
> <idle>-0   157.603157: sched_switch: :R ==> 2481:4294967295: test
> test-2481  157.603203: sched_switch:  2481:R ==> 0:120: swapper/2
> <idle>-0   157.605657: sched_switch:  :R ==> 2481:4294967295: test
> test-2481  157.608183: sched_switch:  2481:R ==> 2483:120: trace-cmd
> trace-cmd-2483 157.609656: sched_switch:2483:R==>2481:4294967295: test
>
> We can get the runtime from the information at some point.
> runtime = 157.605657 - 157.608183
> runtime = 0.002526(2.526ms)
> The correct runtime should be less than or equal to 200us at some point.
>
> The problem is caused by a conditional judgment "delta > 10000".
> Because no hrtimer start up to control the runtime when runtime is less than 10us.
> So the process will continue to run until tick-period coming.
> For fixing this problem, Let delta is equal to 10us when it is less than 10us.
> So the hrtimer will start up to control the end of process.
>
> Signed-off-by: Xiaofeng Yan <xiaofeng.yan@huawei.com>
> ---
>   kernel/sched/deadline.c |    6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fc4f98b1..b71c229 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -997,10 +997,8 @@ static void check_preempt_curr_dl(struct rq *rq, struct task_struct *p,
>   #ifdef CONFIG_SCHED_HRTICK
>   static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
>   {
> -	s64 delta = p->dl.dl_runtime - p->dl.runtime;
> -
> -	if (delta > 10000)
> -		hrtick_start(rq, p->dl.runtime);
> +	delta = max_t(s64, 10000LL, delta);
> +	hrtick_start(rq, delta);
>   }
>   #endif
>   

Re: [PATCH] sched/deadline: overrun could happen in start_hrtick_dl

[Peter Zijlstra]

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

On Tue, Jul 08, 2014 at 08:53:27AM +0000, xiaofeng.yan wrote:
>  static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
>  {
> -	s64 delta = p->dl.dl_runtime - p->dl.runtime;
> -
> -	if (delta > 10000)
> -		hrtick_start(rq, p->dl.runtime);
> +	delta = max_t(s64, 10000LL, delta);
> +	hrtick_start(rq, delta);
>  }

no, no, no. I said to unify the test.

---
 kernel/sched/core.c     | 9 ++++++++-
 kernel/sched/deadline.c | 3 +--
 kernel/sched/fair.c     | 7 -------
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e1a2f31bb0cb..c7b8a6fbac66 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -444,7 +444,14 @@ static void __hrtick_start(void *arg)
 void hrtick_start(struct rq *rq, u64 delay)
 {
 	struct hrtimer *timer = &rq->hrtick_timer;
-	ktime_t time = ktime_add_ns(timer->base->get_time(), delay);
+	ktime_t time;
+
+	/*
+	 * Don't schedule slices shorter than 10000ns, that just
+	 * doesn't make sense and can cause timer DoS.
+	 */
+	delta = max_t(s64, delta, 10000LL);
+	time = ktime_add_ns(timer->base->get_time(), delay);
 
 	hrtimer_set_expires(timer, time);
 
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fc4f98b1258f..e1e24eea8061 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -999,8 +999,7 @@ static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
 {
 	s64 delta = p->dl.dl_runtime - p->dl.runtime;
 
-	if (delta > 10000)
-		hrtick_start(rq, p->dl.runtime);
+	hrtick_start(rq, p->dl.runtime);
 }
 #endif
 
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 923fe32db6b3..713c58d2a7b0 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3901,13 +3901,6 @@ static void hrtick_start_fair(struct rq *rq, struct task_struct *p)
 			return;
 		}
 
-		/*
-		 * Don't schedule slices shorter than 10000ns, that just
-		 * doesn't make sense. Rely on vruntime for fairness.
-		 */
-		if (rq->curr != p)
-			delta = max_t(s64, 10000LL, delta);
-
 		hrtick_start(rq, delta);
 	}
 }

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sched/deadline: overrun could happen in start_hrtick_dl

[Bernd Petrovitsch]

On Die, 2014-07-08 at 11:33 +0200, Peter Zijlstra wrote:
[...]
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fc4f98b1258f..e1e24eea8061 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -999,8 +999,7 @@ static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
>  {
>  	s64 delta = p->dl.dl_runtime - p->dl.runtime;

This line can be removed as well (an then we have a one-line
function ....).

> -	if (delta > 10000)
> -		hrtick_start(rq, p->dl.runtime);
> +	hrtick_start(rq, p->dl.runtime);
>  }
>  #endif


	Bernd
-- 
"I dislike type abstraction if it has no real reason. And saving
on typing is not a good reason - if your typing speed is the main
issue when you're coding, you're doing something seriously wrong."
    - Linus Torvalds

Re: [PATCH] sched/deadline: overrun could happen in start_hrtick_dl

[xiaofeng.yan]

On 2014/7/8 17:33, Peter Zijlstra wrote:
> On Tue, Jul 08, 2014 at 08:53:27AM +0000, xiaofeng.yan wrote:
>>   static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
>>   {
>> -	s64 delta = p->dl.dl_runtime - p->dl.runtime;
>> -
>> -	if (delta > 10000)
>> -		hrtick_start(rq, p->dl.runtime);
>> +	delta = max_t(s64, 10000LL, delta);
>> +	hrtick_start(rq, delta);
>>   }
> no, no, no. I said to unify the test.

I understand your idea after reading the next patch. This is good solution.
I will test it with your patch.

> ---
>   kernel/sched/core.c     | 9 ++++++++-
>   kernel/sched/deadline.c | 3 +--
>   kernel/sched/fair.c     | 7 -------
>   3 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index e1a2f31bb0cb..c7b8a6fbac66 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -444,7 +444,14 @@ static void __hrtick_start(void *arg)
>   void hrtick_start(struct rq *rq, u64 delay)
>   {
>   	struct hrtimer *timer = &rq->hrtick_timer;
> -	ktime_t time = ktime_add_ns(timer->base->get_time(), delay);
> +	ktime_t time;
> +
> +	/*
> +	 * Don't schedule slices shorter than 10000ns, that just
> +	 * doesn't make sense and can cause timer DoS.
> +	 */
> +	delta = max_t(s64, delta, 10000LL);

transfer the argument delta to delay

> +	time = ktime_add_ns(timer->base->get_time(), delay);
>   
>   	hrtimer_set_expires(timer, time);
>   
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fc4f98b1258f..e1e24eea8061 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -999,8 +999,7 @@ static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
>   {
>   	s64 delta = p->dl.dl_runtime - p->dl.runtime;
>   
> -	if (delta > 10000)
> -		hrtick_start(rq, p->dl.runtime);
> +	hrtick_start(rq, p->dl.runtime);
>   }
>   #endif
>   
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 923fe32db6b3..713c58d2a7b0 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3901,13 +3901,6 @@ static void hrtick_start_fair(struct rq *rq, struct task_struct *p)
>   			return;
>   		}
>   
> -		/*
> -		 * Don't schedule slices shorter than 10000ns, that just
> -		 * doesn't make sense. Rely on vruntime for fairness.
> -		 */
> -		if (rq->curr != p)
> -			delta = max_t(s64, 10000LL, delta);
> -
>   		hrtick_start(rq, delta);
>   	}
>   }

Re: [PATCH] sched/deadline: overrun could happen in start_hrtick_dl

[xiaofeng.yan]

On 2014/7/8 19:23, xiaofeng.yan wrote:
> On 2014/7/8 17:33, Peter Zijlstra wrote:
>> On Tue, Jul 08, 2014 at 08:53:27AM +0000, xiaofeng.yan wrote:
>>>   static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
>>>   {
>>> -    s64 delta = p->dl.dl_runtime - p->dl.runtime;
>>> -
>>> -    if (delta > 10000)
>>> -        hrtick_start(rq, p->dl.runtime);
>>> +    delta = max_t(s64, 10000LL, delta);
>>> +    hrtick_start(rq, delta);
>>>   }
>> no, no, no. I said to unify the test.
>
> I understand your idea after reading the next patch. This is good 
> solution.
> I will test it with your patch.
>

I have tested this solution, It can work very well with deadline 
schedule class.

  kernel/sched/core.c     |    9 ++++++++-
  kernel/sched/deadline.c |    5 +----
  kernel/sched/fair.c     |    8 --------
  3 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3bdf01b..cc9a058 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -442,8 +442,15 @@ static void __hrtick_start(void *arg)
  void hrtick_start(struct rq *rq, u64 delay)
  {
         struct hrtimer *timer = &rq->hrtick_timer;
-       ktime_t time = ktime_add_ns(timer->base->get_time(), delay);

+       ktime_t time;
+
+       /*
+        * Don't schedule slices shorter than 10000ns, that just
+        * doesn't make sense and can cause timer DoS.
+        */
+       s64 delta = max_t(s64, delay, 10000LL);
+       time = ktime_add_ns(timer->base->get_time(), delta);
         hrtimer_set_expires(timer, time);

         if (rq == this_rq()) {
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fc4f98b1..9135771 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -997,10 +997,7 @@ static void check_preempt_curr_dl(struct rq *rq, 
struct task_struct *p,
  #ifdef CONFIG_SCHED_HRTICK
static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
  {
-       s64 delta = p->dl.dl_runtime - p->dl.runtime;
-
-       if (delta > 10000)
-               hrtick_start(rq, p->dl.runtime);
+       hrtick_start(rq, p->dl.runtime);
  }
  #endif

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index fea7d33..e5cfd57 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3857,14 +3857,6 @@ static void hrtick_start_fair(struct rq *rq, 
struct task_struct *p)
                                 resched_task(p);
                         return;
                 }
-
-               /*
-                * Don't schedule slices shorter than 10000ns, that just
-                * doesn't make sense. Rely on vruntime for fairness.
-                */
-               if (rq->curr != p)
-                       delta = max_t(s64, 10000LL, delta);
-
                 hrtick_start(rq, delta);
         }
  }
-- 


>> ---
>>   kernel/sched/core.c     | 9 ++++++++-
>>   kernel/sched/deadline.c | 3 +--
>>   kernel/sched/fair.c     | 7 -------
>>   3 files changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index e1a2f31bb0cb..c7b8a6fbac66 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -444,7 +444,14 @@ static void __hrtick_start(void *arg)
>>   void hrtick_start(struct rq *rq, u64 delay)
>>   {
>>       struct hrtimer *timer = &rq->hrtick_timer;
>> -    ktime_t time = ktime_add_ns(timer->base->get_time(), delay);
>> +    ktime_t time;
>> +
>> +    /*
>> +     * Don't schedule slices shorter than 10000ns, that just
>> +     * doesn't make sense and can cause timer DoS.
>> +     */
>> +    delta = max_t(s64, delta, 10000LL);
>
> transfer the argument delta to delay
>
>> +    time = ktime_add_ns(timer->base->get_time(), delay);
>>         hrtimer_set_expires(timer, time);
>>   diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>> index fc4f98b1258f..e1e24eea8061 100644
>> --- a/kernel/sched/deadline.c
>> +++ b/kernel/sched/deadline.c
>> @@ -999,8 +999,7 @@ static void start_hrtick_dl(struct rq *rq, struct 
>> task_struct *p)
>>   {
>>       s64 delta = p->dl.dl_runtime - p->dl.runtime;
>>   -    if (delta > 10000)
>> -        hrtick_start(rq, p->dl.runtime);
>> +    hrtick_start(rq, p->dl.runtime);
>>   }
>>   #endif
>>   diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 923fe32db6b3..713c58d2a7b0 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -3901,13 +3901,6 @@ static void hrtick_start_fair(struct rq *rq, 
>> struct task_struct *p)
>>               return;
>>           }
>>   -        /*
>> -         * Don't schedule slices shorter than 10000ns, that just
>> -         * doesn't make sense. Rely on vruntime for fairness.
>> -         */
>> -        if (rq->curr != p)
>> -            delta = max_t(s64, 10000LL, delta);
>> -
>>           hrtick_start(rq, delta);
>>       }
>>   }
>
>
> -- 
> 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] sched/deadline: overrun could happen in start_hrtick_dl

[Peter Zijlstra]

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

On Tue, Jul 08, 2014 at 07:50:22PM +0800, xiaofeng.yan wrote:
> I have tested this solution, It can work very well with deadline schedule
> class.

Great, please send it as a proper patch and I might just press 'A' ;-)

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sched/deadline: overrun could happen in start_hrtick_dl

[xiaofeng.yan]

On 2014/7/8 20:52, Peter Zijlstra wrote:
> On Tue, Jul 08, 2014 at 07:50:22PM +0800, xiaofeng.yan wrote:
>> I have tested this solution, It can work very well with deadline schedule
>> class.
> Great, please send it as a proper patch and I might just press 'A' ;-)

Ok, I will send it later.

[PATCH] sched/deadline: overrun could happen in start_hrtick_dl

[xiaofeng.yan]

It could be wrong for the precision of runtime and deadline
when the precision is within microsecond level. For example:
Task runtime deadline period
 P1   200us   500us   500us

This case need enbale HRTICK feature by the next command
PC#echo "HRTICK" > /sys/kernel/debug/sched_features
PC#trace-cmd record -e sched_switch &
PC#./schedtool -E -t 200000:500000 -e ./test

Some of runtime and deadline run with millisecond level by
reading kernershark. Some pieces of trace.dat are as follows:
(remove some irrelevant information)
<idle>-0   157.603157: sched_switch: :R ==> 2481:4294967295: test
test-2481  157.603203: sched_switch:  2481:R ==> 0:120: swapper/2
<idle>-0   157.605657: sched_switch:  :R ==> 2481:4294967295: test
test-2481  157.608183: sched_switch:  2481:R ==> 2483:120: trace-cmd
trace-cmd-2483 157.609656: sched_switch:2483:R==>2481:4294967295: test

We can get the runtime from the information at some point.
runtime = 157.605657 - 157.608183
runtime = 0.002526(2.526ms)
The correct runtime should be less than or equal to 200us at some point.

The problem is caused by a conditional judgment "delta > 10000".
Because no hrtimer start up to control the runtime when runtime is less than 10us.
So the process will continue to run until tick-period coming.
For fixing this problem, Let delta is equal to 10us when it is less than 10us.
So the hrtimer will start up to control the end of process.

Signed-off-by: Xiaofeng Yan <xiaofeng.yan@huawei.com>
---
 kernel/sched/deadline.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fc4f98b1..51e6b0e 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -997,10 +997,8 @@ static void check_preempt_curr_dl(struct rq *rq, struct task_struct *p,
 #ifdef CONFIG_SCHED_HRTICK
 static void start_hrtick_dl(struct rq *rq, struct task_struct *p)
 {
-	s64 delta = p->dl.dl_runtime - p->dl.runtime;
-
-	if (delta > 10000)
-		hrtick_start(rq, p->dl.runtime);
+	s64 delta = max_t(s64, 10000LL, p->dl.runtime);
+	hrtick_start(rq, delta);
 }
 #endif
 
-- 
1.7.9.5