* [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
@ 2017-01-30 18:39 Fabian Frederick
2017-01-31 7:33 ` Ingo Molnar
2017-01-31 10:41 ` Peter Zijlstra
0 siblings, 2 replies; 8+ messages in thread
From: Fabian Frederick @ 2017-01-30 18:39 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, Ingo Molnar, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
2017-01-30 18:39 [PATCH 01/14] locking/atomic: import atomic_dec_not_zero() Fabian Frederick
@ 2017-01-31 7:33 ` Ingo Molnar
2017-01-31 10:41 ` Peter Zijlstra
1 sibling, 0 replies; 8+ messages in thread
From: Ingo Molnar @ 2017-01-31 7:33 UTC (permalink / raw)
To: Fabian Frederick; +Cc: Andrew Morton, linux-kernel
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
2017-01-30 18:39 [PATCH 01/14] locking/atomic: import atomic_dec_not_zero() Fabian Frederick
2017-01-31 7:33 ` Ingo Molnar
@ 2017-01-31 10:41 ` Peter Zijlstra
2017-01-31 17:41 ` Fabian Frederick
1 sibling, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2017-01-31 10:41 UTC (permalink / raw)
To: Fabian Frederick; +Cc: Andrew Morton, linux-kernel, Ingo Molnar
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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
2017-01-31 10:41 ` Peter Zijlstra
@ 2017-01-31 17:41 ` Fabian Frederick
2017-01-31 19:17 ` Peter Zijlstra
0 siblings, 1 reply; 8+ messages in thread
From: Fabian Frederick @ 2017-01-31 17:41 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Andrew Morton, Ingo Molnar, linux-kernel
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
2017-01-31 17:41 ` Fabian Frederick
@ 2017-01-31 19:17 ` Peter Zijlstra
2017-01-31 20:55 ` Fabian Frederick
0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2017-01-31 19:17 UTC (permalink / raw)
To: Fabian Frederick; +Cc: Andrew Morton, Ingo Molnar, linux-kernel
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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
2017-01-31 19:17 ` Peter Zijlstra
@ 2017-01-31 20:55 ` Fabian Frederick
2017-02-01 9:25 ` Peter Zijlstra
0 siblings, 1 reply; 8+ messages in thread
From: Fabian Frederick @ 2017-01-31 20:55 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Andrew Morton, Ingo Molnar, linux-kernel
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
2017-01-31 20:55 ` Fabian Frederick
@ 2017-02-01 9:25 ` Peter Zijlstra
2017-02-01 19:24 ` Fabian Frederick
0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2017-02-01 9:25 UTC (permalink / raw)
To: Fabian Frederick; +Cc: Andrew Morton, Ingo Molnar, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
2017-02-01 9:25 ` Peter Zijlstra
@ 2017-02-01 19:24 ` Fabian Frederick
0 siblings, 0 replies; 8+ messages in thread
From: Fabian Frederick @ 2017-02-01 19:24 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Andrew Morton, Ingo Molnar, linux-kernel
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-02-01 19:24 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-30 18:39 [PATCH 01/14] locking/atomic: import atomic_dec_not_zero() Fabian Frederick
2017-01-31 7:33 ` Ingo Molnar
2017-01-31 10:41 ` Peter Zijlstra
2017-01-31 17:41 ` Fabian Frederick
2017-01-31 19:17 ` Peter Zijlstra
2017-01-31 20:55 ` Fabian Frederick
2017-02-01 9:25 ` Peter Zijlstra
2017-02-01 19:24 ` Fabian Frederick
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).