| Message ID | 1349595838-31274-5-git-send-email-vincent.guittot@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
$subject is bit confusing here. On Sunday 07 October 2012 01:13 PM, Vincent Guittot wrote: > The atomic update of runnable_avg_sum and runnable_avg_period are ensured > by their size and the toolchain. But we must ensure to not read an old value > for one field and a newly updated value for the other field. As we don't > want to lock other CPU while reading these fields, we read twice each fields > and check that no change have occured in the middle. > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > --- > kernel/sched/fair.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 8c9d3ed..6df53b5 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -3133,13 +3133,28 @@ static int select_idle_sibling(struct task_struct *p, int target) > static inline bool is_buddy_busy(int cpu) > { > struct rq *rq = cpu_rq(cpu); > + volatile u32 *psum = &rq->avg.runnable_avg_sum; > + volatile u32 *pperiod = &rq->avg.runnable_avg_period; > + u32 sum, new_sum, period, new_period; > + int timeout = 10; So it can be 2 times read or more as well. > + > + while (timeout) { > + sum = *psum; > + period = *pperiod; > + new_sum = *psum; > + new_period = *pperiod; > + > + if ((sum == new_sum) && (period == new_period)) > + break; > + > + timeout--; > + } > Seems like you did notice incorrect pair getting read for rq runnable_avg_sum and runnable_avg_period. Seems like the fix is to update them together under some lock to avoid such issues. Regards Santosh
On 24 October 2012 17:21, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > $subject is bit confusing here. > > > On Sunday 07 October 2012 01:13 PM, Vincent Guittot wrote: >> >> The atomic update of runnable_avg_sum and runnable_avg_period are ensured >> by their size and the toolchain. But we must ensure to not read an old >> value >> for one field and a newly updated value for the other field. As we don't >> want to lock other CPU while reading these fields, we read twice each >> fields >> and check that no change have occured in the middle. >> >> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> >> --- >> kernel/sched/fair.c | 19 +++++++++++++++++-- >> 1 file changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index 8c9d3ed..6df53b5 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -3133,13 +3133,28 @@ static int select_idle_sibling(struct task_struct >> *p, int target) >> static inline bool is_buddy_busy(int cpu) >> { >> struct rq *rq = cpu_rq(cpu); >> + volatile u32 *psum = &rq->avg.runnable_avg_sum; >> + volatile u32 *pperiod = &rq->avg.runnable_avg_period; >> + u32 sum, new_sum, period, new_period; >> + int timeout = 10; > > So it can be 2 times read or more as well. > >> + >> + while (timeout) { >> + sum = *psum; >> + period = *pperiod; >> + new_sum = *psum; >> + new_period = *pperiod; >> + >> + if ((sum == new_sum) && (period == new_period)) >> + break; >> + >> + timeout--; >> + } >> > Seems like you did notice incorrect pair getting read > for rq runnable_avg_sum and runnable_avg_period. Seems > like the fix is to update them together under some lock > to avoid such issues. My goal is to have a lock free mechanism because I don't want to lock another CPU while reading its statistic > > Regards > Santosh >
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 8c9d3ed..6df53b5 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -3133,13 +3133,28 @@ static int select_idle_sibling(struct task_struct *p, int target) static inline bool is_buddy_busy(int cpu) { struct rq *rq = cpu_rq(cpu); + volatile u32 *psum = &rq->avg.runnable_avg_sum; + volatile u32 *pperiod = &rq->avg.runnable_avg_period; + u32 sum, new_sum, period, new_period; + int timeout = 10; + + while (timeout) { + sum = *psum; + period = *pperiod; + new_sum = *psum; + new_period = *pperiod; + + if ((sum == new_sum) && (period == new_period)) + break; + + timeout--; + } /* * A busy buddy is a CPU with a high load or a small load with a lot of * running tasks. */ - return ((rq->avg.usage_avg_sum << rq->nr_running) > - rq->avg.runnable_avg_period); + return ((new_sum << rq->nr_running) > new_period); } static inline bool is_light_task(struct task_struct *p)
The atomic update of runnable_avg_sum and runnable_avg_period are ensured by their size and the toolchain. But we must ensure to not read an old value for one field and a newly updated value for the other field. As we don't want to lock other CPU while reading these fields, we read twice each fields and check that no change have occured in the middle. Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> --- kernel/sched/fair.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)