Messages in this thread | | | Date | Fri, 24 Apr 2015 11:10:31 -0400 | From | Rik van Riel <> | Subject | Re: [PATCH] context_tracking: remove local_irq_save from __acct_update_integrals |
| |
On 04/24/2015 05:11 AM, Heiko Carstens wrote: > On Thu, Apr 23, 2015 at 09:57:13PM -0400, Rik van Riel wrote: >> diff --git a/kernel/tsacct.c b/kernel/tsacct.c >> index 975cb49e32bf..0b967f116a6b 100644 >> --- a/kernel/tsacct.c >> +++ b/kernel/tsacct.c >> @@ -126,23 +126,29 @@ static void __acct_update_integrals(struct task_struct *tsk, >> if (likely(tsk->mm)) { >> cputime_t time, dtime; >> struct timeval value; >> - unsigned long flags; >> u64 delta; >> >> - local_irq_save(flags); >> time = stime + utime; >> dtime = time - tsk->acct_timexpd; >> + /* >> + * This code is called both from irq context and from >> + * task context. There is a race where irq context advances >> + * tsk->acct_timexpd to a value larger than time, creating >> + * a negative value. In that case, the irq has already >> + * updated the statistics. >> + */ >> + if (unlikely((signed long)dtime <= 0)) >> + return; > > FWIW, I think you either need a barrier() before the if-statement or use > READ_ONCE() when reading tsk->acct_timexpd above. > > Otherwise the compiler could (in theory at least) generate code which > would translate to > if (unlikely(time <= tsk->acct_timexpd)) > in order to achieve the same result, no? > > Besides that cputime_t might be 64 bit in size, therefore you don't have > much of a guarentee that reading tsk->acct_timexpd happens atomically on > 32 bit architectures, so you _may_ end up with garbage, no?
You are right on both counts. Thank you for pointing out what should have been obvious...
Let me post a new patch :)
-- All rights reversed.
| |