On Fri, 19 Oct 2007, Herbert Xu wrote: > > In other words I think this patch is great :)
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/