[PATCH 01/14] locking/atomic: import atomic_dec_not_zero()

[PATCH 01/14] locking/atomic: import atomic_dec_not_zero()

[Fabian Frederick]

complementary definition to atomic_inc_not_zero() featured in
lib/fault-inject.c

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 include/linux/atomic.h | 2 ++
 lib/fault-inject.c     | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index e71835b..d8e6551 100644
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -517,6 +517,8 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
 #define atomic_inc_not_zero(v)		atomic_add_unless((v), 1, 0)
 #endif
 
+#define atomic_dec_not_zero(v)          atomic_add_unless((v), -1, 0)
+
 #ifndef atomic_andnot
 static inline void atomic_andnot(int i, atomic_t *v)
 {
diff --git a/lib/fault-inject.c b/lib/fault-inject.c
index 6a823a5..4ad5dcc 100644
--- a/lib/fault-inject.c
+++ b/lib/fault-inject.c
@@ -52,8 +52,6 @@ static void fail_dump(struct fault_attr *attr)
 	}
 }
 
-#define atomic_dec_not_zero(v)		atomic_add_unless((v), -1, 0)
-
 static bool fail_task(struct fault_attr *attr, struct task_struct *task)
 {
 	return !in_interrupt() && task->make_it_fail;
-- 
2.9.3

Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()

[Ingo Molnar]

* Fabian Frederick <fabf@skynet.be> wrote:

> complementary definition to atomic_inc_not_zero() featured in
> lib/fault-inject.c
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> ---
>  include/linux/atomic.h | 2 ++
>  lib/fault-inject.c     | 2 --
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/atomic.h b/include/linux/atomic.h
> index e71835b..d8e6551 100644
> --- a/include/linux/atomic.h
> +++ b/include/linux/atomic.h
> @@ -517,6 +517,8 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
>  #define atomic_inc_not_zero(v)		atomic_add_unless((v), 1, 0)
>  #endif
>  
> +#define atomic_dec_not_zero(v)          atomic_add_unless((v), -1, 0)
> +
>  #ifndef atomic_andnot
>  static inline void atomic_andnot(int i, atomic_t *v)
>  {
> diff --git a/lib/fault-inject.c b/lib/fault-inject.c
> index 6a823a5..4ad5dcc 100644
> --- a/lib/fault-inject.c
> +++ b/lib/fault-inject.c
> @@ -52,8 +52,6 @@ static void fail_dump(struct fault_attr *attr)
>  	}
>  }
>  
> -#define atomic_dec_not_zero(v)		atomic_add_unless((v), -1, 0)
> -

Why did you convert the tabs to spaces?

Thanks,

	Ingo

Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()

[Peter Zijlstra]

On Mon, Jan 30, 2017 at 07:39:38PM +0100, Fabian Frederick wrote:
> complementary definition to atomic_inc_not_zero() featured in
> lib/fault-inject.c

Why?

Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()

[Fabian Frederick]

> On 31 January 2017 at 11:41 Peter Zijlstra <peterz@infradead.org> wrote:
>
>
> On Mon, Jan 30, 2017 at 07:39:38PM +0100, Fabian Frederick wrote:
> > complementary definition to atomic_inc_not_zero() featured in
> > lib/fault-inject.c
>
> Why?

Maybe this commit message should be ok ?

complementary definition to atomic_inc_not_zero() featured in lib/fault-inject.c
and is more readable than atomic_add_unless((v), -1, 0) used in different
places.

Andrew, do I have to submit this patch again replacing spaces with tabs in code
line ?

Regards,
Fabian

Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()

[Peter Zijlstra]

On Tue, Jan 31, 2017 at 06:41:28PM +0100, Fabian Frederick wrote:
> 
> 
> > On 31 January 2017 at 11:41 Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >
> > On Mon, Jan 30, 2017 at 07:39:38PM +0100, Fabian Frederick wrote:
> > > complementary definition to atomic_inc_not_zero() featured in
> > > lib/fault-inject.c
> >
> > Why?
> 
> Maybe this commit message should be ok ?
> 
> complementary definition to atomic_inc_not_zero() featured in lib/fault-inject.c
> and is more readable than atomic_add_unless((v), -1, 0) used in different
> places.

I still don't see why such a primitive makes sense. Yes there's a few
usage sites, but from them I don't see a sensible pattern.

What sane pattern desires this?

Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()

[Fabian Frederick]

> On 31 January 2017 at 20:17 Peter Zijlstra <peterz@infradead.org> wrote:
>
>
> On Tue, Jan 31, 2017 at 06:41:28PM +0100, Fabian Frederick wrote:
> >
> >
> > > On 31 January 2017 at 11:41 Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > >
> > > On Mon, Jan 30, 2017 at 07:39:38PM +0100, Fabian Frederick wrote:
> > > > complementary definition to atomic_inc_not_zero() featured in
> > > > lib/fault-inject.c
> > >
> > > Why?
> >
> > Maybe this commit message should be ok ?
> >
> > complementary definition to atomic_inc_not_zero() featured in
> > lib/fault-inject.c
> > and is more readable than atomic_add_unless((v), -1, 0) used in different
> > places.
>
> I still don't see why such a primitive makes sense. Yes there's a few
> usage sites, but from them I don't see a sensible pattern.
>
> What sane pattern desires this?

Once again it's just about readability:

"add -1 unless value is zero" looks more complex in code than "dec not zero"
but maybe it's just a matter of taste :) It it's not the case why would there be
more sense about having
atomic_inc_not_zero() globally ?

Regards,
Fabian

Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()

[Peter Zijlstra]

On Tue, Jan 31, 2017 at 09:55:08PM +0100, Fabian Frederick wrote:
> Once again it's just about readability:

I feel APIs should be about common use-cases, not about sporadic weird cases.

> "add -1 unless value is zero" looks more complex in code than "dec not zero"
> but maybe it's just a matter of taste :) It it's not the case why would there be
> more sense about having
> atomic_inc_not_zero() globally ?

inc_not_zero() has a very strong use-case, its for lockless refcount
increment. Incrementing a 0 reference count is bad because the object
will be freed and you'll have a use-after-free.

Arguably, once we move reference counting over to its own type, it would
make sense to remove it from atomic, specifically to discourage that use
case.

Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()

[Fabian Frederick]

> On 01 February 2017 at 10:25 Peter Zijlstra <peterz@infradead.org> wrote:
>
>
> On Tue, Jan 31, 2017 at 09:55:08PM +0100, Fabian Frederick wrote:
> > Once again it's just about readability:
>
> I feel APIs should be about common use-cases, not about sporadic weird cases.
>
> > "add -1 unless value is zero" looks more complex in code than "dec not zero"
> > but maybe it's just a matter of taste :) It it's not the case why would
> > there be
> > more sense about having
> > atomic_inc_not_zero() globally ?
>
> inc_not_zero() has a very strong use-case, its for lockless refcount
> increment. Incrementing a 0 reference count is bad because the object
> will be freed and you'll have a use-after-free.
>
> Arguably, once we move reference counting over to its own type, it would
> make sense to remove it from atomic, specifically to discourage that use
> case.

Do you know about some commit doing such conversion or could you give me 2
distinct models
so I could compare ? btw atomic_inc_not_zero() is also adviced in
Documentation/RCU/rcuref.txt

Regards,
Fabian