Re: [PATCH] mm: don't be stuck to rmap lock on reclaim path

[Minchan Kim <minchan@kernel.org>]

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,