* [PATCH -next 1/1] percpu: cleanup invalid assignment to err in pcpu_alloc @ 2022-12-04 3:14 Wupeng Ma 2022-12-04 12:11 ` Baoquan He 0 siblings, 1 reply; 5+ messages in thread From: Wupeng Ma @ 2022-12-04 3:14 UTC (permalink / raw) To: akpm, tj, dennis, cl; +Cc: linux-mm, linux-kernel, mawupeng1 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; - } /* No space left. Create a new chunk. */ if (list_empty(&pcpu_chunk_lists[pcpu_free_slot])) { -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH -next 1/1] percpu: cleanup invalid assignment to err in pcpu_alloc 2022-12-04 3:14 [PATCH -next 1/1] percpu: cleanup invalid assignment to err in pcpu_alloc Wupeng Ma @ 2022-12-04 12:11 ` Baoquan He 2022-12-04 16:21 ` Christoph Lameter 2022-12-05 0:30 ` Dennis Zhou 0 siblings, 2 replies; 5+ messages in thread From: Baoquan He @ 2022-12-04 12:11 UTC (permalink / raw) To: Wupeng Ma; +Cc: akpm, tj, dennis, cl, linux-mm, linux-kernel 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(); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH -next 1/1] percpu: cleanup invalid assignment to err in pcpu_alloc 2022-12-04 12:11 ` Baoquan He @ 2022-12-04 16:21 ` Christoph Lameter 2022-12-05 0:30 ` Dennis Zhou 1 sibling, 0 replies; 5+ messages in thread From: Christoph Lameter @ 2022-12-04 16:21 UTC (permalink / raw) To: Baoquan He; +Cc: Wupeng Ma, akpm, tj, dennis, linux-mm, linux-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next 1/1] percpu: cleanup invalid assignment to err in pcpu_alloc 2022-12-04 12:11 ` Baoquan He 2022-12-04 16:21 ` Christoph Lameter @ 2022-12-05 0:30 ` Dennis Zhou 2022-12-05 2:24 ` Baoquan He 1 sibling, 1 reply; 5+ messages in thread From: Dennis Zhou @ 2022-12-05 0:30 UTC (permalink / raw) To: Baoquan He, Wupeng Ma; +Cc: akpm, tj, cl, linux-mm, linux-kernel 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(); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next 1/1] percpu: cleanup invalid assignment to err in pcpu_alloc 2022-12-05 0:30 ` Dennis Zhou @ 2022-12-05 2:24 ` Baoquan He 0 siblings, 0 replies; 5+ messages in thread From: Baoquan He @ 2022-12-05 2:24 UTC (permalink / raw) To: Dennis Zhou; +Cc: Wupeng Ma, akpm, tj, cl, linux-mm, linux-kernel 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(); > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-12-05 2:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-04 3:14 [PATCH -next 1/1] percpu: cleanup invalid assignment to err in pcpu_alloc Wupeng Ma 2022-12-04 12:11 ` Baoquan He 2022-12-04 16:21 ` Christoph Lameter 2022-12-05 0:30 ` Dennis Zhou 2022-12-05 2:24 ` Baoquan He
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).