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=-12.9 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1, USER_IN_DEF_DKIM_WL autolearn=no 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 B0BA4C56201 for ; Thu, 12 Nov 2020 05:34:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5299320825 for ; Thu, 12 Nov 2020 05:34:22 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="iqee5mg0" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728695AbgKLFeV (ORCPT ); Thu, 12 Nov 2020 00:34:21 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56930 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726276AbgKLCDi (ORCPT ); Wed, 11 Nov 2020 21:03:38 -0500 Received: from mail-oi1-x244.google.com (mail-oi1-x244.google.com [IPv6:2607:f8b0:4864:20::244]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 27397C0613D1 for ; Wed, 11 Nov 2020 18:03:39 -0800 (PST) Received: by mail-oi1-x244.google.com with SMTP id d9so4605737oib.3 for ; Wed, 11 Nov 2020 18:03:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=KyT2mlBYlT2kAm/JHbWqMsOuBJM1d87BrqaaRaUfl50=; b=iqee5mg0JMbddEDyf2/xuBU1NZWnAWQDmcxpGfelMd2nKAPy/CPLgkG4iApYTGebQh yUBfhOvZXGiwgsEDXQxTtAr7ihzRkOkBe33dvPe2T9i4nrIsRa+nmWvkU6ri/CmLLHU+ 0k7mh1HRa+kWhDkkT1SWLyhPJ9t5KGqxKeLrVvIWgRoXmaWgvAMRjhxLbn1VpzsA+OFG EmxMxMa3aKoHuvgHtwDhMqy1TKQyoxAReTZAM95dC7u16k4wnNgEvA0A6tssToLXQzvq uWgRdxzDYElGfeIiIVf3cDvAOaYb+231CM6UI0zHdEqLILTIpfXcYLfiMfX75UJ9zujd kciw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version; bh=KyT2mlBYlT2kAm/JHbWqMsOuBJM1d87BrqaaRaUfl50=; b=FA7VEY7Qpf+jyxLnw7fUPHJqksSZkArmQiBEzJTWsygsnTCYgEXW38hQ71kaIkhYgK HhwBdsD2SqG9eF0zjsENMcxhBadG8YKNR8cXe/t4SZXx6iuZFjdK4KSMVaXYTGTfMviy TEXmnWbAa0iHYP8h3BauQ0jxbGySqZhu9idGb5cq4V/ewxAdj8lHnaeu13glxHt+C9WP jWGBx1/BzsIcHrXUnvALI47SJGa5S2lU6EaVF8KvaNpIPAvJmYC9eWIvs3T3v9xf9npc wXNmtv70L48JojalcLYYuOPqWnC5XdCNVM7oZKzn0t0HxPBr2qKJTbQdjRpuVaS+wisD MvoA== X-Gm-Message-State: AOAM531Yoyz76XNZRu7IjabjnY3v8amb0qklRWkBzUqQdIcRVMswQwL9 9TT0i7mjqbM5gPHp5+Syn5+3RXdqv9l/gQ== X-Google-Smtp-Source: ABdhPJx+VVz+pvUawrMnkgTlv6UoYdI6b3G5zOvDXUE/xLJ+9e8Kjh1DckUSTAeZd8F3PDjyrVp08w== X-Received: by 2002:aca:6004:: with SMTP id u4mr4357158oib.8.1605146618042; Wed, 11 Nov 2020 18:03:38 -0800 (PST) Received: from eggly.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id m10sm982866oon.27.2020.11.11.18.03.35 (version=TLS1 cipher=ECDHE-ECDSA-AES128-SHA bits=128/128); Wed, 11 Nov 2020 18:03:37 -0800 (PST) Date: Wed, 11 Nov 2020 18:03:22 -0800 (PST) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Vlastimil Babka cc: Alex Shi , akpm@linux-foundation.org, mgorman@techsingularity.net, tj@kernel.org, hughd@google.com, khlebnikov@yandex-team.ru, daniel.m.jordan@oracle.com, willy@infradead.org, hannes@cmpxchg.org, lkp@intel.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, shakeelb@google.com, iamjoonsoo.kim@lge.com, richard.weiyang@gmail.com, kirill@shutemov.name, alexander.duyck@gmail.com, rong.a.chen@intel.com, mhocko@suse.com, vdavydov.dev@gmail.com, shy828301@gmail.com, Michal Hocko Subject: Re: [PATCH v21 14/19] mm/lru: introduce TestClearPageLRU In-Reply-To: Message-ID: References: <1604566549-62481-1-git-send-email-alex.shi@linux.alibaba.com> <1604566549-62481-15-git-send-email-alex.shi@linux.alibaba.com> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 11 Nov 2020, Vlastimil Babka wrote: > On 11/5/20 9:55 AM, Alex Shi wrote: > > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -1542,7 +1542,7 @@ unsigned int reclaim_clean_pages_from_list(struct > > zone *zone, > > */ > > int __isolate_lru_page(struct page *page, isolate_mode_t mode) > > { > > - int ret = -EINVAL; > > + int ret = -EBUSY; > > /* Only take pages on the LRU. */ > > if (!PageLRU(page)) > > @@ -1552,8 +1552,6 @@ int __isolate_lru_page(struct page *page, > > isolate_mode_t mode) > > if (PageUnevictable(page) && !(mode & ISOLATE_UNEVICTABLE)) > > return ret; > > - ret = -EBUSY; > > I'm not sure why this change is here, looks unrelated to the patch? > > Oh I see, you want to prevent the BUG() in isolate_lru_pages(). Yes, I suggested this part of the patch to Alex, when I hit that BUG(). > > But due to that, the PageUnevictable check was also affected unintentionally. > But I don't think it's that important to BUG() when we run into > PageUnevictable unexpectedly, so that's probably ok. Not unintentional. __isolate_lru_page(), or __isolate_lru_page_prepare(), is a silly function, used by two callers whose requirements are almost entirely disjoint. The ISOLATE_UNEVICTABLE case is only for compaction.c, which takes no interest in -EINVAL versus -EBUSY, and has no such BUG(). I think it dates back to lumpy reclaim days, and it probably made more sense back then. > > But with that, we can just make __isolate_lru_page() a bool function and > remove the ugly switch in isolate_lru_pages()? I agree that the switch statement in isolate_lru_pages() seems pointless now, and can be turned into an if{}else{}. But that cleanup is a diversion from this particular TestClearPageLRU patch, and I think from the whole series (checking final state of the patchset, yes, the switch is still there - though I think there have been variant series which removed it). Can we please leave that cleanup until after the series has gone in? I think several of us have cleanups or optimization that we want to follow (I had one that inlines what isolate_migratepages_block() wanted of __isolate_lru_page() into that function, so simplifying what vmscan.c needs; perhaps that can now eliminate it completely, I've not tried recently). But there was a point at which the series was growing ten patches per release as we all added our bits and pieces on top, it got harder and harder to review the whole, and further from getting the basics in: I do push back against that tendency. Hugh From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hugh Dickins Subject: Re: [PATCH v21 14/19] mm/lru: introduce TestClearPageLRU Date: Wed, 11 Nov 2020 18:03:22 -0800 (PST) Message-ID: References: <1604566549-62481-1-git-send-email-alex.shi@linux.alibaba.com> <1604566549-62481-15-git-send-email-alex.shi@linux.alibaba.com> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=KyT2mlBYlT2kAm/JHbWqMsOuBJM1d87BrqaaRaUfl50=; b=iqee5mg0JMbddEDyf2/xuBU1NZWnAWQDmcxpGfelMd2nKAPy/CPLgkG4iApYTGebQh yUBfhOvZXGiwgsEDXQxTtAr7ihzRkOkBe33dvPe2T9i4nrIsRa+nmWvkU6ri/CmLLHU+ 0k7mh1HRa+kWhDkkT1SWLyhPJ9t5KGqxKeLrVvIWgRoXmaWgvAMRjhxLbn1VpzsA+OFG EmxMxMa3aKoHuvgHtwDhMqy1TKQyoxAReTZAM95dC7u16k4wnNgEvA0A6tssToLXQzvq uWgRdxzDYElGfeIiIVf3cDvAOaYb+231CM6UI0zHdEqLILTIpfXcYLfiMfX75UJ9zujd kciw== In-Reply-To: List-ID: Content-Type: TEXT/PLAIN; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Vlastimil Babka Cc: Alex Shi , akpm@linux-foundation.org, mgorman@techsingularity.net, tj@kernel.org, hughd@google.com, khlebnikov@yandex-team.ru, daniel.m.jordan@oracle.com, willy@infradead.org, hannes@cmpxchg.org, lkp@intel.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, shakeelb@google.com, iamjoonsoo.kim@lge.com, richard.weiyang@gmail.com, kirill@shutemov.name, alexander.duyck@gmail.com, rong.a.chen@intel.com, mhocko@suse.com, vdavydov.dev@gmail.com, shy828301@gmail.com, Michal Hocko On Wed, 11 Nov 2020, Vlastimil Babka wrote: > On 11/5/20 9:55 AM, Alex Shi wrote: > > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -1542,7 +1542,7 @@ unsigned int reclaim_clean_pages_from_list(struct > > zone *zone, > > */ > > int __isolate_lru_page(struct page *page, isolate_mode_t mode) > > { > > - int ret = -EINVAL; > > + int ret = -EBUSY; > > /* Only take pages on the LRU. */ > > if (!PageLRU(page)) > > @@ -1552,8 +1552,6 @@ int __isolate_lru_page(struct page *page, > > isolate_mode_t mode) > > if (PageUnevictable(page) && !(mode & ISOLATE_UNEVICTABLE)) > > return ret; > > - ret = -EBUSY; > > I'm not sure why this change is here, looks unrelated to the patch? > > Oh I see, you want to prevent the BUG() in isolate_lru_pages(). Yes, I suggested this part of the patch to Alex, when I hit that BUG(). > > But due to that, the PageUnevictable check was also affected unintentionally. > But I don't think it's that important to BUG() when we run into > PageUnevictable unexpectedly, so that's probably ok. Not unintentional. __isolate_lru_page(), or __isolate_lru_page_prepare(), is a silly function, used by two callers whose requirements are almost entirely disjoint. The ISOLATE_UNEVICTABLE case is only for compaction.c, which takes no interest in -EINVAL versus -EBUSY, and has no such BUG(). I think it dates back to lumpy reclaim days, and it probably made more sense back then. > > But with that, we can just make __isolate_lru_page() a bool function and > remove the ugly switch in isolate_lru_pages()? I agree that the switch statement in isolate_lru_pages() seems pointless now, and can be turned into an if{}else{}. But that cleanup is a diversion from this particular TestClearPageLRU patch, and I think from the whole series (checking final state of the patchset, yes, the switch is still there - though I think there have been variant series which removed it). Can we please leave that cleanup until after the series has gone in? I think several of us have cleanups or optimization that we want to follow (I had one that inlines what isolate_migratepages_block() wanted of __isolate_lru_page() into that function, so simplifying what vmscan.c needs; perhaps that can now eliminate it completely, I've not tried recently). But there was a point at which the series was growing ten patches per release as we all added our bits and pieces on top, it got harder and harder to review the whole, and further from getting the basics in: I do push back against that tendency. Hugh