On Thu, 18 Oct 2007, Benjamin Herrenschmidt wrote:
>
> + smp_mb();
> while (desc->status & IRQ_INPROGRESS)
> cpu_relax();
So, what exactly does it protect against? At a minimum, this needs a
comment in the changelog, and probably preferably in the source code too.
The thing is, synchronize_irq() can only protect against interrupts that
are *already*running* on another CPU, and the caller must have made sure
that no new interrupts are coming in (or at least that whatever new
interrupts that come in will not pick up a certain piece of data).
So I can imagine that the smb_mb() is there so that the caller - who has
cleared some list or done something like that - will have any preceding
writes that it did be serialized with actually checking the old state of
"desc->status".
Fair enough - I can see that a smp_mb() is useful. But I think it needs
documenting as such, and preferably with an example of how this actually
happened in the first place (do you have one?)
The synchronize_irq() users that are really fundamental (ie the irq
datastructures themselves) all should use the irq descriptor spinlock, and
should *not* be needing any of this, since they would have serialized with
whoever actually set the IRQ_INPROGRESS bit in the first place.
So in *that* sense, I think the memory barrier is useless, and I can't
make up my mind whether it's good or bad. Which is why I'd really like to
have an example scenario spelled out where it makes a difference..
Ok?
Linus
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
