From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.1 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 66602C4363A for ; Mon, 5 Oct 2020 06:58:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 19D542080C for ; Mon, 5 Oct 2020 06:58:58 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="mJKhyuUp" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725912AbgJEG65 (ORCPT ); Mon, 5 Oct 2020 02:58:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54260 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725869AbgJEG65 (ORCPT ); Mon, 5 Oct 2020 02:58:57 -0400 Received: from mail-ed1-x543.google.com (mail-ed1-x543.google.com [IPv6:2a00:1450:4864:20::543]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EE0F0C0613CE; Sun, 4 Oct 2020 23:58:56 -0700 (PDT) Received: by mail-ed1-x543.google.com with SMTP id j2so8062443eds.9; Sun, 04 Oct 2020 23:58:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:date:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=gRVfMn1y3J1C+vGuMZF2LipjcOOXpjH1BPs85qkeY9I=; b=mJKhyuUpK1czGMhr3NIrmOT1aqCSpfTCVHRLKrrKneUrmoSvr9uQMBqETyYeNoea+U IeK0JjiPdhyJxJetWUGZ5zAqvuNnkEP584WnevYsaklihWJVxY+3h9kS7nHTHc+P8PHp vYxA/ZSzphHvZwIQzVLyMypYaWBL2sVQdIP2Ugd1nZSflP8DnoqvJ/s20hjdy5Yt94L4 Sh/jrVpidiEAS4OPFTM1SfwILy75ZxYh+W6Xo/dCTS/rqLAKv6EmJ+lTedfOJ7gBnVaR D+l2Yr5RUEpVAoAGnLy0UhmFRK6lVzXqZS4X3740EQAKkVn6Q+yNzH5OcVje6JdQHbEr A7bA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version; bh=gRVfMn1y3J1C+vGuMZF2LipjcOOXpjH1BPs85qkeY9I=; b=c4rV2e/jwenD7Bg+BAzsYTINNooD0JRv9vjs1FhRjtxoi++KWbjIwLmTE9p3A6fTVn 8NatEH5XkEIa/cPD1lRXXwcKpKJaEr9wZ6SlWZ026UyPwAJxC34OgrWzBjUim/ODRjFR 3H6PM9v0e9YR8sMXDdGvGusvTQK9jNSGmFcJQP+DUYXISSLAaE5+IlcYJD7tXLVn6D+0 dNh7A8GqtfBrmMCR1Zv22IJ17PiANLfuCHeozJ0zMrOVcpwel+7TaK3g+IZw9GcTgSh5 gpzcdrWQ6YQQfl1s0nVKB/3F8MgcHpBIPX/wNcTo6aQkHAw7lRjIyEwe5P12NQslFh15 r42w== X-Gm-Message-State: AOAM5336f1gn8uiegfwokYFYr7y/d/Fht+y8ba0SPQpnSxHzaICn2oth aPPP8EIb5U5Vq1USY4j31zI= X-Google-Smtp-Source: ABdhPJzoG44HkP0BJW9eFlGS3ah1NuoR/b/9sdRFfTA7f/mr7xp3yJ8sldhhlKCyJmpnepeMNEGJkQ== X-Received: by 2002:aa7:d690:: with SMTP id d16mr7880545edr.301.1601881135466; Sun, 04 Oct 2020 23:58:55 -0700 (PDT) Received: from felia ([2001:16b8:2dcc:7300:fc41:427:81ae:8ef0]) by smtp.gmail.com with ESMTPSA id a13sm200597edx.53.2020.10.04.23.58.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 04 Oct 2020 23:58:54 -0700 (PDT) From: Lukas Bulwahn X-Google-Original-From: Lukas Bulwahn Date: Mon, 5 Oct 2020 08:58:53 +0200 (CEST) X-X-Sender: lukas@felia To: Mel Gorman cc: Lukas Bulwahn , Andrew Morton , linux-mm@kvack.org, Vlastimil Babka , Michal Hocko , Nathan Chancellor , Nick Desaulniers , linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com, kernel-janitors@vger.kernel.org, linux-safety@lists.elisa.tech Subject: Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd() In-Reply-To: <20201004192437.GF3227@techsingularity.net> Message-ID: References: <20201004125827.17679-1-lukas.bulwahn@gmail.com> <20201004192437.GF3227@techsingularity.net> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 4 Oct 2020, Mel Gorman wrote: > On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote: > > The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent > > kswapd sleeping prematurely due to mismatched classzone_idx") turned an > > assignment to reclaim_order into a dead store, as in all further paths, > > reclaim_order will be assigned again before it is used. > > > > make clang-analyzer on x86_64 tinyconfig caught my attention with: > > > > mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is > > used in the enclosing expression, the value is never actually read from > > 'reclaim_order' [clang-analyzer-deadcode.DeadStores] > > > > Compilers will detect this unneeded assignment and optimize this anyway. > > So, the resulting binary is identical before and after this change. > > > > Simplify the code and remove unneeded assignment to make clang-analyzer > > happy. > > > > No functional change. No change in binary code. > > > > Signed-off-by: Lukas Bulwahn > > I'm not really keen on this. With the patch, reclaim_order can be passed > uninitialised to kswapd_try_to_sleep. While a sufficiently smart > compiler might be able to optimise how reclaim_order is used, it's not > guaranteed either. Similarly, a change in kswapd_try_to_sleep and its > called functions could rely on reclaim_order being a valid value and > then introduce a subtle bug. > Just for my own understanding: How would you see reclaim_order being passed unitialised to kswapd_try_to_sleep? >From kswapd() entry, any path must reach the line alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order); before kswap_try_to_sleep(...). Then it reads back the order into alloc_order and reclaim_order and resets pgdat->kswapd to 0. I argue that the second store to reclaim_order is not used. Path kthread_should_stop() is true: Then, it either exits and does not use those temporary values, reclaim_order and alloc_order, at all. Path try_to_freeze() is true: It goes back to the beginning of the loop and repeats reading alloc_order and reclaim_order after the reset to 0, and then passes that to kswapd_try_to_sleep(...). Previous reclaim_order is not used. So, the previous store to alloc_order and reclaim_order is lost. (Is that intentional?) Path try_to_freeze() is false: We call trace_mm_vmscan_kswapd_wake with alloc_order but not with reclaim_order. reclaim_order is set by the return of balance_pgdat(...); So, the previous reclaim_order is again not used. The diff in the patch might be a bit small, but we are looking at the second assignment after kswapd_try_to_sleep(...), not the first assignment that just looks the same. Lukas From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukas Bulwahn Date: Mon, 05 Oct 2020 06:58:53 +0000 Subject: Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd() Message-Id: List-Id: References: <20201004125827.17679-1-lukas.bulwahn@gmail.com> <20201004192437.GF3227@techsingularity.net> In-Reply-To: <20201004192437.GF3227@techsingularity.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Mel Gorman Cc: Lukas Bulwahn , Andrew Morton , linux-mm@kvack.org, Vlastimil Babka , Michal Hocko , Nathan Chancellor , Nick Desaulniers , linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com, kernel-janitors@vger.kernel.org, linux-safety@lists.elisa.tech On Sun, 4 Oct 2020, Mel Gorman wrote: > On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote: > > The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent > > kswapd sleeping prematurely due to mismatched classzone_idx") turned an > > assignment to reclaim_order into a dead store, as in all further paths, > > reclaim_order will be assigned again before it is used. > > > > make clang-analyzer on x86_64 tinyconfig caught my attention with: > > > > mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is > > used in the enclosing expression, the value is never actually read from > > 'reclaim_order' [clang-analyzer-deadcode.DeadStores] > > > > Compilers will detect this unneeded assignment and optimize this anyway. > > So, the resulting binary is identical before and after this change. > > > > Simplify the code and remove unneeded assignment to make clang-analyzer > > happy. > > > > No functional change. No change in binary code. > > > > Signed-off-by: Lukas Bulwahn > > I'm not really keen on this. With the patch, reclaim_order can be passed > uninitialised to kswapd_try_to_sleep. While a sufficiently smart > compiler might be able to optimise how reclaim_order is used, it's not > guaranteed either. Similarly, a change in kswapd_try_to_sleep and its > called functions could rely on reclaim_order being a valid value and > then introduce a subtle bug. > Just for my own understanding: How would you see reclaim_order being passed unitialised to kswapd_try_to_sleep? >From kswapd() entry, any path must reach the line alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order); before kswap_try_to_sleep(...). Then it reads back the order into alloc_order and reclaim_order and resets pgdat->kswapd to 0. I argue that the second store to reclaim_order is not used. Path kthread_should_stop() is true: Then, it either exits and does not use those temporary values, reclaim_order and alloc_order, at all. Path try_to_freeze() is true: It goes back to the beginning of the loop and repeats reading alloc_order and reclaim_order after the reset to 0, and then passes that to kswapd_try_to_sleep(...). Previous reclaim_order is not used. So, the previous store to alloc_order and reclaim_order is lost. (Is that intentional?) Path try_to_freeze() is false: We call trace_mm_vmscan_kswapd_wake with alloc_order but not with reclaim_order. reclaim_order is set by the return of balance_pgdat(...); So, the previous reclaim_order is again not used. The diff in the patch might be a bit small, but we are looking at the second assignment after kswapd_try_to_sleep(...), not the first assignment that just looks the same. Lukas From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.1 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 81678C4727D for ; Mon, 5 Oct 2020 06:58:59 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E2D2A2078A for ; Mon, 5 Oct 2020 06:58:58 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="mJKhyuUp" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E2D2A2078A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 069476B005D; Mon, 5 Oct 2020 02:58:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F356F6B0062; Mon, 5 Oct 2020 02:58:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DFE0D6B0068; Mon, 5 Oct 2020 02:58:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0037.hostedemail.com [216.40.44.37]) by kanga.kvack.org (Postfix) with ESMTP id AD1016B005D for ; Mon, 5 Oct 2020 02:58:57 -0400 (EDT) Received: from smtpin23.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 53E648249980 for ; Mon, 5 Oct 2020 06:58:57 +0000 (UTC) X-FDA: 77336969514.23.floor94_600486f271bc Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin23.hostedemail.com (Postfix) with ESMTP id 3323237608 for ; Mon, 5 Oct 2020 06:58:57 +0000 (UTC) X-HE-Tag: floor94_600486f271bc X-Filterd-Recvd-Size: 6194 Received: from mail-ed1-f68.google.com (mail-ed1-f68.google.com [209.85.208.68]) by imf47.hostedemail.com (Postfix) with ESMTP for ; Mon, 5 Oct 2020 06:58:56 +0000 (UTC) Received: by mail-ed1-f68.google.com with SMTP id g4so8073887edk.0 for ; Sun, 04 Oct 2020 23:58:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:date:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=gRVfMn1y3J1C+vGuMZF2LipjcOOXpjH1BPs85qkeY9I=; b=mJKhyuUpK1czGMhr3NIrmOT1aqCSpfTCVHRLKrrKneUrmoSvr9uQMBqETyYeNoea+U IeK0JjiPdhyJxJetWUGZ5zAqvuNnkEP584WnevYsaklihWJVxY+3h9kS7nHTHc+P8PHp vYxA/ZSzphHvZwIQzVLyMypYaWBL2sVQdIP2Ugd1nZSflP8DnoqvJ/s20hjdy5Yt94L4 Sh/jrVpidiEAS4OPFTM1SfwILy75ZxYh+W6Xo/dCTS/rqLAKv6EmJ+lTedfOJ7gBnVaR D+l2Yr5RUEpVAoAGnLy0UhmFRK6lVzXqZS4X3740EQAKkVn6Q+yNzH5OcVje6JdQHbEr A7bA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version; bh=gRVfMn1y3J1C+vGuMZF2LipjcOOXpjH1BPs85qkeY9I=; b=dkBqNogtTUCDIUYx42po5TK+r/oLkhfuivdPi4IbeP58UCDCx3aEgu7opbge4Asv1Q JGNKwM7VwQV+xi05klsekmxfwrXXana4ZyvSPi9vXKrlDs+9IgSp+f142xuI0gV4Nqa6 TmM6Faiv8JGEhhr0m6uVrz1mg71SnHLPhKwVbtZw6k5DbA4TrjSsMxeNkfgj5hi8+D1E lAmw0cvNfTPSMqv5zHbk0xuHqNaZ6y8VAVOGxJeueFj5bfQCtis1cXXf+oTL9/TVUA2j +PgUpod2de7D1VMBGQh5/eP8dj45ZZsr3NVCbxU8nZkXViy2B3oyVbHgklLjB3PKAbSE pejQ== X-Gm-Message-State: AOAM530/4nl62fofjJmAr+BVhuiOG65gAXT6N95RmooymugbJewtzKGW WZlWDOCtRWlRjSHv+1Lnd5Y= X-Google-Smtp-Source: ABdhPJzoG44HkP0BJW9eFlGS3ah1NuoR/b/9sdRFfTA7f/mr7xp3yJ8sldhhlKCyJmpnepeMNEGJkQ== X-Received: by 2002:aa7:d690:: with SMTP id d16mr7880545edr.301.1601881135466; Sun, 04 Oct 2020 23:58:55 -0700 (PDT) Received: from felia ([2001:16b8:2dcc:7300:fc41:427:81ae:8ef0]) by smtp.gmail.com with ESMTPSA id a13sm200597edx.53.2020.10.04.23.58.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 04 Oct 2020 23:58:54 -0700 (PDT) From: Lukas Bulwahn X-Google-Original-From: Lukas Bulwahn Date: Mon, 5 Oct 2020 08:58:53 +0200 (CEST) X-X-Sender: lukas@felia To: Mel Gorman cc: Lukas Bulwahn , Andrew Morton , linux-mm@kvack.org, Vlastimil Babka , Michal Hocko , Nathan Chancellor , Nick Desaulniers , linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com, kernel-janitors@vger.kernel.org, linux-safety@lists.elisa.tech Subject: Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd() In-Reply-To: <20201004192437.GF3227@techsingularity.net> Message-ID: References: <20201004125827.17679-1-lukas.bulwahn@gmail.com> <20201004192437.GF3227@techsingularity.net> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Sun, 4 Oct 2020, Mel Gorman wrote: > On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote: > > The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent > > kswapd sleeping prematurely due to mismatched classzone_idx") turned an > > assignment to reclaim_order into a dead store, as in all further paths, > > reclaim_order will be assigned again before it is used. > > > > make clang-analyzer on x86_64 tinyconfig caught my attention with: > > > > mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is > > used in the enclosing expression, the value is never actually read from > > 'reclaim_order' [clang-analyzer-deadcode.DeadStores] > > > > Compilers will detect this unneeded assignment and optimize this anyway. > > So, the resulting binary is identical before and after this change. > > > > Simplify the code and remove unneeded assignment to make clang-analyzer > > happy. > > > > No functional change. No change in binary code. > > > > Signed-off-by: Lukas Bulwahn > > I'm not really keen on this. With the patch, reclaim_order can be passed > uninitialised to kswapd_try_to_sleep. While a sufficiently smart > compiler might be able to optimise how reclaim_order is used, it's not > guaranteed either. Similarly, a change in kswapd_try_to_sleep and its > called functions could rely on reclaim_order being a valid value and > then introduce a subtle bug. > Just for my own understanding: How would you see reclaim_order being passed unitialised to kswapd_try_to_sleep? >From kswapd() entry, any path must reach the line alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order); before kswap_try_to_sleep(...). Then it reads back the order into alloc_order and reclaim_order and resets pgdat->kswapd to 0. I argue that the second store to reclaim_order is not used. Path kthread_should_stop() is true: Then, it either exits and does not use those temporary values, reclaim_order and alloc_order, at all. Path try_to_freeze() is true: It goes back to the beginning of the loop and repeats reading alloc_order and reclaim_order after the reset to 0, and then passes that to kswapd_try_to_sleep(...). Previous reclaim_order is not used. So, the previous store to alloc_order and reclaim_order is lost. (Is that intentional?) Path try_to_freeze() is false: We call trace_mm_vmscan_kswapd_wake with alloc_order but not with reclaim_order. reclaim_order is set by the return of balance_pgdat(...); So, the previous reclaim_order is again not used. The diff in the patch might be a bit small, but we are looking at the second assignment after kswapd_try_to_sleep(...), not the first assignment that just looks the same. Lukas