Messages in this thread | | | Date | Thu, 18 Nov 2004 11:05:33 -0800 (PST) | From | Linus Torvalds <> | Subject | Re: [patch, 2.6.10-rc2] fix __flush_tlb*() preemption bug on CONFIG_PREEMPT |
| |
On Thu, 18 Nov 2004, Ingo Molnar wrote: > > yeah, if we have such a debugging check then it should be pretty safe to > find the places one by one and fix them. The patch below (against -rc2) > adds the debugging check and fixes ioremap, on x86. I've done a quick > testboot and it doesnt trigger any other warnings.
What about moving the "preempt_disable/enable" into the special flush cases (ie "global_flush_tlb()" and "flush_tlb_all()"). I don't think it makes sense in the code that uses them.
Then we'd just make it a rule that TLB flushing _has_ to be done either from a valid VM context (that follows the flush) or non-preemptible. Otherwise it's kind of undefined what happens to the current context, not just from an implementation angle, but from a _conceptual_ standpoint.
In contrast, the "global" flushes obviously make sense from any context (there is no "conceptual" confusion there and the confusion is purely an implementation bug), which is why I think they should do the preemption disable themselves.
I really want core kernel code to make sense from a conceptual angle. Not just "it works", but "it works because it makes sense".
> (maybe we want to change that check to WARN_ON(!preempt_count()), while > non-lazy-TLB tasks are fine right now, it's quite fragile i think to > allow a TLB flush to preempt and it's a ticking timebomb i think.)
Hmm.. I don't see the problem. Maybe we should have other rules (like "you have to hold the page table lock") that would make that true, but I'd prefer the warning to have some "raison d'etre", if you see what I mean?
Another way of saying the same thing: if we cannot put a comment saying "this is why we check _this_ condition", then we shouldn't check that condition.
But yes, it may well make perfect sense to say "we have to hold the page table spinlock in order to flush the tlb". Is that actually true right now?
Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |