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;
Date:   Fri, 24 Jun 2022 10:09:09 -0700
From:   Zach O'Keefe <zokeefe@google.com>
To:     Miaohe Lin <linmiaohe@huawei.com>
Cc:     Muchun Song <songmuchun@bytedance.com>, akpm@linux-foundation.org,
        shy828301@gmail.com, willy@infradead.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 14/16] mm/huge_memory: fix comment of page_deferred_list
Message-ID: <YrXvtRSm0koKa89w@google.com>
References: <20220622170627.19786-1-linmiaohe@huawei.com>
 <20220622170627.19786-15-linmiaohe@huawei.com>
 <YrQVOcF4PirjGa/M@FVFYT0MHHV2J.usts.net>
 <0b1a1633-2e07-87df-b707-3058353f5609@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0b1a1633-2e07-87df-b707-3058353f5609@huawei.com>
Precedence: bulk

On 23 Jun 20:26, Miaohe Lin wrote:
> On 2022/6/23 15:24, Muchun Song wrote:
> > On Thu, Jun 23, 2022 at 01:06:25AM +0800, Miaohe Lin wrote:
> >> The current comment is confusing because if global or memcg deferred list
> >> in the second tail page is occupied by compound_head, why we still use
> >> page[2].deferred_list here? I think it wants to say that Global or memcg
> >> deferred list in the first tail page is occupied by compound_mapcount and
> >> compound_pincount so we use the second tail page's deferred_list instead.
> >>
> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> >> ---
> >>  include/linux/huge_mm.h | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >> index 12b297f9951d..2e8062b3417a 100644
> >> --- a/include/linux/huge_mm.h
> >> +++ b/include/linux/huge_mm.h
> >> @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void)
> >>  static inline struct list_head *page_deferred_list(struct page *page)
> >>  {
> >>  	/*
> >> -	 * Global or memcg deferred list in the second tail pages is
> >> -	 * occupied by compound_head.
> >> +	 * Global or memcg deferred list in the first tail page is
> >> +	 * occupied by compound_mapcount and compound_pincount.
> >>  	 */
> > 
> > The structure of "struct page" seems to have told us the information that
> > we resue the 2nd tail page to be used as deferred_list. I am not sure the
> 
> Yes, it does.
> 
> > value of those comments. Maybe better to remove them?
> 
> IMHO above comment tries to tell us why deferred list in the second tail page is used
> instead of first tail page. But it should be fine to remove the above comments as they
> don't seem to provide much info (thought I'm not really sure).
> 
> Thanks.
> 

Just a suggestion - feel free to disregard. Maybe we don't need to repeat the
comments in struct page, but maybe a "see organization of tail pages of compound
page in "struct page" definition" would at least point new people to where this
magic 2 comes from.  Maybe an obvious place to check after you're familiar with
overloading struct page data for compound pages - but IMO it's not obvious for
newcomers.

> > 
> > Thanks.
> > 
> >>  	return &page[2].deferred_list;
> >>  }
> >> -- 
> >> 2.23.0
> >>
> >>
> > .
> > 
>