Message ID | 20221204031430.662169-1-mawupeng1@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp1652522wrr; Sat, 3 Dec 2022 21:26:31 -0800 (PST) X-Google-Smtp-Source: AA0mqf7xdV6fORMXWZSxjLtT2Bhi9GW0fHKtmztcyMDj9OAdToDfFMb3UB8DmXAKl8YeOmM0ipVi X-Received: by 2002:a17:90a:f30d:b0:219:536b:41ef with SMTP id ca13-20020a17090af30d00b00219536b41efmr25901831pjb.71.1670131591038; Sat, 03 Dec 2022 21:26:31 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1670131591; cv=none; d=google.com; s=arc-20160816; b=qoOxZAlVNFB+1HYJG58XUGieC6b+5xJ5ubry2Hr/JjaMpBRIySK+dSSLgydmHCJq3+ lIbmVrDG4JLDN9hHcxzM+EWhh6/JtLn8T4Su0ABGQxooo4tW2m61672auLJTy2PZxbKx hoquwqqxgOakQS18p5wQM/1SfNc9WeCSMrypuU2A0/dRTfq+WwhFwxQSwK9G7WzMdoUB Ay1OS7zQbhVRsipy/VNnNhKo1y1UXhcMVvH74sjgO6I4MTyJDZGiSwdrGoNmdj3boieO jRU2USX5iGzZdeyEi5XCX8QD0u2Gj8Gqq0Xt2kSjv2edUVnSZZxSzcSkA1nC6JyiKsLP w9qw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=A94Jx5gpZnDfPPcO1nelBjLpEijULp99U4xEQt4BRR8=; b=T5HDm38jTNI4AXvdiDALlCa0JZrAa2WIiupUauHXh53OTVrrN2dxteMMuO/4u6ytHo xqIEq7r68Q7WMhsrw1jVTZEY9JvpgNF0C0JX2kbg8BH2zg21pazFWtaPU8PWNsAt+zFU UojDTc4iL1ptuWFnRVNfP3MNvD0U53GhjxgSTsIP321qrYSq2KKjm2U8igmgM2ejAvdL zSCYgUa6CTcMnWkZpmafebjF4Td/BSI3rk85HFP+0QaenXlSqXhAW1oXldCd+meHdhm7 qlZEywbAjwKLtptf5p5YtRNF6VVgIoOO1DDtVVsmijMgkf8xNZtbAQm/ytOfFwRwVeYC Y4QQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b10-20020a6541ca000000b0047769403088si11479377pgq.627.2022.12.03.21.26.01; Sat, 03 Dec 2022 21:26:31 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229870AbiLDDOr (ORCPT <rfc822;jaysivo@gmail.com> + 99 others); Sat, 3 Dec 2022 22:14:47 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45346 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229765AbiLDDOq (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 3 Dec 2022 22:14:46 -0500 Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0505D1903B for <linux-kernel@vger.kernel.org>; Sat, 3 Dec 2022 19:14:44 -0800 (PST) Received: from dggpemm500014.china.huawei.com (unknown [172.30.72.54]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4NPsGG1pZgz15MvX; Sun, 4 Dec 2022 11:13:58 +0800 (CST) Received: from localhost.localdomain (10.175.112.125) by dggpemm500014.china.huawei.com (7.185.36.153) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Sun, 4 Dec 2022 11:14:42 +0800 From: Wupeng Ma <mawupeng1@huawei.com> To: <akpm@linux-foundation.org>, <tj@kernel.org>, <dennis@kernel.org>, <cl@linux.com> CC: <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>, <mawupeng1@huawei.com> Subject: [PATCH -next 1/1] percpu: cleanup invalid assignment to err in pcpu_alloc Date: Sun, 4 Dec 2022 11:14:30 +0800 Message-ID: <20221204031430.662169-1-mawupeng1@huawei.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.175.112.125] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To dggpemm500014.china.huawei.com (7.185.36.153) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1751259902978920473?= X-GMAIL-MSGID: =?utf-8?q?1751259902978920473?= |
Series |
[-next,1/1] percpu: cleanup invalid assignment to err in pcpu_alloc
|
|
Commit Message
mawupeng
Dec. 4, 2022, 3:14 a.m. UTC
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(-)
Comments
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])) {