Re: [PATCH v9 08/20] gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Fri, Sep 25, 2020 at 2:56 PM Kent Gibson <warthog618@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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