| Message ID | 20240826040132.1202297-3-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: convert to folio_isolate_movable() | expand |
Hi Kefeng, On 2024/8/26 12:01, Kefeng Wang wrote: > The tail page will always fail to isolate for non-lru-movable and > LRU page in isolate_migratepages_block(), so move PageTail check > ahead, then convert to use folio_isolate_movable() helper and more > folios. > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > mm/compaction.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index a2b16b08cbbf..aa2e8bb9fa58 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, > } > } > > + if (PageTail(page)) > + goto isolate_fail; > + > + folio = page_folio(page); I wonder if this is stable. Before page_folio(), it does not hold a reference on the page, so seems we should re-check the folio still contains this page after gaining a reference on the folio? > /* > * Check may be lockless but that's ok as we recheck later. > - * It's possible to migrate LRU and non-lru movable pages. > - * Skip any other type of page > + * It's possible to migrate LRU and non-lru movable folioss. > + * Skip any other type of folios. > */ > - if (!PageLRU(page)) { > + if (!folio_test_lru(folio)) { > /* > - * __PageMovable can return false positive so we need > - * to verify it under page_lock. > + * __folio_test_movable can return false positive so > + * we need to verify it under page_lock. > */ > - if (unlikely(__PageMovable(page)) && > - !PageIsolated(page)) { > + if (unlikely(__folio_test_movable(folio)) && > + !folio_test_isolated(folio)) { > if (locked) { > unlock_page_lruvec_irqrestore(locked, flags); > locked = NULL; > } > > - if (isolate_movable_page(page, mode)) { > - folio = page_folio(page); > + if (folio_isolate_movable(folio, mode)) > goto isolate_success; > - } > } > > goto isolate_fail; > } > > /* > - * Be careful not to clear PageLRU until after we're > - * sure the page is not being freed elsewhere -- the > - * page release code relies on it. > + * Be careful not to clear lru flag until after we're > + * sure the folio is not being freed elsewhere -- the > + * folio release code relies on it. > */ > - folio = folio_get_nontail_page(page); > - if (unlikely(!folio)) > + if (unlikely(folio_try_get(folio))) > goto isolate_fail; > > /*
On 2024/8/28 18:04, Baolin Wang wrote: > Hi Kefeng, > > On 2024/8/26 12:01, Kefeng Wang wrote: >> The tail page will always fail to isolate for non-lru-movable and >> LRU page in isolate_migratepages_block(), so move PageTail check >> ahead, then convert to use folio_isolate_movable() helper and more >> folios. >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> mm/compaction.c | 32 +++++++++++++++++--------------- >> 1 file changed, 17 insertions(+), 15 deletions(-) >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index a2b16b08cbbf..aa2e8bb9fa58 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct >> compact_control *cc, unsigned long low_pfn, >> } >> } >> + if (PageTail(page)) >> + goto isolate_fail; >> + >> + folio = page_folio(page); > > I wonder if this is stable. Before page_folio(), it does not hold a > reference on the page, so seems we should re-check the folio still > contains this page after gaining a reference on the folio? Oh, you are right, so two way to avoid this, 1) re-check 'page_folio(page) == folio', but this need change a little more. 2) directly use folio_get_nontail_page(page) here, and folio_put in the following path, this will try to get for any pages, but it should be accepted ? I'd prefer 2) but any other suggestion? Thanks. > >> /* >> * Check may be lockless but that's ok as we recheck later. >> - * It's possible to migrate LRU and non-lru movable pages. >> - * Skip any other type of page >> + * It's possible to migrate LRU and non-lru movable folioss. >> + * Skip any other type of folios. >> */ >> - if (!PageLRU(page)) { >> + if (!folio_test_lru(folio)) { >> /* >> - * __PageMovable can return false positive so we need >> - * to verify it under page_lock. >> + * __folio_test_movable can return false positive so >> + * we need to verify it under page_lock. >> */ >> - if (unlikely(__PageMovable(page)) && >> - !PageIsolated(page)) { >> + if (unlikely(__folio_test_movable(folio)) && >> + !folio_test_isolated(folio)) { >> if (locked) { >> unlock_page_lruvec_irqrestore(locked, flags); >> locked = NULL; >> } >> - if (isolate_movable_page(page, mode)) { >> - folio = page_folio(page); >> + if (folio_isolate_movable(folio, mode)) >> goto isolate_success; >> - } >> } >> goto isolate_fail; >> } >> /* >> - * Be careful not to clear PageLRU until after we're >> - * sure the page is not being freed elsewhere -- the >> - * page release code relies on it. >> + * Be careful not to clear lru flag until after we're >> + * sure the folio is not being freed elsewhere -- the >> + * folio release code relies on it. >> */ >> - folio = folio_get_nontail_page(page); >> - if (unlikely(!folio)) >> + if (unlikely(folio_try_get(folio))) >> goto isolate_fail; >> /*
On Wed, 28 Aug 2024 20:36:03 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > I wonder if this is stable. Before page_folio(), it does not hold a > > reference on the page, so seems we should re-check the folio still > > contains this page after gaining a reference on the folio? > > Oh, you are right, so two way to avoid this, > > 1) re-check 'page_folio(page) == folio', but this need change a little > more. > > 2) directly use folio_get_nontail_page(page) here, and folio_put in the > following path, this will try to get for any pages, but it should be > accepted ? > > I'd prefer 2) but any other suggestion? I dropped the v1 series.
On Wed, Aug 28, 2024 at 08:36:03PM +0800, Kefeng Wang wrote: > > > On 2024/8/28 18:04, Baolin Wang wrote: > > Hi Kefeng, > > > > On 2024/8/26 12:01, Kefeng Wang wrote: > > > The tail page will always fail to isolate for non-lru-movable and > > > LRU page in isolate_migratepages_block(), so move PageTail check > > > ahead, then convert to use folio_isolate_movable() helper and more > > > folios. > > > > > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > > --- > > > mm/compaction.c | 32 +++++++++++++++++--------------- > > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > > > diff --git a/mm/compaction.c b/mm/compaction.c > > > index a2b16b08cbbf..aa2e8bb9fa58 100644 > > > --- a/mm/compaction.c > > > +++ b/mm/compaction.c > > > @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct > > > compact_control *cc, unsigned long low_pfn, > > > } > > > } > > > + if (PageTail(page)) > > > + goto isolate_fail; > > > + > > > + folio = page_folio(page); > > > > I wonder if this is stable. Before page_folio(), it does not hold a > > reference on the page, so seems we should re-check the folio still > > contains this page after gaining a reference on the folio? > > Oh, you are right, so two way to avoid this, > > 1) re-check 'page_folio(page) == folio', but this need change a little > more. > > 2) directly use folio_get_nontail_page(page) here, and folio_put in the > following path, this will try to get for any pages, but it should be > accepted ? > > I'd prefer 2) but any other suggestion? I think option 2 makes sense, and simply use folio_put() on success and goto isolate_fail_put on failure instead of isolate_fail. With option 2, it might make sense to have folio_isolate_movable() expect to be passed a folio with elevated refcount. Then it could be treated similarly to its sister function folio_isolate_lru() and simply use folio_get() for its reference.
On 2024/8/29 07:37, Vishal Moola wrote: > On Wed, Aug 28, 2024 at 08:36:03PM +0800, Kefeng Wang wrote: >> >> >> On 2024/8/28 18:04, Baolin Wang wrote: >>> Hi Kefeng, >>> >>> On 2024/8/26 12:01, Kefeng Wang wrote: >>>> The tail page will always fail to isolate for non-lru-movable and >>>> LRU page in isolate_migratepages_block(), so move PageTail check >>>> ahead, then convert to use folio_isolate_movable() helper and more >>>> folios. >>>> >>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>>> --- >>>> mm/compaction.c | 32 +++++++++++++++++--------------- >>>> 1 file changed, 17 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/mm/compaction.c b/mm/compaction.c >>>> index a2b16b08cbbf..aa2e8bb9fa58 100644 >>>> --- a/mm/compaction.c >>>> +++ b/mm/compaction.c >>>> @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct >>>> compact_control *cc, unsigned long low_pfn, >>>> } >>>> } >>>> + if (PageTail(page)) >>>> + goto isolate_fail; >>>> + >>>> + folio = page_folio(page); >>> >>> I wonder if this is stable. Before page_folio(), it does not hold a >>> reference on the page, so seems we should re-check the folio still >>> contains this page after gaining a reference on the folio? >> >> Oh, you are right, so two way to avoid this, >> >> 1) re-check 'page_folio(page) == folio', but this need change a little >> more. >> >> 2) directly use folio_get_nontail_page(page) here, and folio_put in the >> following path, this will try to get for any pages, but it should be >> accepted ? >> >> I'd prefer 2) but any other suggestion? > > I think option 2 makes sense, and simply use folio_put() on success and > goto isolate_fail_put on failure instead of isolate_fail. > > With option 2, it might make sense to have folio_isolate_movable() > expect to be passed a folio with elevated refcount. Then it could be > treated similarly to its sister function folio_isolate_lru() and simply > use folio_get() for its reference. Yes, that makes sense to me too.
On 2024/8/29 9:11, Baolin Wang wrote: > > > On 2024/8/29 07:37, Vishal Moola wrote: >> On Wed, Aug 28, 2024 at 08:36:03PM +0800, Kefeng Wang wrote: >>> >>> >>> On 2024/8/28 18:04, Baolin Wang wrote: >>>> Hi Kefeng, >>>> >>>> On 2024/8/26 12:01, Kefeng Wang wrote: >>>>> The tail page will always fail to isolate for non-lru-movable and >>>>> LRU page in isolate_migratepages_block(), so move PageTail check >>>>> ahead, then convert to use folio_isolate_movable() helper and more >>>>> folios. >>>>> >>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>>>> --- >>>>> mm/compaction.c | 32 +++++++++++++++++--------------- >>>>> 1 file changed, 17 insertions(+), 15 deletions(-) >>>>> >>>>> diff --git a/mm/compaction.c b/mm/compaction.c >>>>> index a2b16b08cbbf..aa2e8bb9fa58 100644 >>>>> --- a/mm/compaction.c >>>>> +++ b/mm/compaction.c >>>>> @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct >>>>> compact_control *cc, unsigned long low_pfn, >>>>> } >>>>> } >>>>> + if (PageTail(page)) >>>>> + goto isolate_fail; >>>>> + >>>>> + folio = page_folio(page); >>>> >>>> I wonder if this is stable. Before page_folio(), it does not hold a >>>> reference on the page, so seems we should re-check the folio still >>>> contains this page after gaining a reference on the folio? >>> >>> Oh, you are right, so two way to avoid this, >>> >>> 1) re-check 'page_folio(page) == folio', but this need change a little >>> more. >>> >>> 2) directly use folio_get_nontail_page(page) here, and folio_put in the >>> following path, this will try to get for any pages, but it should be >>> accepted ? >>> >>> I'd prefer 2) but any other suggestion? >> >> I think option 2 makes sense, and simply use folio_put() on success and >> goto isolate_fail_put on failure instead of isolate_fail. >> >> With option 2, it might make sense to have folio_isolate_movable() >> expect to be passed a folio with elevated refcount. Then it could be >> treated similarly to its sister function folio_isolate_lru() and simply >> use folio_get() for its reference. > > Yes, that makes sense to me too. Thanks Vishal/Baolin, will refresh and send v2.
diff --git a/mm/compaction.c b/mm/compaction.c index a2b16b08cbbf..aa2e8bb9fa58 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -1074,39 +1074,41 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, } } + if (PageTail(page)) + goto isolate_fail; + + folio = page_folio(page); + /* * Check may be lockless but that's ok as we recheck later. - * It's possible to migrate LRU and non-lru movable pages. - * Skip any other type of page + * It's possible to migrate LRU and non-lru movable folioss. + * Skip any other type of folios. */ - if (!PageLRU(page)) { + if (!folio_test_lru(folio)) { /* - * __PageMovable can return false positive so we need - * to verify it under page_lock. + * __folio_test_movable can return false positive so + * we need to verify it under page_lock. */ - if (unlikely(__PageMovable(page)) && - !PageIsolated(page)) { + if (unlikely(__folio_test_movable(folio)) && + !folio_test_isolated(folio)) { if (locked) { unlock_page_lruvec_irqrestore(locked, flags); locked = NULL; } - if (isolate_movable_page(page, mode)) { - folio = page_folio(page); + if (folio_isolate_movable(folio, mode)) goto isolate_success; - } } goto isolate_fail; } /* - * Be careful not to clear PageLRU until after we're - * sure the page is not being freed elsewhere -- the - * page release code relies on it. + * Be careful not to clear lru flag until after we're + * sure the folio is not being freed elsewhere -- the + * folio release code relies on it. */ - folio = folio_get_nontail_page(page); - if (unlikely(!folio)) + if (unlikely(folio_try_get(folio))) goto isolate_fail; /*
The tail page will always fail to isolate for non-lru-movable and LRU page in isolate_migratepages_block(), so move PageTail check ahead, then convert to use folio_isolate_movable() helper and more folios. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/compaction.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)