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, 3 May 2022 11:12:45 -0700
From:   Minchan Kim <minchan@kernel.org>
To:     David Hildenbrand <david@redhat.com>,
        "Paul E. McKenney" <paulmck@kernel.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        John Hubbard <jhubbard@nvidia.com>,
        John Dias <joaodias@google.com>
Subject: Re: [PATCH] mm: fix is_pinnable_page against on cma page
Message-ID: <YnFwnToAUDBu1vKv@google.com>
References: <20220502173558.2510641-1-minchan@kernel.org>
 <29d0c1c3-a44e-4573-7e7e-32be07544dbe@redhat.com>
 <YnAhaRNjmIhtGUjk@google.com>
 <08e9855c-395d-f40c-de3d-1ec8b644bfe8@redhat.com>
 <YnFJss0doXGCmq3w@google.com>
 <da07530d-92ad-7aef-2f2b-d115f878ef76@redhat.com>
 <YnFkZmvW2thpIn8o@google.com>
 <f271ca5e-c573-1c48-35a7-b59e9f2e122e@redhat.com>
 <YnFvmc+eMoXvLCWf@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YnFvmc+eMoXvLCWf@google.com>
Precedence: bulk

On Tue, May 03, 2022 at 11:08:25AM -0700, Minchan Kim wrote:
< snip >

Ccing Paul really this time.

Attach original code for Paul.

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6acca5cecbc5..f59bbe3296e3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1625,8 +1625,10 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
 #ifdef CONFIG_MIGRATION
 static inline bool is_pinnable_page(struct page *page)
 {
-	return !(is_zone_movable_page(page) || is_migrate_cma_page(page)) ||
-		is_zero_pfn(page_to_pfn(page));
+	int mt = get_pageblock_migratetype(page);
+
+	return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
+		mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
 }
 #else
 static inline bool is_pinnable_page(struct page *page)
--
2.36.0.464.gb9c8b46e94-goog


> > >>>
> > >>> A thing to get some attention is whether we need READ_ONCE or not
> > >>> for the local variable mt.
> > >>>
> > >>
> > >> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
> > >> there is anything stopping the compiler from re-reading the value. But
> > >> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
> > >> something in between.
> > > 
> > > How about this?
> > > 
> > >      CPU A                                                      CPU B
> > > 
> > > is_pinnable_page
> > >   ..
> > >   ..                                                set_pageblock_migratetype(MIGRATE_ISOLATE)
> > >   mt == MIGRATE_CMA
> > >     get_pageblock_miratetype(page)
> > >         returns MIGRATE_ISOLATE
> > >   mt == MIGRATE_ISOLATE                             set_pageblock_migratetype(MIGRATE_CMA)
> > >     get_pageblock_miratetype(page)
> > >         returns MIGRATE_CMA
> > >  
> > > So both conditions fails to detect it.
> > 
> > I think you're right. That's nasty.
> 
> Ccing Paul to borrow expertise. :)
> 
> int mt = get_pageblock_migratetype(page);
> 
> if (mt == MIGRATE_CMA)
>   return true;
> 
> if (mt == MIGRATE_ISOLATE)
>   return true;
> 
> I'd like to keep use the local variable mt's value in folloing
> conditions checks instead of refetching the value from
> get_pageblock_migratetype.
> 
> What's the right way to achieve it?
> 
> Thanks in advance!

Paul, could you give any hint?