Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200818184704.3625199-1-yuzhao@google.com> <20200818184704.3625199-3-yuzhao@google.com>
In-Reply-To: <20200818184704.3625199-3-yuzhao@google.com>
From:   Yang Shi <shy828301@gmail.com>
Date:   Wed, 19 Aug 2020 16:06:32 -0700
Message-ID: <CAHbLzkr7oPFtUog1zJWs54dsS8dhwkWp6ET_Zk71nXmRMtGvDQ@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] mm: remove superfluous __ClearPageWaiters()
To:     Yu Zhao <yuzhao@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Alexander Duyck <alexander.h.duyck@linux.intel.com>,
        Huang Ying <ying.huang@intel.com>,
        David Hildenbrand <david@redhat.com>,
        Michal Hocko <mhocko@suse.com>,
        Yang Shi <yang.shi@linux.alibaba.com>, Qian Cai <cai@lca.pw>,
        Mel Gorman <mgorman@suse.de>,
        Nicholas Piggin <npiggin@gmail.com>,
        =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= <jglisse@redhat.com>,
        Hugh Dickins <hughd@google.com>, Linux MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 18, 2020 at 11:47 AM Yu Zhao <yuzhao@google.com> wrote:
>
> Presumably __ClearPageWaiters() was added to follow the previously
> removed __ClearPageActive() pattern.
>
> Only flags that are in PAGE_FLAGS_CHECK_AT_FREE needs to be properly
> cleared because otherwise we think there may be some kind of leak.
> PG_waiters is not one of those flags and leaving the clearing to
> PAGE_FLAGS_CHECK_AT_PREP is more appropriate.

Actually TBH I'm not very keen to this change, it seems the clearing
is just moved around and the allocation side pays for that instead of
free side.

>
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  include/linux/page-flags.h | 2 +-
>  mm/filemap.c               | 2 ++
>  mm/memremap.c              | 2 --
>  mm/swap.c                  | 3 ---
>  4 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 6be1aa559b1e..dba80a2bdfba 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -318,7 +318,7 @@ static inline int TestClearPage##uname(struct page *page) { return 0; }
>         TESTSETFLAG_FALSE(uname) TESTCLEARFLAG_FALSE(uname)
>
>  __PAGEFLAG(Locked, locked, PF_NO_TAIL)
> -PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) __CLEARPAGEFLAG(Waiters, waiters, PF_ONLY_HEAD)
> +PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD)
>  PAGEFLAG(Error, error, PF_NO_TAIL) TESTCLEARFLAG(Error, error, PF_NO_TAIL)
>  PAGEFLAG(Referenced, referenced, PF_HEAD)
>         TESTCLEARFLAG(Referenced, referenced, PF_HEAD)
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1aaea26556cc..75240c7ef73f 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1079,6 +1079,8 @@ static void wake_up_page_bit(struct page *page, int bit_nr)
>                  * other pages on it.
>                  *
>                  * That's okay, it's a rare case. The next waker will clear it.
> +                * Otherwise the bit will be cleared by PAGE_FLAGS_CHECK_AT_PREP
> +                * when the page is being freed.
>                  */
>         }
>         spin_unlock_irqrestore(&q->lock, flags);
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 3a06eb91cb59..a9d02ffaf9e3 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -451,8 +451,6 @@ void free_devmap_managed_page(struct page *page)
>                 return;
>         }
>
> -       __ClearPageWaiters(page);
> -
>         mem_cgroup_uncharge(page);
>
>         /*
> diff --git a/mm/swap.c b/mm/swap.c
> index 999a84dbe12c..40bf20a75278 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -90,7 +90,6 @@ static void __page_cache_release(struct page *page)
>                 del_page_from_lru_list(page, lruvec, page_off_lru(page));
>                 spin_unlock_irqrestore(&pgdat->lru_lock, flags);
>         }
> -       __ClearPageWaiters(page);
>  }
>
>  static void __put_single_page(struct page *page)
> @@ -900,8 +899,6 @@ void release_pages(struct page **pages, int nr)
>                         del_page_from_lru_list(page, lruvec, page_off_lru(page));
>                 }
>
> -               __ClearPageWaiters(page);
> -
>                 list_add(&page->lru, &pages_to_free);
>         }
>         if (locked_pgdat)
> --
> 2.28.0.220.ged08abb693-goog
>
>