Re: [PATCH v4] sched: fix wrong rq's runnable_avg update with rt tasks
On 10 April 2013 09:26, Peter Zijlstra wrote:
> On Tue, 2013-04-09 at 11:06 +0200, Vincent Guittot wrote:
>> >> +void idle_enter(struct rq *this_rq)
>> >> +{
>> >> + update_rq_runnable_avg(this_rq, 1);
>> >> +}
>
>> >> +void idle_exit(struct rq *this_rq)
>> >> +{
>> >> + update_rq_runnable_avg(this_rq, 0);
>> >> +}
>> >
>> > These seem like fairly unfortunate names to expose to the global
>> > namespace, why not expose update_rq_runnable_avg() instead?
>>
>> Just to gather in one place all cfs actions that should be done when
>> exiting idle even if we only have update_rq_runnable_avg right now. I
>> have distinguished that from idle_balance because this sequence can't
>> generate extra context switch like idle_balance and they would finally
>> not be called in the same time
>
> OK, but could we then please give then more scheduler specific names?
> It just seems to me that idle_enter/idle_exit() are very obvious
> function names for unrelated things.
>
> How about calling it idle_{enter,exit}_fair; so that once other classes
> grow hooks we can do something like:
My primary goal was to align with idle_balance name but
idle_{enter,exit}_fair is better
In the same way, should we change idle_balance to idle_balance_fair ?
and since we don't have Steve's irq constraint anymore, we could move
idle_balance in the beginning of the function pick_next_task_fair ? We
will not have spurious switch context and we will remove fair function
from __schedule function
Vincent
>
> static void pre_schedule_idle(struct rq *rq, struct task_struct *p)
> {
> struct sched_class *class;
>
> for_each_class(class) {
> if (class->idle_enter)
> class->idle_enter(rq);
> }
> }
>
> or whatnot..
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On Tue, 2013-04-09 at 11:06 +0200, Vincent Guittot wrote:
> >> +void idle_enter(struct rq *this_rq)
> >> +{
> >> + update_rq_runnable_avg(this_rq, 1);
> >> +}
> >> +void idle_exit(struct rq *this_rq)
> >> +{
> >> + update_rq_runnable_avg(this_rq, 0);
> >> +}
> >
> > These seem like fairly unfortunate names to expose to the global
> > namespace, why not expose update_rq_runnable_avg() instead?
>
> Just to gather in one place all cfs actions that should be done when
> exiting idle even if we only have update_rq_runnable_avg right now. I
> have distinguished that from idle_balance because this sequence can't
> generate extra context switch like idle_balance and they would finally
> not be called in the same time
OK, but could we then please give then more scheduler specific names?
It just seems to me that idle_enter/idle_exit() are very obvious
function names for unrelated things.
How about calling it idle_{enter,exit}_fair; so that once other classes
grow hooks we can do something like:
static void pre_schedule_idle(struct rq *rq, struct task_struct *p)
{
struct sched_class *class;
for_each_class(class) {
if (class->idle_enter)
class->idle_enter(rq);
}
}
or whatnot..
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On Tue, 2013-04-09 at 11:06 +0200, Vincent Guittot wrote:
+void idle_enter(struct rq *this_rq)
+{
+ update_rq_runnable_avg(this_rq, 1);
+}
+void idle_exit(struct rq *this_rq)
+{
+ update_rq_runnable_avg(this_rq, 0);
+}
These seem like fairly unfortunate names to expose to the global
namespace, why not expose update_rq_runnable_avg() instead?
Just to gather in one place all cfs actions that should be done when
exiting idle even if we only have update_rq_runnable_avg right now. I
have distinguished that from idle_balance because this sequence can't
generate extra context switch like idle_balance and they would finally
not be called in the same time
OK, but could we then please give then more scheduler specific names?
It just seems to me that idle_enter/idle_exit() are very obvious
function names for unrelated things.
How about calling it idle_{enter,exit}_fair; so that once other classes
grow hooks we can do something like:
static void pre_schedule_idle(struct rq *rq, struct task_struct *p)
{
struct sched_class *class;
for_each_class(class) {
if (class-idle_enter)
class-idle_enter(rq);
}
}
or whatnot..
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On 10 April 2013 09:26, Peter Zijlstra pet...@infradead.org wrote:
On Tue, 2013-04-09 at 11:06 +0200, Vincent Guittot wrote:
+void idle_enter(struct rq *this_rq)
+{
+ update_rq_runnable_avg(this_rq, 1);
+}
+void idle_exit(struct rq *this_rq)
+{
+ update_rq_runnable_avg(this_rq, 0);
+}
These seem like fairly unfortunate names to expose to the global
namespace, why not expose update_rq_runnable_avg() instead?
Just to gather in one place all cfs actions that should be done when
exiting idle even if we only have update_rq_runnable_avg right now. I
have distinguished that from idle_balance because this sequence can't
generate extra context switch like idle_balance and they would finally
not be called in the same time
OK, but could we then please give then more scheduler specific names?
It just seems to me that idle_enter/idle_exit() are very obvious
function names for unrelated things.
How about calling it idle_{enter,exit}_fair; so that once other classes
grow hooks we can do something like:
My primary goal was to align with idle_balance name but
idle_{enter,exit}_fair is better
In the same way, should we change idle_balance to idle_balance_fair ?
and since we don't have Steve's irq constraint anymore, we could move
idle_balance in the beginning of the function pick_next_task_fair ? We
will not have spurious switch context and we will remove fair function
from __schedule function
Vincent
static void pre_schedule_idle(struct rq *rq, struct task_struct *p)
{
struct sched_class *class;
for_each_class(class) {
if (class-idle_enter)
class-idle_enter(rq);
}
}
or whatnot..
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On 9 April 2013 15:16, Steven Rostedt wrote: > On Tue, 2013-04-09 at 14:18 +0200, Vincent Guittot wrote: >> On 9 April 2013 10:55, Peter Zijlstra wrote: >> > On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote: >> >> Changes since V2: >> >> - remove useless definition for UP platform >> >> - rebased on top of Steven Rostedt's patches : >> >> https://lkml.org/lkml/2013/2/12/558 >> > >> > So what's the status of those patches? I still worry about the extra >> > context switch overhead for the high-frequency idle scenario. >> >> I don't know. I have seen a pulled answer from Ingo but can't find the >> commits in the tip tree. >> >> Steve, have you got more info about the status of your patches ? >> > > Yeah, I asked Ingo to revert it due to Peter's concerns. I was able to > get the latencies I needed without that patch set. That made it not so > urgent. > > Can you rebase your patches doing something similar? That is, still use > the pre/post_schedule_idle() calls, but don't base it off of my patch > set. Yes. I'm going to rebase my patches and add the declaration of post_schedule_idle in my patch instead of using your patch Thanks, Vincent > > Thanks, > > -- Steve > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On Tue, 2013-04-09 at 14:18 +0200, Vincent Guittot wrote: > On 9 April 2013 10:55, Peter Zijlstra wrote: > > On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote: > >> Changes since V2: > >> - remove useless definition for UP platform > >> - rebased on top of Steven Rostedt's patches : > >> https://lkml.org/lkml/2013/2/12/558 > > > > So what's the status of those patches? I still worry about the extra > > context switch overhead for the high-frequency idle scenario. > > I don't know. I have seen a pulled answer from Ingo but can't find the > commits in the tip tree. > > Steve, have you got more info about the status of your patches ? > Yeah, I asked Ingo to revert it due to Peter's concerns. I was able to get the latencies I needed without that patch set. That made it not so urgent. Can you rebase your patches doing something similar? That is, still use the pre/post_schedule_idle() calls, but don't base it off of my patch set. Thanks, -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On 9 April 2013 10:55, Peter Zijlstra wrote: > On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote: >> Changes since V2: >> - remove useless definition for UP platform >> - rebased on top of Steven Rostedt's patches : >> https://lkml.org/lkml/2013/2/12/558 > > So what's the status of those patches? I still worry about the extra > context switch overhead for the high-frequency idle scenario. I don't know. I have seen a pulled answer from Ingo but can't find the commits in the tip tree. Steve, have you got more info about the status of your patches ? Vincent > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On 9 April 2013 10:50, Peter Zijlstra wrote:
> On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote:
>> Signed-off-by: Vincent Guittot
>> ---
>> kernel/sched/fair.c | 23 +--
>> kernel/sched/idle_task.c | 10 ++
>> kernel/sched/sched.h | 12
>> 3 files changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 0fcdbff..1851ca8 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -1562,6 +1562,27 @@ static inline void
>> dequeue_entity_load_avg(struct cfs_rq *cfs_rq,
>> se->avg.decay_count =
>> atomic64_read(_rq->decay_counter);
>> } /* migrations, e.g. sleep=0 leave decay_count == 0 */
>> }
>> +
>> +/*
>> + * Update the rq's load with the elapsed running time before entering
>> + * idle. if the last scheduled task is not a CFS task, idle_enter
>> will
>> + * be the only way to update the runnable statistic.
>> + */
>> +void idle_enter(struct rq *this_rq)
>> +{
>> + update_rq_runnable_avg(this_rq, 1);
>> +}
>> +
>> +/*
>> + * Update the rq's load with the elapsed idle time before a task is
>> + * scheduled. if the newly scheduled task is not a CFS task,
>> idle_exit will
>> + * be the only way to update the runnable statistic.
>> + */
>> +void idle_exit(struct rq *this_rq)
>> +{
>> + update_rq_runnable_avg(this_rq, 0);
>> +}
>
> These seem like fairly unfortunate names to expose to the global
> namespace, why not expose update_rq_runnable_avg() instead?
Just to gather in one place all cfs actions that should be done when
exiting idle even if we only have update_rq_runnable_avg right now. I
have distinguished that from idle_balance because this sequence can't
generate extra context switch like idle_balance and they would finally
not be called in the same time
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote: > Changes since V2: > - remove useless definition for UP platform > - rebased on top of Steven Rostedt's patches : > https://lkml.org/lkml/2013/2/12/558 So what's the status of those patches? I still worry about the extra context switch overhead for the high-frequency idle scenario. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote:
> Signed-off-by: Vincent Guittot
> ---
> kernel/sched/fair.c | 23 +--
> kernel/sched/idle_task.c | 10 ++
> kernel/sched/sched.h | 12
> 3 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 0fcdbff..1851ca8 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -1562,6 +1562,27 @@ static inline void
> dequeue_entity_load_avg(struct cfs_rq *cfs_rq,
> se->avg.decay_count =
> atomic64_read(_rq->decay_counter);
> } /* migrations, e.g. sleep=0 leave decay_count == 0 */
> }
> +
> +/*
> + * Update the rq's load with the elapsed running time before entering
> + * idle. if the last scheduled task is not a CFS task, idle_enter
> will
> + * be the only way to update the runnable statistic.
> + */
> +void idle_enter(struct rq *this_rq)
> +{
> + update_rq_runnable_avg(this_rq, 1);
> +}
> +
> +/*
> + * Update the rq's load with the elapsed idle time before a task is
> + * scheduled. if the newly scheduled task is not a CFS task,
> idle_exit will
> + * be the only way to update the runnable statistic.
> + */
> +void idle_exit(struct rq *this_rq)
> +{
> + update_rq_runnable_avg(this_rq, 0);
> +}
These seem like fairly unfortunate names to expose to the global
namespace, why not expose update_rq_runnable_avg() instead?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote:
Signed-off-by: Vincent Guittot vincent.guit...@linaro.org
---
kernel/sched/fair.c | 23 +--
kernel/sched/idle_task.c | 10 ++
kernel/sched/sched.h | 12
3 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0fcdbff..1851ca8 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1562,6 +1562,27 @@ static inline void
dequeue_entity_load_avg(struct cfs_rq *cfs_rq,
se-avg.decay_count =
atomic64_read(cfs_rq-decay_counter);
} /* migrations, e.g. sleep=0 leave decay_count == 0 */
}
+
+/*
+ * Update the rq's load with the elapsed running time before entering
+ * idle. if the last scheduled task is not a CFS task, idle_enter
will
+ * be the only way to update the runnable statistic.
+ */
+void idle_enter(struct rq *this_rq)
+{
+ update_rq_runnable_avg(this_rq, 1);
+}
+
+/*
+ * Update the rq's load with the elapsed idle time before a task is
+ * scheduled. if the newly scheduled task is not a CFS task,
idle_exit will
+ * be the only way to update the runnable statistic.
+ */
+void idle_exit(struct rq *this_rq)
+{
+ update_rq_runnable_avg(this_rq, 0);
+}
These seem like fairly unfortunate names to expose to the global
namespace, why not expose update_rq_runnable_avg() instead?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote: Changes since V2: - remove useless definition for UP platform - rebased on top of Steven Rostedt's patches : https://lkml.org/lkml/2013/2/12/558 So what's the status of those patches? I still worry about the extra context switch overhead for the high-frequency idle scenario. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On 9 April 2013 10:50, Peter Zijlstra pet...@infradead.org wrote:
On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote:
Signed-off-by: Vincent Guittot vincent.guit...@linaro.org
---
kernel/sched/fair.c | 23 +--
kernel/sched/idle_task.c | 10 ++
kernel/sched/sched.h | 12
3 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0fcdbff..1851ca8 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1562,6 +1562,27 @@ static inline void
dequeue_entity_load_avg(struct cfs_rq *cfs_rq,
se-avg.decay_count =
atomic64_read(cfs_rq-decay_counter);
} /* migrations, e.g. sleep=0 leave decay_count == 0 */
}
+
+/*
+ * Update the rq's load with the elapsed running time before entering
+ * idle. if the last scheduled task is not a CFS task, idle_enter
will
+ * be the only way to update the runnable statistic.
+ */
+void idle_enter(struct rq *this_rq)
+{
+ update_rq_runnable_avg(this_rq, 1);
+}
+
+/*
+ * Update the rq's load with the elapsed idle time before a task is
+ * scheduled. if the newly scheduled task is not a CFS task,
idle_exit will
+ * be the only way to update the runnable statistic.
+ */
+void idle_exit(struct rq *this_rq)
+{
+ update_rq_runnable_avg(this_rq, 0);
+}
These seem like fairly unfortunate names to expose to the global
namespace, why not expose update_rq_runnable_avg() instead?
Just to gather in one place all cfs actions that should be done when
exiting idle even if we only have update_rq_runnable_avg right now. I
have distinguished that from idle_balance because this sequence can't
generate extra context switch like idle_balance and they would finally
not be called in the same time
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On 9 April 2013 10:55, Peter Zijlstra pet...@infradead.org wrote: On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote: Changes since V2: - remove useless definition for UP platform - rebased on top of Steven Rostedt's patches : https://lkml.org/lkml/2013/2/12/558 So what's the status of those patches? I still worry about the extra context switch overhead for the high-frequency idle scenario. I don't know. I have seen a pulled answer from Ingo but can't find the commits in the tip tree. Steve, have you got more info about the status of your patches ? Vincent -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On Tue, 2013-04-09 at 14:18 +0200, Vincent Guittot wrote: On 9 April 2013 10:55, Peter Zijlstra pet...@infradead.org wrote: On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote: Changes since V2: - remove useless definition for UP platform - rebased on top of Steven Rostedt's patches : https://lkml.org/lkml/2013/2/12/558 So what's the status of those patches? I still worry about the extra context switch overhead for the high-frequency idle scenario. I don't know. I have seen a pulled answer from Ingo but can't find the commits in the tip tree. Steve, have you got more info about the status of your patches ? Yeah, I asked Ingo to revert it due to Peter's concerns. I was able to get the latencies I needed without that patch set. That made it not so urgent. Can you rebase your patches doing something similar? That is, still use the pre/post_schedule_idle() calls, but don't base it off of my patch set. Thanks, -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 v4] sched: fix wrong rq's runnable_avg update with rt tasks
On 9 April 2013 15:16, Steven Rostedt rost...@goodmis.org wrote: On Tue, 2013-04-09 at 14:18 +0200, Vincent Guittot wrote: On 9 April 2013 10:55, Peter Zijlstra pet...@infradead.org wrote: On Thu, 2013-04-04 at 16:15 +0200, Vincent Guittot wrote: Changes since V2: - remove useless definition for UP platform - rebased on top of Steven Rostedt's patches : https://lkml.org/lkml/2013/2/12/558 So what's the status of those patches? I still worry about the extra context switch overhead for the high-frequency idle scenario. I don't know. I have seen a pulled answer from Ingo but can't find the commits in the tip tree. Steve, have you got more info about the status of your patches ? Yeah, I asked Ingo to revert it due to Peter's concerns. I was able to get the latencies I needed without that patch set. That made it not so urgent. Can you rebase your patches doing something similar? That is, still use the pre/post_schedule_idle() calls, but don't base it off of my patch set. Yes. I'm going to rebase my patches and add the declaration of post_schedule_idle in my patch instead of using your patch Thanks, Vincent Thanks, -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
