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:   Tue, 17 Jan 2023 16:35:00 -0800
From:   Minchan Kim <minchan@kernel.org>
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     Suren Baghdasaryan <surenb@google.com>,
        Matthew Wilcox <willy@infradead.org>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Michal Hocko <mhocko@suse.com>, SeongJae Park <sj@kernel.org>
Subject: Re: [PATCH 1/3] mm: return the number of pages successfully paged out
Message-ID: <Y8c+tFfwSTwlRFOk@google.com>
References: <20230117231632.2734737-1-minchan@kernel.org>
 <20230117155312.5464d01c2d639b1ac44f9330@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230117155312.5464d01c2d639b1ac44f9330@linux-foundation.org>
Precedence: bulk

On Tue, Jan 17, 2023 at 03:53:12PM -0800, Andrew Morton wrote:
> 
> I'm all hung up on the naming of everything.
> 
> > mm: return the number of pages successfully paged out
> 
> This is a vague title - MM is a big place.  Perhaps "mm/vmscan: ..."
> 
> On Tue, 17 Jan 2023 15:16:30 -0800 Minchan Kim <minchan@kernel.org> wrote:
> 
> > The reclaim_pages MADV_PAGEOUT uses needs to return the number of
> > pages paged-out successfully, not only the number of reclaimed pages
> > in the operation because those pages paged-out successfully will be
> > reclaimed easily at the memory pressure due to asynchronous writeback
> > rotation(i.e., PG_reclaim with folio_rotate_reclaimable).
> 
> So...  what does "paged out" actually mean?  "writeback to backing
> store was initiated"?  From an application's point of view it means "no
> longer in page tables needs a fault to get it back", no?

Yes, both are correct in my view since pageout is initiated after
unmapping the page from page table and think that's better wording
to be in description. Let me use the explanation in the description
at next spin. Thanks.

> 
> > This patch renames the reclaim_pages with paging_out(with hope that
> 
> "page_out" or "pageout" would be better than "paging_out".

pageout was taken from vmscan.c. Then I will use the page_out unless
some suggests better naming.

> 
> > it's clear from operation point of view) and then adds a additional
> > stat in reclaim_stat to represent the number of paged-out but kept
> > in the memory for rotation on writeback completion.
> 
> So it's the number of pages against which we have initiated writeback. 
> Why not call it "nr_writeback" or similar?

Currently, nr_writeback is used to indicate how many times shrink_folio_list
encoutered PG_writeback in the page list.

TLDR: I need to distinguish syncronous writeback and asynchronous
writeback.

Actually, I wanted to use nr_pageout but it would make double counting
from madvise_pageout PoV depending on backing storage's speed.
(For example, madvise_pageout tried 32 pages and 12 pages were swapped
out very quickly so those 12 pages were reclaimed under shrink_folio_list
context so it returns 12. But the other 20 pages were swapped out slowly
due to the device was congested so they were rotated back when the write
was done. In the case, madvise_pageout want to get 32 as successful paging
out's result rather than only 12 pages)

Maybe, nr_pageout_async?

> 
> > With that stat, madvise_pageout can know how many pages were paged-out
> > successfully as well as reclaimed. The return value will be used for
> > statistics in next patch.
> > 
> > ...
> >
> > -unsigned long reclaim_pages(struct list_head *folio_list)
> > +/*
> > + * paging_out - reclaim clean pages and write dirty pages into storage
> > + * @folio_list: pages for paging out
> > + *
> > + * paging_out() writes dirty pages to backing storage and/or reclaim
> > + * clean pages from memory. Returns the number of written/reclaimed pages.
> 
> s/reclaim/reclaims/
> 
> "and/or" it vague - just "or", I think.

Since the page list could have immediate reclaimable pages(A) when backing storage
is enough fast and just written pages(B) when the backing storage is slowed
down, even A + B if the device is congested in the middle of doing
operation, I think "and/or" is right.

> 
> "written/reclaimed" is vague.  "number of reclaimed pages plus the
> number of pages against which writeback was initiated" is precise.

Sure, let me correct it.

Thanks, Andrew.