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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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);
}
}
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
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);
> }
> }
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
> .
>
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' ;-)
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.