lkml.org 
[lkml]   [2008]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: MMIO and gcc re-ordering issue
    Date
    On Wednesday 11 June 2008 15:13, Paul Mackerras wrote:
    > Nick Piggin writes:
    > > > 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.


    > > 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.


    \
     
     \ /
      Last update: 2008-06-11 07:39    [W:4.164 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site