On 5/10/22 14:17, Minchan Kim wrote:
Pages on CMA area could have MIGRATE_ISOLATE as well as MIGRATE_CMA
so current is_pinnable_page could miss CMA pages which has MIGRATE_
ISOLATE. It ends up pinning CMA pages as longterm at pin_user_pages
APIs so CMA allocation keep failed until the pin is released.
CPU 0 CPU 1 - Task B
cma_alloc
alloc_contig_range
pin_user_pages_fast(FOLL_LONGTERM)
change pageblock as MIGRATE_ISOLATE
internal_get_user_pages_fast
lockless_pages_from_mm
gup_pte_range
try_grab_folio
is_pinnable_page
return true;
So, pinned the page successfully.
page migration failure with pinned page
..
.. After 30 sec
unpin_user_page(page)
CMA allocation succeeded after 30 sec.
Hi Minchan,
Thanks for spelling out how this works, that really speeds up the
review and helps others quickly learn what is going on with the code.
For my own information, mainly: where is CMA blocking, so that
it waits (apparently) for the during of the pin, before proceeding?
(Or is the caller retrying?)
I noticed a few minor points but was too slow to reply, notes below:
The CMA allocation path protects the migration type change race
using zone->lock but what GUP path need to know is just whether the
page is on CMA area or not rather than exact migration type.
Thus, we don't need zone->lock but just checks migration type in
either of (MIGRATE_ISOLATE and MIGRATE_CMA).
Adding the MIGRATE_ISOLATE check in is_pinnable_page could cause
rejecting of pinning pages on MIGRATE_ISOLATE pageblocks even
though it's neither CMA nor movable zone if the page is temporarily
unmovable. However, such a migration failure by unexpected temporal
refcount holding is general issue, not only come from MIGRATE_ISOLATE
and the MIGRATE_ISOLATE is also transient state like other temporal
elevated refcount problem.
Cc: David Hildenbrand <david@xxxxxxxxxx>
Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
---
* from v3 - https://lore.kernel.org/all/20220509153430.4125710-1-minchan@xxxxxxxxxx/
* Fix typo and adding more description - akpm
* from v2 - https://lore.kernel.org/all/20220505064429.2818496-1-minchan@xxxxxxxxxx/
* Use __READ_ONCE instead of volatile - akpm
* from v1 - https://lore.kernel.org/all/20220502173558.2510641-1-minchan@xxxxxxxxxx/
* fix build warning - lkp
* fix refetching issue of migration type
* add side effect on !ZONE_MOVABLE and !MIGRATE_CMA in description - david
include/linux/mm.h | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6acca5cecbc5..cbf79eb790e0 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1625,8 +1625,19 @@ 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));
+#ifdef CONFIG_CMA
+ /*
+ * use volatile to use local variable mt instead of
+ * refetching mt value.
+ */
This comment is stale and should therefore be deleted.
+ int __mt = get_pageblock_migratetype(page);
+ int mt = __READ_ONCE(__mt);
Although I saw the email discussion about this in v2, that discussion
didn't go far enough. It started with "don't use volatile", and went
on to "try __READ_ONCE() instead", but it should have continued on
to "you don't need this at all".
Because you don't. There is nothing you are racing with, and adding
__READ_ONCE() in order to avoid a completely not-going-to-happen
compiler re-invocation of a significant code block is just very wrong.
So let's just let it go entirely. :)
+
+ if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE)
MIGRATE_ISOLATE is not always defined, and must therefore be protected
with a check on CONFIG_MEMORY_ISOLATION...oh never mind, I see in
mm/Kconfig:
config CMA
...
select MEMORY_ISOLATION
...so that's OK. What a tangled web, I wonder if enum migratetype
really needs to be sliced up like that, but that's a separate topic.
+ return false;
+#endif
+
+ return !(is_zone_movable_page(page) || is_zero_pfn(page_to_pfn(page)));
And actually this area is getting rather nested with the various ifdefs,
and it is probably time to refactor them a bit, considering the above
point about MIGRATE_ISOLATE. I had something in mind (which is why I
delayed my feedback), along the lines of merging _ISOLATE and _CMA and
the ifdefs. But it's just a fine point and not critical of course, just
a thought.
}
#else
static inline bool is_pinnable_page(struct page *page)
thanks,
--
John Hubbard
NVIDIA
