Messages in this thread | | | From | Linus Torvalds <> | Date | Sun, 16 Oct 2011 18:39:57 -0700 | Subject | Re: Linux 3.1-rc9 |
| |
On Thu, Oct 13, 2011 at 4:25 PM, Simon Kirby <sim@hostway.ca> wrote: > > Looks good. No hangs or crashes for two days on any of them running > 3.1-rc9 plus this patch. Not sure if you want to deuglify it, but it > seems to work... > > Tested-by: Simon Kirby <sim@hostway.ca>
Peter, what's the status of this one?
Quite frankly, I personally consider it to be broken - why are we introducing this new lock for this very special thing? A spinlock to protect a *single* word of counter seems broken.
It seems more likely that the real bug is that kernel/sched_stats.h currently takes cputimer->lock without disabling interrupts. Everybody else uses irq-safe locking, why would sched_stats.h not need that?
However, I don't see why that spinlock is needed at all. Why aren't those fields just atomics (or at least just "sum_exec_runtime")? And why does "cputime_add()" exist at all? It seems to always be just a plain add, and nothing else would seem to ever make sense *anyway*?
In other words, none of that code makes any sense to me at all. And the patch in question that fixes a hang for Simon seems to make it even worse. Can somebody explain to me why it looks that crappy?
Please?
That stupid definition of cputime_add() has apparently existed as-is since it was introduced in 2005. Why do we have code like this:
times->utime = cputime_add(times->utime, t->utime);
instead of just
times->utime += t->utime;
which seems not just shorter, but more readable too? The reason is not some type safety in the cputime_add() thing, it's just a macro.
Added Martin and Ingo to the discussion - Martin because he added that cputime_add in the first place, and Ingo because he gets the most hits on kernel/sched_stats.h. Guys - you can see the history on lkml.
Linus
| |