Messages in this thread | | | Date | Wed, 5 May 1999 17:18:01 -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: > > I thought the current semantic of synchronize_irq() is that > synchronize_irq() simply flush the _running_ _irqhandler_ (not the > raw-irq) and so is _far_ from making sure that there won't be a new irq > after the call.
A pure synchronize_irq() yes.
But you missed the larger picture, which was that synchronize_irq() was used at the end of "disable_irq()". And in that case, we know that there can't be a new irq after the call, because we just disabled it (and we don't depend on any magic hardware that we don't control directly for disabling: the disabled state shows up in the irq descriptor data structures, so we are _guaranteed_ that a new one cannot come in once we have set those correctly).
So I again agree that there seems to be a race, I just disagree with the approaches taken for that race, and especially patch#2 which I just think is a no-op for the above reasons.
> I think it's _only_ disable_irq() that must make sure that no other irqX > will run after the call. synchronize_irq() has not such semantic (at least > according to me) and I don't consider it buggy (so it's not obvious that > we want to make it bloated -> irqinprogress/irqdisabled aware).
Indeed. But the only real valid use for "synchronize_irq()" is some code that looks something like this:
outw(DISABLE_IRQ, hardwareregister); synchronize_irq();
where the point of synchronize_irq() is to make sure that we wait for any pending irq that might be running now (or that might have _just_ been raised before the outw).
However, the above use of synchronize_irq() may not actually be physically possible (for the simple reason that the device and the CPU really aren't synchronized ina very real sense, and there is nothing we can do at a low interrupt controller layer to make them so), so I'm certainly willing to entertain the notion of just making it a non-supported option.
> This also > considering that cli() instead is perfectly safe for the underlined line: > > static inline void irq_enter(int cpu, unsigned int irq) > { > hardirq_enter(cpu); > while (test_bit(0,&global_irq_lock)) { > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > /* nothing */; > } > } > > But if you then issue a sti() (as synchronize_irq() does) the pending irq > will unblock (and it will be allowed to race in the disable_irq case) > then.
Oh, agreed - but synchronize_irq() doesn't make sense on its own: it only makes sense in conjunction with "disable_irq()" or a device-specific disable operation.
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/
| |