Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1430807 > unrolled thread

Re: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls for FETCH-OP flavors

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls  for FETCH-OP flavors James Bottomley <James.Bottomley@HansenPartnership.com> - 2016-06-24 18:50 +0200
    Re: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls  for FETCH-OP flavors Davidlohr Bueso <dave@stgolabs.net> - 2016-06-24 19:40 +0200
      Re: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls  for FETCH-OP flavors James Bottomley <James.Bottomley@HansenPartnership.com> - 2016-06-24 19:50 +0200
        Re: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls  for FETCH-OP flavors Davidlohr Bueso <dave@stgolabs.net> - 2016-06-24 22:40 +0200
      RE: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls for  FETCH-OP flavors KY Srinivasan <kys@microsoft.com> - 2016-06-24 20:10 +0200
        Re: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls  for FETCH-OP flavors Davidlohr Bueso <dave@stgolabs.net> - 2016-06-24 21:40 +0200
    Re: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls  for FETCH-OP flavors Peter Zijlstra <peterz@infradead.org> - 2016-06-24 21:20 +0200

#1430807 — Re: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls for FETCH-OP flavors

On Mon, 2016-06-20 at 13:05 -0700, Davidlohr Bueso wrote:
> Hi,
> 
> The series is really straightforward and based on Peter's work that
> introduces[1] the atomic_fetch_$op machinery. Only patch 1 implements
> the actual atomic_fetch_{inc,dec} calls based on 
> atomic_fetch_{add,sub}.

Could I just ask why?  atomic_inc_return(x) - 1 seems a reasonable
thing to do to me.  Is it because on architectures where atomics are
implemented in asm, it costs us one more CPU instruction to do the
extra decrement which gcc can't optimise?   If that's it, I'm not sure
the added complexity justifies the cycle savings.

James

[toc] | [next] | [standalone]

#1430830

On Fri, 24 Jun 2016, James Bottomley wrote:

>On Mon, 2016-06-20 at 13:05 -0700, Davidlohr Bueso wrote:
>> Hi,
>>
>> The series is really straightforward and based on Peter's work that
>> introduces[1] the atomic_fetch_$op machinery. Only patch 1 implements
>> the actual atomic_fetch_{inc,dec} calls based on
>> atomic_fetch_{add,sub}.
>
>Could I just ask why?  atomic_inc_return(x) - 1 seems a reasonable
>thing to do to me.

For one restoring the old state like that can be racy and looses the notion
of atomicity. The new family of atomic_fetch_$ops also better express the
purpose of the call imo. Finally, the added machinery (considering it came
from fetch_op() NOHZ needs), was mainly suggested by Linus (although yes, we
don't have users for all the calls): https://lkml.org/lkml/2016/3/15/352.

Thanks,
Davidlohr

[toc] | [prev] | [next] | [standalone]

#1430838

On Fri, 2016-06-24 at 10:30 -0700, Davidlohr Bueso wrote:
> On Fri, 24 Jun 2016, James Bottomley wrote:
> 
> > On Mon, 2016-06-20 at 13:05 -0700, Davidlohr Bueso wrote:
> > > Hi,
> > > 
> > > The series is really straightforward and based on Peter's work 
> > > that introduces[1] the atomic_fetch_$op machinery. Only patch 1
> > > implements the actual atomic_fetch_{inc,dec} calls based on
> > > atomic_fetch_{add,sub}.
> > 
> > Could I just ask why?  atomic_inc_return(x) - 1 seems a reasonable
> > thing to do to me.
> 
> For one restoring the old state like that can be racy and looses the 
> notion of atomicity.

I don't understand this argument: any return of an atomic value is
inherently racy because the atomic source may have changed by the time
you use the returned value.  It's no more or less racy to my mind to
return the original value and increment than to return the incremented
value and subtract one.

>  The new family of atomic_fetch_$ops also better express the purpose
> of the call imo.

So this is probably the core of my objection: adding APIs simply
because we can.  A good reason to add things like this is because it's
a common pattern people get wrong, because we can optimize it nicely on
an architecture, or some other good reason.  Absent a good reason it
doesn't seem like a good API addition because trying to keep up with
all the API variants when you want to use atomics adds to the burden of
the programmer.

>  Finally, the added machinery (considering it came from fetch_op()
> NOHZ needs), was mainly suggested by Linus (although yes, we don't
> have users for all the calls): 
> https://lkml.org/lkml/2016/3/15/352.

I'd really rather you told me *you* believed it was a good idea (and
why) ...

James

[toc] | [prev] | [next] | [standalone]

#1430934

On Fri, 24 Jun 2016, James Bottomley wrote:

>On Fri, 2016-06-24 at 10:30 -0700, Davidlohr Bueso wrote:
>> On Fri, 24 Jun 2016, James Bottomley wrote:
>>
>> > On Mon, 2016-06-20 at 13:05 -0700, Davidlohr Bueso wrote:
>> > > Hi,
>> > >
>> > > The series is really straightforward and based on Peter's work
>> > > that introduces[1] the atomic_fetch_$op machinery. Only patch 1
>> > > implements the actual atomic_fetch_{inc,dec} calls based on
>> > > atomic_fetch_{add,sub}.
>> >
>> > Could I just ask why?  atomic_inc_return(x) - 1 seems a reasonable
>> > thing to do to me.
>>
>> For one restoring the old state like that can be racy and looses the
>> notion of atomicity.
>
>I don't understand this argument: any return of an atomic value is
>inherently racy because the atomic source may have changed by the time
>you use the returned value.  It's no more or less racy to my mind to
>return the original value and increment than to return the incremented
>value and subtract one.

I was looking at the n + xadd() vs xadd(). But yeah, same applies to
checking the return value of any cas operation.

>>  The new family of atomic_fetch_$ops also better express the purpose
>> of the call imo.
>
>So this is probably the core of my objection: adding APIs simply
>because we can.  A good reason to add things like this is because it's
>a common pattern people get wrong, because we can optimize it nicely on
>an architecture, or some other good reason.  Absent a good reason it
>doesn't seem like a good API addition because trying to keep up with
>all the API variants when you want to use atomics adds to the burden of
>the programmer.

That all makes sense, but again the 'race' was my main concern. Although
saving the add cycles in the rwsem (loop) path is always welcome. The rest,
and including this patchset, is merely for completeness. 

Thanks,
Davidlohr

[toc] | [prev] | [next] | [standalone]

#1430850 — RE: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls for FETCH-OP flavors

> -----Original Message-----
> From: Davidlohr Bueso [mailto:dave@stgolabs.net]
> Sent: Friday, June 24, 2016 10:30 AM
> To: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: peterz@infradead.org; mingo@kernel.org; davem@davemloft.net;
> cw00.choi@samsung.com; dougthompson@xmission.com; bp@alien8.de;
> mchehab@osg.samsung.com; gregkh@linuxfoundation.org; pfg@sgi.com;
> jikos@kernel.org; hans.verkuil@cisco.com; awalls@md.metrocast.net;
> dledford@redhat.com; sean.hefty@intel.com; KY Srinivasan
> <kys@microsoft.com>; heiko.carstens@de.ibm.com;
> sumit.semwal@linaro.org; schwidefsky@de.ibm.com; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH -tip 00/12] locking/atomics: Add and use inc,dec calls for
> FETCH-OP flavors
> 
> On Fri, 24 Jun 2016, James Bottomley wrote:
> 
> >On Mon, 2016-06-20 at 13:05 -0700, Davidlohr Bueso wrote:
> >> Hi,
> >>
> >> The series is really straightforward and based on Peter's work that
> >> introduces[1] the atomic_fetch_$op machinery. Only patch 1 implements
> >> the actual atomic_fetch_{inc,dec} calls based on
> >> atomic_fetch_{add,sub}.
> >
> >Could I just ask why?  atomic_inc_return(x) - 1 seems a reasonable
> >thing to do to me.
> 
> For one restoring the old state like that can be racy and looses the notion of
> atomicity. The new family of atomic_fetch_$ops also better express the
How so? Can you expand on the racy part. The subtraction is done on a local copy of
the value.

K. Y

> purpose of the call imo. Finally, the added machinery (considering it came from
> fetch_op() NOHZ needs), was mainly suggested by Linus (although yes, we
> don't have users for all the calls):
> https://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2flkml.org%
> 2flkml%2f2016%2f3%2f15%2f352&data=01%7c01%7ckys%40microsoft.com%
> 7c5c7cfad67568440f6e2108d39c5546e0%7c72f988bf86f141af91ab2d7cd011
> db47%7c1&sdata=uZrdmvDCuTp%2bMNHAXzMPT68w%2bVGtvH2V99nUEBr6
> 1ro%3d.
> 
> Thanks,
> Davidlohr

[toc] | [prev] | [next] | [standalone]

#1430896

On Fri, 24 Jun 2016, KY Srinivasan wrote:

>How so? Can you expand on the racy part. The subtraction is done on a local copy of
>the value.

Yeah, you're right. I took a look at the generated code and I was unnecessarily worried
about the window between the return value and the subtraction.

Thanks,
Davidlohr

[toc] | [prev] | [next] | [standalone]

#1430891

On Fri, Jun 24, 2016 at 09:46:05AM -0700, James Bottomley wrote:
> On Mon, 2016-06-20 at 13:05 -0700, Davidlohr Bueso wrote:
> > Hi,
> > 
> > The series is really straightforward and based on Peter's work that
> > introduces[1] the atomic_fetch_$op machinery. Only patch 1 implements
> > the actual atomic_fetch_{inc,dec} calls based on 
> > atomic_fetch_{add,sub}.
> 
> Could I just ask why?  atomic_inc_return(x) - 1 seems a reasonable
> thing to do to me.  Is it because on architectures where atomics are
> implemented in asm, it costs us one more CPU instruction to do the
> extra decrement which gcc can't optimise?   If that's it, I'm not sure
> the added complexity justifies the cycle savings.

That boat has sailed, fetch_$op is implemented (in asm mostly) for _all_
architectures already.

All Davidlohr does here is add fetch_{inc,dec}(v) -> fetch_{add,sub}(1,
v) macros because he's lazy.

In any case, fetch_$op is the natural form of atomics that return a
value; Linux has historically chosen the 'wrong' form. The fetch_$op,
test-and-modify, load-store whatever is what hardware typically does
natively and is what works for irreversible operations.

Sure, for reversible operations (add/sub) what you say can (and is)
done, and then we hope the compiler knows that x-x == 0 (and it
typically does). As you say, that's slightly sub-optimal for archs where
the compiler cannot see into the atomic (typically LL/SC archs).

But add/sub were _2_ lines extra after I did all the groundwork for
fetch_{or,and,xor}. So we might as well save those few extra add/dec
cycles. Some of them are in fairly hot paths.

Lastly; and the weakest argument; fetch_$op is what C11 has, probably
because the above reasons.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web