Re: [PATCH percpu/for-3.11 1/2] percpu-refcount: add __must_check to percpu_ref_init() and don't use ACCESS_ONCE() in percpu_ref_kill_rcu()
Hello, On Thu, Jun 20, 2013 at 10:29:51AM +0930, Rusty Russell wrote: > Now seems to get abused by lazy coders who blame users for their own > broken APIs. And Ubuntu, who turn it on by default in their gcc when > optimizing. Yeah, it's a sore point :) How is the API broken? It is a function which either succeeds or fails and, if it fails during boot, like any other allocation failures during boot, the boot fails. It's not different from kmalloc() returning NULL on failure. > if (system(command)) > doesnt_matter(); It *does* matter. > Sorry, I was unclear. If you fail the percpu allocation, you have a > counter which is always in atomic mode. > > This saves everyone a headache. init doesn't fail, no poorly-tested > failure paths, no whining Rusty. How does that save a headache? It *should* fail if allocation fails. Having untraceable persistent slow down after heavy memory pressure is no fun to track down. If you're worried about not being able to detect bugs in error path, the right thing to do would be inducing allocation failures regularly so that those paths can be tested, which we already do. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH percpu/for-3.11 1/2] percpu-refcount: add __must_check to percpu_ref_init() and don't use ACCESS_ONCE() in percpu_ref_kill_rcu()
Tejun Heo writes: > On Wed, Jun 19, 2013 at 12:25:14PM +0930, Rusty Russell wrote: >> But it's quite OK to ignore OOM errors in builtin init functions. > > I think it'd be cleaner to let those use cases use BUG_ON() around it. > We really want most users to be checking its return value. Yeah, but it's an admission of API design failure. __attribute__((warn_unused_result)) is a bad implementation of a poorly conceived idea. It was originally designed to catch realloc misuse, which is presumably why casting to (void) doesn't suppress it. Protecting realloc properly would mean the GCC understanding that the pointer arg handed to realloc was no longer valid which would catch many more cases, but compilers are hard, so we got the hacky attribute. Now seems to get abused by lazy coders who blame users for their own broken APIs. And Ubuntu, who turn it on by default in their gcc when optimizing. Yeah, it's a sore point :) So I end up writing code like this (to quote from ccan): /* Gcc's warn_unused_result is fascist bullshit. */ #define doesnt_matter() ... if (system(command)) doesnt_matter(); >> It would be neatest to have it fail into slow mode, of course, but it's >> probably not worth the pain. > > percpu allocation is always GFP_KERNEL, so it can't get any slower > without deadlocking. Sorry, I was unclear. If you fail the percpu allocation, you have a counter which is always in atomic mode. This saves everyone a headache. init doesn't fail, no poorly-tested failure paths, no whining Rusty. Rant over, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH percpu/for-3.11 1/2] percpu-refcount: add __must_check to percpu_ref_init() and don't use ACCESS_ONCE() in percpu_ref_kill_rcu()
On Wed, Jun 19, 2013 at 12:25:14PM +0930, Rusty Russell wrote: > But it's quite OK to ignore OOM errors in builtin init functions. I think it'd be cleaner to let those use cases use BUG_ON() around it. We really want most users to be checking its return value. > It would be neatest to have it fail into slow mode, of course, but it's > probably not worth the pain. percpu allocation is always GFP_KERNEL, so it can't get any slower without deadlocking. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH percpu/for-3.11 1/2] percpu-refcount: add __must_check to percpu_ref_init() and don't use ACCESS_ONCE() in percpu_ref_kill_rcu()
On Wed, Jun 19, 2013 at 12:25:14PM +0930, Rusty Russell wrote: But it's quite OK to ignore OOM errors in builtin init functions. I think it'd be cleaner to let those use cases use BUG_ON() around it. We really want most users to be checking its return value. It would be neatest to have it fail into slow mode, of course, but it's probably not worth the pain. percpu allocation is always GFP_KERNEL, so it can't get any slower without deadlocking. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH percpu/for-3.11 1/2] percpu-refcount: add __must_check to percpu_ref_init() and don't use ACCESS_ONCE() in percpu_ref_kill_rcu()
Tejun Heo t...@kernel.org writes: On Wed, Jun 19, 2013 at 12:25:14PM +0930, Rusty Russell wrote: But it's quite OK to ignore OOM errors in builtin init functions. I think it'd be cleaner to let those use cases use BUG_ON() around it. We really want most users to be checking its return value. Yeah, but it's an admission of API design failure. __attribute__((warn_unused_result)) is a bad implementation of a poorly conceived idea. It was originally designed to catch realloc misuse, which is presumably why casting to (void) doesn't suppress it. Protecting realloc properly would mean the GCC understanding that the pointer arg handed to realloc was no longer valid which would catch many more cases, but compilers are hard, so we got the hacky attribute. Now seems to get abused by lazy coders who blame users for their own broken APIs. And Ubuntu, who turn it on by default in their gcc when optimizing. Yeah, it's a sore point :) So I end up writing code like this (to quote from ccan): /* Gcc's warn_unused_result is fascist bullshit. */ #define doesnt_matter() ... if (system(command)) doesnt_matter(); It would be neatest to have it fail into slow mode, of course, but it's probably not worth the pain. percpu allocation is always GFP_KERNEL, so it can't get any slower without deadlocking. Sorry, I was unclear. If you fail the percpu allocation, you have a counter which is always in atomic mode. This saves everyone a headache. init doesn't fail, no poorly-tested failure paths, no whining Rusty. Rant over, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH percpu/for-3.11 1/2] percpu-refcount: add __must_check to percpu_ref_init() and don't use ACCESS_ONCE() in percpu_ref_kill_rcu()
Hello, On Thu, Jun 20, 2013 at 10:29:51AM +0930, Rusty Russell wrote: Now seems to get abused by lazy coders who blame users for their own broken APIs. And Ubuntu, who turn it on by default in their gcc when optimizing. Yeah, it's a sore point :) How is the API broken? It is a function which either succeeds or fails and, if it fails during boot, like any other allocation failures during boot, the boot fails. It's not different from kmalloc() returning NULL on failure. if (system(command)) doesnt_matter(); It *does* matter. Sorry, I was unclear. If you fail the percpu allocation, you have a counter which is always in atomic mode. This saves everyone a headache. init doesn't fail, no poorly-tested failure paths, no whining Rusty. How does that save a headache? It *should* fail if allocation fails. Having untraceable persistent slow down after heavy memory pressure is no fun to track down. If you're worried about not being able to detect bugs in error path, the right thing to do would be inducing allocation failures regularly so that those paths can be tested, which we already do. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH percpu/for-3.11 1/2] percpu-refcount: add __must_check to percpu_ref_init() and don't use ACCESS_ONCE() in percpu_ref_kill_rcu()
Tejun Heo writes: > Two small changes. > > * Unlike most init functions, percpu_ref_init() allocates memory and > may fail. Let's mark it with __must_check in case the caller > forgets. But it's quite OK to ignore OOM errors in builtin init functions. It would be neatest to have it fail into slow mode, of course, but it's probably not worth the pain. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH percpu/for-3.11 1/2] percpu-refcount: add __must_check to percpu_ref_init() and don't use ACCESS_ONCE() in percpu_ref_kill_rcu()
Tejun Heo t...@kernel.org writes: Two small changes. * Unlike most init functions, percpu_ref_init() allocates memory and may fail. Let's mark it with __must_check in case the caller forgets. But it's quite OK to ignore OOM errors in builtin init functions. It would be neatest to have it fail into slow mode, of course, but it's probably not worth the pain. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH percpu/for-3.11 1/2] percpu-refcount: add __must_check to percpu_ref_init() and don't use ACCESS_ONCE() in percpu_ref_kill_rcu()
Two small changes.
* Unlike most init functions, percpu_ref_init() allocates memory and
may fail. Let's mark it with __must_check in case the caller
forgets.
* percpu_ref_kill_rcu() is unnecessarily using ACCESS_ONCE() to
dereference @ref->pcpu_count, which can be misleading. The pointer
is guaranteed to be valid and visible and can't change underneath
the function. Drop ACCESS_ONCE().
Signed-off-by: Tejun Heo
---
include/linux/percpu-refcount.h |3 ++-
lib/percpu-refcount.c |4 +---
2 files changed, 3 insertions(+), 4 deletions(-)
--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -67,7 +67,8 @@ struct percpu_ref {
struct rcu_head rcu;
};
-int percpu_ref_init(struct percpu_ref *ref, percpu_ref_func_t *release);
+int __must_check percpu_ref_init(struct percpu_ref *ref,
+percpu_ref_func_t *release);
void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
percpu_ref_func_t *confirm_kill);
--- a/lib/percpu-refcount.c
+++ b/lib/percpu-refcount.c
@@ -57,12 +57,10 @@ int percpu_ref_init(struct percpu_ref *r
static void percpu_ref_kill_rcu(struct rcu_head *rcu)
{
struct percpu_ref *ref = container_of(rcu, struct percpu_ref, rcu);
- unsigned __percpu *pcpu_count;
+ unsigned __percpu *pcpu_count = ref->pcpu_count;
unsigned count = 0;
int cpu;
- pcpu_count = ACCESS_ONCE(ref->pcpu_count);
-
/* Mask out PCPU_REF_DEAD */
pcpu_count = (unsigned __percpu *)
(((unsigned long) pcpu_count) & ~PCPU_STATUS_MASK);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH percpu/for-3.11 1/2] percpu-refcount: add __must_check to percpu_ref_init() and don't use ACCESS_ONCE() in percpu_ref_kill_rcu()
Two small changes.
* Unlike most init functions, percpu_ref_init() allocates memory and
may fail. Let's mark it with __must_check in case the caller
forgets.
* percpu_ref_kill_rcu() is unnecessarily using ACCESS_ONCE() to
dereference @ref-pcpu_count, which can be misleading. The pointer
is guaranteed to be valid and visible and can't change underneath
the function. Drop ACCESS_ONCE().
Signed-off-by: Tejun Heo t...@kernel.org
---
include/linux/percpu-refcount.h |3 ++-
lib/percpu-refcount.c |4 +---
2 files changed, 3 insertions(+), 4 deletions(-)
--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -67,7 +67,8 @@ struct percpu_ref {
struct rcu_head rcu;
};
-int percpu_ref_init(struct percpu_ref *ref, percpu_ref_func_t *release);
+int __must_check percpu_ref_init(struct percpu_ref *ref,
+percpu_ref_func_t *release);
void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
percpu_ref_func_t *confirm_kill);
--- a/lib/percpu-refcount.c
+++ b/lib/percpu-refcount.c
@@ -57,12 +57,10 @@ int percpu_ref_init(struct percpu_ref *r
static void percpu_ref_kill_rcu(struct rcu_head *rcu)
{
struct percpu_ref *ref = container_of(rcu, struct percpu_ref, rcu);
- unsigned __percpu *pcpu_count;
+ unsigned __percpu *pcpu_count = ref-pcpu_count;
unsigned count = 0;
int cpu;
- pcpu_count = ACCESS_ONCE(ref-pcpu_count);
-
/* Mask out PCPU_REF_DEAD */
pcpu_count = (unsigned __percpu *)
(((unsigned long) pcpu_count) ~PCPU_STATUS_MASK);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
