| Message ID | 20221204031430.662169-1-mawupeng1@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-next,1/1] percpu: cleanup invalid assignment to err in pcpu_alloc | expand |
On 12/04/22 at 11:14am, Wupeng Ma wrote: > From: Ma Wupeng <mawupeng1@huawei.com> > > Assignment to err if is_atomic is true will never be used since warn > message can only be shown if is_atomic is false after label fail. So drop > it. > > Signed-off-by: Ma Wupeng <mawupeng1@huawei.com> > --- > mm/percpu.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/mm/percpu.c b/mm/percpu.c > index acd78da0493b..df86d79325b2 100644 > --- a/mm/percpu.c > +++ b/mm/percpu.c > @@ -1817,10 +1817,8 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, > > spin_unlock_irqrestore(&pcpu_lock, flags); > > - if (is_atomic) { > - err = "atomic alloc failed, no space left"; > + if (is_atomic) > goto fail; > - } This is good catch. But I think Dennis may not like this way because he added the message intentionally in commit 11df02bf9bc1 ("percpu: resolve err may not be initialized in pcpu_alloc"). Can we change the conditional checking in fail part as below? diff --git a/mm/percpu.c b/mm/percpu.c index 27697b2429c2..0ac55500fad9 100644 --- a/mm/percpu.c +++ b/mm/percpu.c @@ -1897,7 +1897,7 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, fail: trace_percpu_alloc_percpu_fail(reserved, is_atomic, size, align); - if (!is_atomic && do_warn && warn_limit) { + if (do_warn && warn_limit) { pr_warn("allocation failed, size=%zu align=%zu atomic=%d, %s\n", size, align, is_atomic, err); dump_stack();
On Sun, 4 Dec 2022, Baoquan He wrote:
> Can we change the conditional checking in fail part as below?
Yes please. This is potentially a rare condition that should get some
visibility.
Hi Baoquan and Wupeng, On Sun, Dec 04, 2022 at 08:11:23PM +0800, Baoquan He wrote: > On 12/04/22 at 11:14am, Wupeng Ma wrote: > > From: Ma Wupeng <mawupeng1@huawei.com> > > > > Assignment to err if is_atomic is true will never be used since warn > > message can only be shown if is_atomic is false after label fail. So drop > > it. > > > > Signed-off-by: Ma Wupeng <mawupeng1@huawei.com> > > --- > > mm/percpu.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/mm/percpu.c b/mm/percpu.c > > index acd78da0493b..df86d79325b2 100644 > > --- a/mm/percpu.c > > +++ b/mm/percpu.c > > @@ -1817,10 +1817,8 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, > > > > spin_unlock_irqrestore(&pcpu_lock, flags); > > > > - if (is_atomic) { > > - err = "atomic alloc failed, no space left"; > > + if (is_atomic) > > goto fail; > > - } > > This is good catch. But I think Dennis may not like this way because he > added the message intentionally in commit 11df02bf9bc1 ("percpu: resolve > err may not be initialized in pcpu_alloc"). > You're right Baoquan haha. I agree with Christoph as well we should surface atomic. Though I don't think below is quite right either. We should likely have a separate warn_limit for atomic and I need to think about dump_stack() if there are any requirements there. Thanks, Dennis > Can we change the conditional checking in fail part as below? > > diff --git a/mm/percpu.c b/mm/percpu.c > index 27697b2429c2..0ac55500fad9 100644 > --- a/mm/percpu.c > +++ b/mm/percpu.c > @@ -1897,7 +1897,7 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, > fail: > trace_percpu_alloc_percpu_fail(reserved, is_atomic, size, align); > > - if (!is_atomic && do_warn && warn_limit) { > + if (do_warn && warn_limit) { > pr_warn("allocation failed, size=%zu align=%zu atomic=%d, %s\n", > size, align, is_atomic, err); > dump_stack(); >
On 12/04/22 at 04:30pm, Dennis Zhou wrote: > Hi Baoquan and Wupeng, > > On Sun, Dec 04, 2022 at 08:11:23PM +0800, Baoquan He wrote: > > On 12/04/22 at 11:14am, Wupeng Ma wrote: > > > From: Ma Wupeng <mawupeng1@huawei.com> > > > > > > Assignment to err if is_atomic is true will never be used since warn > > > message can only be shown if is_atomic is false after label fail. So drop > > > it. > > > > > > Signed-off-by: Ma Wupeng <mawupeng1@huawei.com> > > > --- > > > mm/percpu.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/mm/percpu.c b/mm/percpu.c > > > index acd78da0493b..df86d79325b2 100644 > > > --- a/mm/percpu.c > > > +++ b/mm/percpu.c > > > @@ -1817,10 +1817,8 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, > > > > > > spin_unlock_irqrestore(&pcpu_lock, flags); > > > > > > - if (is_atomic) { > > > - err = "atomic alloc failed, no space left"; > > > + if (is_atomic) > > > goto fail; > > > - } > > > > This is good catch. But I think Dennis may not like this way because he > > added the message intentionally in commit 11df02bf9bc1 ("percpu: resolve > > err may not be initialized in pcpu_alloc"). > > > > You're right Baoquan haha. I agree with Christoph as well we should > surface atomic. > > Though I don't think below is quite right either. We should likely have > a separate warn_limit for atomic and I need to think about dump_stack() > if there are any requirements there. Yeah, sounds reasonable. I didn't think it over. > > > > Can we change the conditional checking in fail part as below? > > > > diff --git a/mm/percpu.c b/mm/percpu.c > > index 27697b2429c2..0ac55500fad9 100644 > > --- a/mm/percpu.c > > +++ b/mm/percpu.c > > @@ -1897,7 +1897,7 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, > > fail: > > trace_percpu_alloc_percpu_fail(reserved, is_atomic, size, align); > > > > - if (!is_atomic && do_warn && warn_limit) { > > + if (do_warn && warn_limit) { > > pr_warn("allocation failed, size=%zu align=%zu atomic=%d, %s\n", > > size, align, is_atomic, err); > > dump_stack(); > > >
diff --git a/mm/percpu.c b/mm/percpu.c index acd78da0493b..df86d79325b2 100644 --- a/mm/percpu.c +++ b/mm/percpu.c @@ -1817,10 +1817,8 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved, spin_unlock_irqrestore(&pcpu_lock, flags); - if (is_atomic) { - err = "atomic alloc failed, no space left"; + if (is_atomic) goto fail; - } /* No space left. Create a new chunk. */ if (list_empty(&pcpu_chunk_lists[pcpu_free_slot])) {