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: <20210601053143.1380078-1-ying.huang@intel.com>
 <YLYef3i2OGseGbsS@casper.infradead.org> <YLZLf7MI11rzGI1B@cmpxchg.org>
In-Reply-To: <YLZLf7MI11rzGI1B@cmpxchg.org>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 1 Jun 2021 17:18:54 -1000
Message-ID: <CAHk-=wjg66VL1=F74-YDndwN2FvfUDG30XMgG7USiNkmExDeZA@mail.gmail.com>
Subject: Re: [PATCH] mm: free idle swap cache page after COW
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     Matthew Wilcox <willy@infradead.org>,
        Huang Ying <ying.huang@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux-MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Peter Xu <peterx@redhat.com>, Hugh Dickins <hughd@google.com>,
        Mel Gorman <mgorman@suse.de>, Rik van Riel <riel@surriel.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Michal Hocko <mhocko@kernel.org>,
        Dave Hansen <dave.hansen@intel.com>,
        Tim Chen <tim.c.chen@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jun 1, 2021 at 5:00 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> It's free_page[s]_and_swap_cache() we should reconsider, IMO.
>
> free_swap_cache() makes for a clean API function that does one thing,
> and does it right. free_page_and_swap_cache() combines two independent
> operations, which has the habit of accumulating special case-handling
> for some callers that is unncessary overhead for others (Abstraction
> Inversion anti-pattern).

Agreed. That "free_page_and_swap_cache()" function is odd. Much better
written as

        free_swap_cache();
        free_page();

because there's no real advantage to try to merge the two.

It's not like the merged function can actually do any optimization
based on the merging, it just does the above anyway - except it then
has that extra odd huge_zero_page test that makes no sense
what-so-ever.

So if anything we should try to get rid of uses of that odd
free_page_and_swap_cache() thing. But it's not exactly urgent.

             Linus
             Linus