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;
Sender: Minchan Kim <minchan.kim@gmail.com>
Date:   Wed, 4 May 2022 23:11:04 -0700
From:   Minchan Kim <minchan@kernel.org>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-mm <linux-mm@kvack.org>,
        Suren Baghdasaryan <surenb@google.com>,
        Michal Hocko <mhocko@suse.com>,
        John Dias <joaodias@google.com>,
        Tim Murray <timmurray@google.com>,
        Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH] mm: don't be stuck to rmap lock on reclaim path
Message-ID: <YnNqeB5tUf6LZ57b@google.com>
References: <20220503170341.1413961-1-minchan@kernel.org>
 <YnHzvV2Uz2ynENnG@casper.infradead.org>
 <YnIBbjRYACzvuZpp@google.com>
 <YnIYofrw/GGEvc0U@casper.infradead.org>
 <YnKhLX+jzJc+2KwB@google.com>
 <YnKwBeRAWwIlEVqy@casper.infradead.org>
 <YnMQp/zJVHu9Qq/S@google.com>
 <YnMdaW67GEDF0Ynr@casper.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YnMdaW67GEDF0Ynr@casper.infradead.org>
Precedence: bulk

On Thu, May 05, 2022 at 01:42:17AM +0100, Matthew Wilcox wrote:
> On Wed, May 04, 2022 at 04:47:51PM -0700, Minchan Kim wrote:
> > Since this patch introduces a new "contended" field as out param
> > in rmap_walk_control, it's not immutable any longer so remove
> > const keywords on rmap related functions. Since rmap walking
> > is already expensive operation, I doubt the const would help sizable
> > benefit(And we didn't have it until 5.17).
> 
> Um?  If it's now mutable, it surely can't be static as that means it
> would be shared by all callers, and you might write to the new fields
> in one caller and have them interpreted by the other caller!
> 
> Or if it is safe, then the comment needs to not be deleted, but modified
> to explain why it's safe to do so in this instance, and that other
> instances should not copy the approach unless they are similarly safe.

It's safe since rwc.contended is used only when rwc.try_lock is true.

> 
> > diff --git a/mm/page_idle.c b/mm/page_idle.c
> > index fc0435abf909..fdff8c6dcd2d 100644
> > --- a/mm/page_idle.c
> > +++ b/mm/page_idle.c
> > @@ -86,11 +86,8 @@ static bool page_idle_clear_pte_refs_one(struct folio *folio,
> >  static void page_idle_clear_pte_refs(struct page *page)
> >  {
> >  	struct folio *folio = page_folio(page);
> > -	/*
> > -	 * Since rwc.arg is unused, rwc is effectively immutable, so we
> > -	 * can make it static const to save some cycles and stack.
> > -	 */
> > -	static const struct rmap_walk_control rwc = {
> > +
> > +	static struct rmap_walk_control rwc = {
> >  		.rmap_one = page_idle_clear_pte_refs_one,
> >  		.anon_lock = folio_lock_anon_vma_read,
> >  	};

So, delta is

diff --git a/mm/page_idle.c b/mm/page_idle.c
index fdff8c6dcd2d..bc08332a609c 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -87,6 +87,10 @@ static void page_idle_clear_pte_refs(struct page *page)
 {
        struct folio *folio = page_folio(page);

+       /*
+        * Since rwc.try_lock is unused, rwc is effectively immutable, so we
+        * can make it static to save some cycles and stack.
+        */
        static struct rmap_walk_control rwc = {
                .rmap_one = page_idle_clear_pte_refs_one,
                .anon_lock = folio_lock_anon_vma_read,