Cc: Benjamin Herrenschmidt <benh@...>, <akpm@...>, Linux Kernel Mailing List <linux-kernel@...>, <linuxppc-dev@...>, Ingo Molnar <mingo@...>, Thomas Gleixner <tglx@...>
Hey, I appreciate it, but I do think that the "spinlock only to
immediately unlock it again" is pretty horrid.
I'm convinced that there should be some way to do this without actually
taking the lock. I *think* it should work with something like
for (;;) {
smp_rmb();
if (!spin_is_locked(&desc->lock)) {
smp_rmb();
if (!(desc->status & IRQ_INPROGRESS)
break;
}
cpu_relax();
}
instead. Which basically requires that we see the descriptor lock being
not held, before we see the IRQ_INPROGRESS bit being clear. Put another
way: it loops until it sees the lock having been released, and the
IRQ_INPROGRESS bit being clear after that.
The above requires no serializing instructions on x86, which is a good
goal (now that smp_rmb() is just a compiler barrier). And it doesn't
actually have to bounce any cachelines.
And it doesn't have that ugly "get lock only to release it", which just
makes me go "Eww!".
But it's a bit subtler. It basically depends on the fact that
spin_unlock() obviously has to make sure that there is a release barrier
in the unlock, so any writes done (to the IRQ_INPROGRESS bit) within the
locked region *must* be visible before the spinlock itself has been
released.
So somebody should:
- use another pair of eyes and brains to back me up on this.
- write up some coherent changelog entry, using the emails that have
passed back and forth.
- actually turn the above into a tested patch with a comment.
And I'm pushing for that "somebody" being somebody else than me ;)
Linus
-
