Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
> On 01 February 2017 at 10:25 Peter Zijlstra 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
Re: [PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
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()
> On 31 January 2017 at 20:17 Peter Zijlstra wrote: > > > On Tue, Jan 31, 2017 at 06:41:28PM +0100, Fabian Frederick wrote: > > > > > > > On 31 January 2017 at 11:41 Peter Zijlstra 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()
On Tue, Jan 31, 2017 at 06:41:28PM +0100, Fabian Frederick wrote: > > > > On 31 January 2017 at 11:41 Peter Zijlstra 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()
> On 31 January 2017 at 11:41 Peter Zijlstra 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()
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 wrote:
> complementary definition to atomic_inc_not_zero() featured in
> lib/fault-inject.c
>
> Signed-off-by: Fabian Frederick
> ---
> 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
[PATCH 01/14] locking/atomic: import atomic_dec_not_zero()
complementary definition to atomic_inc_not_zero() featured in
lib/fault-inject.c
Signed-off-by: Fabian Frederick
---
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
