Re: [patch, 2.6.10-rc2] fix __flush_tlb*() preemption bug onCONFIG_PREEMPT

[Linus Torvalds]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/