lkml.org 
[lkml]   [2020]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v9 08/20] gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL
    On Fri, Sep 25, 2020 at 2:56 PM Kent Gibson <warthog618@gmail.com> wrote:
    >
    > On Fri, Sep 25, 2020 at 01:12:14PM +0300, Andy Shevchenko wrote:
    > > On Thu, Sep 24, 2020 at 12:48 PM Kent Gibson <warthog618@gmail.com> wrote:
    > > > On Thu, Sep 24, 2020 at 11:39:03AM +0300, Andy Shevchenko wrote:
    > > > > On Thu, Sep 24, 2020 at 5:39 AM Kent Gibson <warthog618@gmail.com> wrote:
    > > > > > On Wed, Sep 23, 2020 at 06:41:45PM +0300, Andy Shevchenko wrote:
    > > > > > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote:
    > >
    >
    > [snip]
    > > >
    > > > Lets say CPU0 is setting 1 and CPU1 setting 2, and assuming the xchg()
    > > > completes...
    > > > Your case is not possible - CPU1 would see the value 1 set by CPU0 in the
    > > > read() and so NOK. Its xchg() would fail as it compares against 0
    > > > and that also sees the 1 and so fails.
    > > >
    > > > What am I missing?
    > >
    > > Barriers? That's what documentation says about xchg().
    > > https://stackoverflow.com/q/20950603/2511795
    > >
    >
    > Firstly, the answer in Stackoverflow is from someone who explicitly
    > acknowledges not being a kernel developer, so they aren't sure.
    >
    > Secondly, the latest version of the kernel doc [1] says differently than what
    > is quoted on Stackoverlow - it indicates implementations of atomic_cmpxchg()
    > must provide its own memory barriers.
    >
    > The relevant section says "This performs an atomic compare exchange operation
    > on the atomic value v, with the given old and new values. Like all atomic_xxx
    > operations, atomic_cmpxchg will only satisfy its atomicity semantics as long
    > as all other accesses of *v are performed through atomic_xxx operations.
    >
    > atomic_cmpxchg must provide explicit memory barriers around the operation,
    > although if the comparison fails then no memory ordering guarantees are required."
    >
    > Note that this doc is aimed at atomic_cmpxchg() implementors, so I took
    > that to mean the operation itself must provide the barriers - not
    > the caller. Also, the sentence only makes sense wrt the
    > atomic_cmpxchg() implementation - the caller can't decide on memory barriers
    > if the comparison fails or not.
    >
    > The memory-barriers.txt they quote is also dated - the atomic section they quote
    > is moved to atomic_t.txt[2]?
    > That says that cmpxchg is a RMW op, and that it will perform an
    > ACQUIRE and RELEASE - for the non-failure case anyway.
    >
    > Again, I took that to mean it will provide the barriers itself.
    >
    > And even the old text they quote says those operations IMPLY a memory barrier,
    > "Any atomic operation that modifies some state in memory and returns
    > information about the state (old or new) implies an SMP-conditional
    > general memory barrier (smp_mb()) on each side of the actual operation"
    > and that "the implicit memory barrier effects are necessary".
    >
    > Again that indicates the barrier is a part of the op, as it is implicit,
    > and not necessary to be added separately.

    Okay!
    Thanks for digging into it.

    > > > > > The atomic_cmpxchg() ensures cdata->watch_abi_version is only set
    > > > > > once - first in wins. The atomic_read() is so we can check that
    > > > > > the set version matches what the caller wants.
    > > > > > Note that multiple callers may request the same version - and all
    > > > > > should succeed.
    > > > >
    > > > > So, that's basically what you need when using _old_ value.
    > > > >
    > > > > 0 means you were first, right?
    > > > > Anything else you simply compare and bail out if it's not the same as
    > > > > what has been asked.
    > > > >
    > > >
    > > > Could you provide a complete implementation that behaves as I expect,
    > > > rather than snippets and verbage?
    > >
    > > if (atomic_cmpxchg(&cdata..., version) == 0)
    > > return 0; // we were first!
    > > return -EPERM; // somebody has changed the version before us!
    > >
    >
    > Which can fail if two callers are requesting the same version - in a
    > race the second one will get a fail - independent of the version they
    > are requesting.
    >
    > I keep flip-flopping and twiddling with the implementation of this -
    > my current one is:
    >
    > /*
    > * returns 0 if the versions match, else the previously selected ABI version
    > */
    > static int lineinfo_ensure_abi_version(struct gpio_chardev_data *cdata,
    > unsigned int version)
    > {
    > int abiv = atomic_cmpxchg(&cdata->watch_abi_version, 0, version);
    >
    > if (abiv == version)
    > return 0;
    >
    > return abiv;
    > }
    >
    >
    > Does that work for you? (assuming no explicit barriers are necessary)

    Perfectly!

    > [1] https://www.kernel.org/doc/html/v5.8/core-api/atomic_ops.html
    > [2] https://www.kernel.org/doc/Documentation/atomic_t.txt

    --
    With Best Regards,
    Andy Shevchenko

    \
     
     \ /
      Last update: 2020-09-25 16:45    [W:3.505 / U:1.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site