On Wednesday 11 June 2008 15:13, Paul Mackerras wrote:

Nick Piggin writes:

quoted

quoted

I just wish we had even one actual example of things going wrong with
the current rules we have on powerpc to motivate changing to this
model.

~/usr/src/linux-2.6> git grep test_and_set_bit drivers/ | wc -l
506
How sure are you that none of those forms part of a cobbled-together
locking scheme that hopes to constrain some IO access?

My comment was precisely about the fact that this sort of argument is
actually FUD.  I want one example that is demonstrably wrong, not just
a "how sure are you".

But this is a case where you really should be scared, so FUD is
completely appropriate.

At least, I don't see how it is at all worse than FUD about how much
slower everything will be with stronger ordering, and how nobody will
be able to do anything about it. In reality, for most devices it will
not be measurable, and for some like network or disk might use a bit
of a tweak in one or two fastpaths.

If the driver author honestly does not know the code well enough to
say whether a particular ordering is allowed or not, then it should
just not be allowed.


Anyway, what do I win if I give you a concrete example?

I found one in the first file I picked out of my grep: drivers/net/s2io.c

        if (test_and_set_bit(__S2IO_STATE_LINK_TASK, &(nic->state))) {
                /* The card is being reset, no point doing anything */
                goto out_unlock;
        }

...
                val64 = readq(&bar0->adapter_control);
                val64 |= ADAPTER_LED_ON;
                writeq(val64, &bar0->adapter_control);
                s2io_link(nic, LINK_UP);
        } else {
                if (CARDS_WITH_FAULTY_LINK_INDICATORS(nic->device_type,
                                                      subid)) {
                        val64 = readq(&bar0->gpio_control);
                        val64 &= ~GPIO_CTRL_GPIO_0;
                        writeq(val64, &bar0->gpio_control);
                        val64 = readq(&bar0->gpio_control);
                }
                /* turn off LED */
                val64 = readq(&bar0->adapter_control);
                val64 = val64 &(~ADAPTER_LED_ON);
                writeq(val64, &bar0->adapter_control);
                s2io_link(nic, LINK_DOWN);
        }
        clear_bit(__S2IO_STATE_LINK_TASK, &(nic->state));

Now I can't say definitively that this is going to be wrong on
powerpc, because I don't know the code well enough. But I'd be
90% sure that the unlock is not supposed to be visible to
other CPUs before the writeqs are queued to the card. On x86 it
wouldn't be.

quoted

But surely you have to audit the drivers anyway to ensure they are OK
with the current powerpc scheme. In which case, once you have audited
them and know they are safe, you can easily convert them to the even
_faster_ __readl/__writel, and just add the appropriate barriers.

The trouble is that as code gets maintained, using __writel + explicit
barriers is more fragile because some people will change the code, or
add new code, without understanding the barriers.

If the relaxed writel and explicit barriers are not well documented and
well understood, then they should not be used.

And if the memory ordering issues are not well understood, then it
should be done with locking and strongly ordered IO accessors.

So whenever a 
driver gets converted to using __writel + barriers, we will end up
having to watch every change that goes into it forever.  Whereas with

*You* don't have to watch it, the maintainer does. And if they can't
understand the barriers, then it should not be converted to use them.
Unless you want to take over maintainership. And if you do, then you
should be watching all changes that go into it.

the current scheme there's a much smaller set of gotchas to watch out
for, and the gotchas are things that already raise red flags, such as
open-coded locking and any sort of "clever" lockless scheme.

And with the strongly ordered scheme, there is much smaller set of
gotchas again, and it behaves the same as the x86 it was developed on,
and you don't get confused by the list of rules: IO access is either
strongly ordered or weakly ordered.