From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751372AbcFXUf5 (ORCPT ); Fri, 24 Jun 2016 16:35:57 -0400 Received: from mx2.suse.de ([195.135.220.15]:60821 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751082AbcFXUf4 (ORCPT ); Fri, 24 Jun 2016 16:35:56 -0400 Date: Fri, 24 Jun 2016 13:35:36 -0700 From: Davidlohr Bueso To: James Bottomley 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, 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 Message-ID: <20160624203536.GA2763@linux-80c1.suse> References: <1466453164-13185-1-git-send-email-dave@stgolabs.net> <1466786765.2343.37.camel@HansenPartnership.com> <20160624173028.GA24658@linux-80c1.suse> <1466790244.2343.60.camel@HansenPartnership.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <1466790244.2343.60.camel@HansenPartnership.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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