Messages in this thread | | | Date | Wed, 5 May 1999 17:09:57 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: [patch] SMP race fix [was Re: SMP lockup & 3c509 on 2.2.x [aka. the Deadly 'ping -f']] |
| |
On Thu, 6 May 1999, Andrea Arcangeli wrote: > > But in general it make not a lot of sense to disable an irq from the irq > itself, because if you are the irq your are just assured by design that > you can't reenter in yourself.
Yes. But the problem then becomes one of common code. It may make sense to have code that wants to atomically make sure that it's running on only the current CPU and that is called from different contexts (both irq and not), and it just does a
disable_irq(myirq); .. do some slow stuff .. enable_irq(myirq);
and the dangerous thing is that it would work on UP, but mysteriously (and nonobviously) lock up on SMP with your implementation.
I agree that the above is not necessarily _clever_ nor necessarily good programming practice. But I think it's even worse programming practice if we made it lock up with no messages for no really obvious reason, because it _is_ at least straightforward and obvious code and as such we can't really claim that it is outright evil.
So I fully agree with your point that there is a very subtle race in the disable_irq() path. It's just that I want to think more about the solution, and I'd ask you to think about it some more too.
For example, I don't think that changing meaning of the INPROGRESS bit the way you did is the right approach. But the right approach _may_ be to split the thing up into two bits: currently we kind of overload two meanings on INPROGRESS ("BUSY" as in 'we're not taking it any more because it didn't have a handler or it was disabled when it happened' and a separate "RUNNING" as in 'we're right now in the process of handling that particular IRQ').
That kind of split would make sense, and would allow the autoirq behaviour, as well as noticing spurious interrupts with no handler (which was a invaluable debugging tool for me where the current INPROGRESS semantics made a lot of sense).
Alternatively, maybe there are other approaches - and whatever we do I do believe that we need to handle the nesting case gracefully (your "nosync" doesn't really help - because it isn't useful as a above kind of exclusivity guarantee exactly because it doesn't wait, see?)
Linus
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |