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

[Kent Gibson <warthog618@gmail.com>]

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.

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

Cheers,
Kent.

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