| Message ID | 1380709565-22153-1-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 10/02/2013 12:26 PM, Daniel Lezcano wrote: > The sleep_length is computed in the tick_nohz_stop_sched_tick function but it > is used later in the code with in between the local irq enabled. > > cpu_idle_loop > tick_nohz_idle_enter [ exits with local irq enabled ] > __tick_nohz_idle_enter > tick_nohz_stop_sched_tick > ... > > arch_cpu_idle > menu_select [ uses here 'sleep_length' ] > ... > > Between the computation of the sleep length and its usage, some interrupts > may occur, making the sleep length shorter than actually it is because of the > interrupt processing, or different if the timer itself expired. > > This patch fixes that by moving the sleep_length computation in the > tick_nohz_get_sleep_length function and using the tick device's next_event. > > As the sleep_length field is no longer needed, it is removed from the > tick_sched structure. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> Stephen, I added your signed-off as the change in tick_nohz_get_sleep_length is the one you proposed previously. -- Daniel > --- > include/linux/tick.h | 2 -- > kernel/time/tick-sched.c | 5 +++-- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/include/linux/tick.h b/include/linux/tick.h > index 5128d33..53dbbd7 100644 > --- a/include/linux/tick.h > +++ b/include/linux/tick.h > @@ -48,7 +48,6 @@ enum tick_nohz_mode { > * @idle_exittime: Time when the idle state was left > * @idle_sleeptime: Sum of the time slept in idle with sched tick stopped > * @iowait_sleeptime: Sum of the time slept in idle with sched tick stopped, with IO outstanding > - * @sleep_length: Duration of the current idle sleep > * @do_timer_lst: CPU was the last one doing do_timer before going idle > */ > struct tick_sched { > @@ -67,7 +66,6 @@ struct tick_sched { > ktime_t idle_exittime; > ktime_t idle_sleeptime; > ktime_t iowait_sleeptime; > - ktime_t sleep_length; > unsigned long last_jiffies; > unsigned long next_jiffies; > ktime_t idle_expires; > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c > index 3612fc7..60b1dcd 100644 > --- a/kernel/time/tick-sched.c > +++ b/kernel/time/tick-sched.c > @@ -673,7 +673,6 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, > out: > ts->next_jiffies = next_jiffies; > ts->last_jiffies = last_jiffies; > - ts->sleep_length = ktime_sub(dev->next_event, now); > > return ret; > } > @@ -837,8 +836,10 @@ void tick_nohz_irq_exit(void) > ktime_t tick_nohz_get_sleep_length(void) > { > struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched); > + struct clock_event_device *dev = __get_cpu_var(tick_cpu_device).evtdev; > + ktime_t now = ktime_get(); > > - return ts->sleep_length; > + return ktime_sub(dev->next_event, now); > } > > static void tick_nohz_restart(struct tick_sched *ts, ktime_t now) >
2013/10/2 Daniel Lezcano <daniel.lezcano@linaro.org>: > The sleep_length is computed in the tick_nohz_stop_sched_tick function but it > is used later in the code with in between the local irq enabled. > > cpu_idle_loop > tick_nohz_idle_enter [ exits with local irq enabled ] > __tick_nohz_idle_enter > tick_nohz_stop_sched_tick > ... > > arch_cpu_idle > menu_select [ uses here 'sleep_length' ] > ... > > Between the computation of the sleep length and its usage, some interrupts > may occur, making the sleep length shorter than actually it is because of the > interrupt processing So, do you mean that the ts->sleep_length would return a value that is too long given that the CPU already spent some time to service the irqs since we computed the sleep length in tick_nohz_idle_enter()? But then tick_nohz_irq_exit() should take care of that as it calls again tick_nohz_stop_sched_tick(). So I'm a bit confused. > or different if the timer itself expired. Same here, if the timer expired, it triggers an interrupt which can do two things: 1) reprogram a new timer and this recompute sleep_length 2) set_need_resched() and then exit the idle loop, so arch_cpu_idle() won't even be called. Or the timer interrupts hlt, but then menu_select() was called before. So I probably missed something here. Thanks.
On 10/02/2013 05:57 PM, Frederic Weisbecker wrote: > 2013/10/2 Daniel Lezcano <daniel.lezcano@linaro.org>: >> The sleep_length is computed in the tick_nohz_stop_sched_tick function but it >> is used later in the code with in between the local irq enabled. >> >> cpu_idle_loop >> tick_nohz_idle_enter [ exits with local irq enabled ] >> __tick_nohz_idle_enter >> tick_nohz_stop_sched_tick >> ... >> >> arch_cpu_idle >> menu_select [ uses here 'sleep_length' ] >> ... >> >> Between the computation of the sleep length and its usage, some interrupts >> may occur, making the sleep length shorter than actually it is because of the >> interrupt processing > > So, do you mean that the ts->sleep_length would return a value that is too long > given that the CPU already spent some time to service the irqs since we computed > the sleep length in tick_nohz_idle_enter()? > > But then tick_nohz_irq_exit() should take care of that as it calls > again tick_nohz_stop_sched_tick(). > So I'm a bit confused. > >> or different if the timer itself expired. > > Same here, if the timer expired, it triggers an interrupt which can do > two things: > > 1) reprogram a new timer and this recompute sleep_length > 2) set_need_resched() and then exit the idle loop, so arch_cpu_idle() won't even > be called. Or the timer interrupts hlt, but then menu_select() was > called before. > > So I probably missed something here. No you did not :) Indeed... At the first glance, this issue sounded so obvious I suspected there must be a trick somewhere but I did not think to look at the irq_exit, the code is very complex. Thanks for clarifying this. For my personal information, is there any particular reason to set an intermediate 'sleep_length' in tick_nohz_stop_sched_tick instead of doing what does this patch ? Thanks -- Daniel
On Wed, Oct 02, 2013 at 06:22:29PM +0200, Daniel Lezcano wrote: > On 10/02/2013 05:57 PM, Frederic Weisbecker wrote: > >2013/10/2 Daniel Lezcano <daniel.lezcano@linaro.org>: > >>The sleep_length is computed in the tick_nohz_stop_sched_tick function but it > >>is used later in the code with in between the local irq enabled. > >> > >>cpu_idle_loop > >> tick_nohz_idle_enter [ exits with local irq enabled ] > >> __tick_nohz_idle_enter > >> tick_nohz_stop_sched_tick > >> ... > >> > >> arch_cpu_idle > >> menu_select [ uses here 'sleep_length' ] > >> ... > >> > >>Between the computation of the sleep length and its usage, some interrupts > >>may occur, making the sleep length shorter than actually it is because of the > >>interrupt processing > > > >So, do you mean that the ts->sleep_length would return a value that is too long > >given that the CPU already spent some time to service the irqs since we computed > >the sleep length in tick_nohz_idle_enter()? > > > >But then tick_nohz_irq_exit() should take care of that as it calls > >again tick_nohz_stop_sched_tick(). > >So I'm a bit confused. > > > >>or different if the timer itself expired. > > > >Same here, if the timer expired, it triggers an interrupt which can do > >two things: > > > >1) reprogram a new timer and this recompute sleep_length > >2) set_need_resched() and then exit the idle loop, so arch_cpu_idle() won't even > >be called. Or the timer interrupts hlt, but then menu_select() was > >called before. > > > >So I probably missed something here. > > No you did not :) > > Indeed... At the first glance, this issue sounded so obvious I > suspected there must be a trick somewhere but I did not think to > look at the irq_exit, the code is very complex. Thanks for > clarifying this. > > For my personal information, is there any particular reason to set > an intermediate 'sleep_length' in tick_nohz_stop_sched_tick instead > of doing what does this patch ? May be we could do it that way yeah. Is menu_select() called only there? I don't know how much difference that would make. > > Thanks > -- Daniel > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog >
On 10/02/2013 06:42 PM, Frederic Weisbecker wrote: > On Wed, Oct 02, 2013 at 06:22:29PM +0200, Daniel Lezcano wrote: >> On 10/02/2013 05:57 PM, Frederic Weisbecker wrote: >>> 2013/10/2 Daniel Lezcano <daniel.lezcano@linaro.org>: >>>> The sleep_length is computed in the tick_nohz_stop_sched_tick function but it >>>> is used later in the code with in between the local irq enabled. >>>> >>>> cpu_idle_loop >>>> tick_nohz_idle_enter [ exits with local irq enabled ] >>>> __tick_nohz_idle_enter >>>> tick_nohz_stop_sched_tick >>>> ... >>>> >>>> arch_cpu_idle >>>> menu_select [ uses here 'sleep_length' ] >>>> ... >>>> >>>> Between the computation of the sleep length and its usage, some interrupts >>>> may occur, making the sleep length shorter than actually it is because of the >>>> interrupt processing >>> >>> So, do you mean that the ts->sleep_length would return a value that is too long >>> given that the CPU already spent some time to service the irqs since we computed >>> the sleep length in tick_nohz_idle_enter()? >>> >>> But then tick_nohz_irq_exit() should take care of that as it calls >>> again tick_nohz_stop_sched_tick(). >>> So I'm a bit confused. >>> >>>> or different if the timer itself expired. >>> >>> Same here, if the timer expired, it triggers an interrupt which can do >>> two things: >>> >>> 1) reprogram a new timer and this recompute sleep_length >>> 2) set_need_resched() and then exit the idle loop, so arch_cpu_idle() won't even >>> be called. Or the timer interrupts hlt, but then menu_select() was >>> called before. >>> >>> So I probably missed something here. >> >> No you did not :) >> >> Indeed... At the first glance, this issue sounded so obvious I >> suspected there must be a trick somewhere but I did not think to >> look at the irq_exit, the code is very complex. Thanks for >> clarifying this. >> >> For my personal information, is there any particular reason to set >> an intermediate 'sleep_length' in tick_nohz_stop_sched_tick instead >> of doing what does this patch ? > > May be we could do it that way yeah. Is menu_select() called only there? > I don't know how much difference that would make. Yes, it is called just one time in all the code. The benefit would be just to cleanup a field in the struct tick_sched.
On Wed, Oct 02, 2013 at 08:03:39PM +0200, Daniel Lezcano wrote: > On 10/02/2013 06:42 PM, Frederic Weisbecker wrote: > >On Wed, Oct 02, 2013 at 06:22:29PM +0200, Daniel Lezcano wrote: > >>On 10/02/2013 05:57 PM, Frederic Weisbecker wrote: > >>>2013/10/2 Daniel Lezcano <daniel.lezcano@linaro.org>: > >>>>The sleep_length is computed in the tick_nohz_stop_sched_tick function but it > >>>>is used later in the code with in between the local irq enabled. > >>>> > >>>>cpu_idle_loop > >>>> tick_nohz_idle_enter [ exits with local irq enabled ] > >>>> __tick_nohz_idle_enter > >>>> tick_nohz_stop_sched_tick > >>>> ... > >>>> > >>>> arch_cpu_idle > >>>> menu_select [ uses here 'sleep_length' ] > >>>> ... > >>>> > >>>>Between the computation of the sleep length and its usage, some interrupts > >>>>may occur, making the sleep length shorter than actually it is because of the > >>>>interrupt processing > >>> > >>>So, do you mean that the ts->sleep_length would return a value that is too long > >>>given that the CPU already spent some time to service the irqs since we computed > >>>the sleep length in tick_nohz_idle_enter()? > >>> > >>>But then tick_nohz_irq_exit() should take care of that as it calls > >>>again tick_nohz_stop_sched_tick(). > >>>So I'm a bit confused. > >>> > >>>>or different if the timer itself expired. > >>> > >>>Same here, if the timer expired, it triggers an interrupt which can do > >>>two things: > >>> > >>>1) reprogram a new timer and this recompute sleep_length > >>>2) set_need_resched() and then exit the idle loop, so arch_cpu_idle() won't even > >>>be called. Or the timer interrupts hlt, but then menu_select() was > >>>called before. > >>> > >>>So I probably missed something here. > >> > >>No you did not :) > >> > >>Indeed... At the first glance, this issue sounded so obvious I > >>suspected there must be a trick somewhere but I did not think to > >>look at the irq_exit, the code is very complex. Thanks for > >>clarifying this. > >> > >>For my personal information, is there any particular reason to set > >>an intermediate 'sleep_length' in tick_nohz_stop_sched_tick instead > >>of doing what does this patch ? > > > >May be we could do it that way yeah. Is menu_select() called only there? > >I don't know how much difference that would make. > > Yes, it is called just one time in all the code. The benefit would > be just to cleanup a field in the struct tick_sched. Yeah, why not. Thanks. > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog >
diff --git a/include/linux/tick.h b/include/linux/tick.h index 5128d33..53dbbd7 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -48,7 +48,6 @@ enum tick_nohz_mode { * @idle_exittime: Time when the idle state was left * @idle_sleeptime: Sum of the time slept in idle with sched tick stopped * @iowait_sleeptime: Sum of the time slept in idle with sched tick stopped, with IO outstanding - * @sleep_length: Duration of the current idle sleep * @do_timer_lst: CPU was the last one doing do_timer before going idle */ struct tick_sched { @@ -67,7 +66,6 @@ struct tick_sched { ktime_t idle_exittime; ktime_t idle_sleeptime; ktime_t iowait_sleeptime; - ktime_t sleep_length; unsigned long last_jiffies; unsigned long next_jiffies; ktime_t idle_expires; diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 3612fc7..60b1dcd 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -673,7 +673,6 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, out: ts->next_jiffies = next_jiffies; ts->last_jiffies = last_jiffies; - ts->sleep_length = ktime_sub(dev->next_event, now); return ret; } @@ -837,8 +836,10 @@ void tick_nohz_irq_exit(void) ktime_t tick_nohz_get_sleep_length(void) { struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched); + struct clock_event_device *dev = __get_cpu_var(tick_cpu_device).evtdev; + ktime_t now = ktime_get(); - return ts->sleep_length; + return ktime_sub(dev->next_event, now); } static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
The sleep_length is computed in the tick_nohz_stop_sched_tick function but it is used later in the code with in between the local irq enabled. cpu_idle_loop tick_nohz_idle_enter [ exits with local irq enabled ] __tick_nohz_idle_enter tick_nohz_stop_sched_tick ... arch_cpu_idle menu_select [ uses here 'sleep_length' ] ... Between the computation of the sleep length and its usage, some interrupts may occur, making the sleep length shorter than actually it is because of the interrupt processing, or different if the timer itself expired. This patch fixes that by moving the sleep_length computation in the tick_nohz_get_sleep_length function and using the tick device's next_event. As the sleep_length field is no longer needed, it is removed from the tick_sched structure. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- include/linux/tick.h | 2 -- kernel/time/tick-sched.c | 5 +++-- 2 files changed, 3 insertions(+), 4 deletions(-)