| Message ID | 20240817084941.2375713-2-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: memory_hotplug: improve do_migrate_range() | expand |
On Sat, 17 Aug 2024 16:49:37 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > Directly use a folio for HugeTLB and THP when calculate the next pfn, > then remove unused head variable. > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> Hi, I was curious so had a read and have a quick comment on the comment. > --- > mm/memory_hotplug.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index df291f2e509d..dc19b0e28fbc 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -1773,7 +1773,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end, > static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) > { > unsigned long pfn; > - struct page *page, *head; > + struct page *page; > LIST_HEAD(source); > static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL, > DEFAULT_RATELIMIT_BURST); > @@ -1786,14 +1786,20 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) > continue; > page = pfn_to_page(pfn); > folio = page_folio(page); > - head = &folio->page; > > - if (PageHuge(page)) { > - pfn = page_to_pfn(head) + compound_nr(head) - 1; > - isolate_hugetlb(folio, &source); > - continue; > - } else if (PageTransHuge(page)) > - pfn = page_to_pfn(head) + thp_nr_pages(page) - 1; > + /* > + * The folio hold no reference or lock, it might be changed > + * concurrently(eg, split), folio_nr_pages() may read garbage, > + * but out loop could handle that as it revisits the split > + * folio later. > + */ Perhaps reword this (it confused me a bit) I'm not sure I've retained the intended meaning! * No reference or lock is held on the folio, so it might * be modified concurrently (e.g. split). As such, * folio_nr_pages() may read garbage. This is fine as the * outer loop will revisit the split folio later. > + if (folio_test_large(folio)) { > + pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1; > + if (folio_test_hugetlb(folio)) { > + isolate_hugetlb(folio, &source); > + continue; > + } > + } > > /* > * HWPoison pages have elevated reference counts so the migration would
On 2024/8/19 17:28, Jonathan Cameron wrote: > On Sat, 17 Aug 2024 16:49:37 +0800 > Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Directly use a folio for HugeTLB and THP when calculate the next pfn, >> then remove unused head variable. >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > Hi, > > I was curious so had a read and have a quick comment > on the comment. > > >> --- >> mm/memory_hotplug.c | 22 ++++++++++++++-------- >> 1 file changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >> index df291f2e509d..dc19b0e28fbc 100644 >> --- a/mm/memory_hotplug.c >> +++ b/mm/memory_hotplug.c >> @@ -1773,7 +1773,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end, >> static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) >> { >> unsigned long pfn; >> - struct page *page, *head; >> + struct page *page; >> LIST_HEAD(source); >> static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL, >> DEFAULT_RATELIMIT_BURST); >> @@ -1786,14 +1786,20 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) >> continue; >> page = pfn_to_page(pfn); >> folio = page_folio(page); >> - head = &folio->page; >> >> - if (PageHuge(page)) { >> - pfn = page_to_pfn(head) + compound_nr(head) - 1; >> - isolate_hugetlb(folio, &source); >> - continue; >> - } else if (PageTransHuge(page)) >> - pfn = page_to_pfn(head) + thp_nr_pages(page) - 1; >> + /* >> + * The folio hold no reference or lock, it might be changed >> + * concurrently(eg, split), folio_nr_pages() may read garbage, >> + * but out loop could handle that as it revisits the split >> + * folio later. >> + */ > Perhaps reword this (it confused me a bit) I'm not sure I've retained > the intended meaning! > > * No reference or lock is held on the folio, so it might > * be modified concurrently (e.g. split). As such, > * folio_nr_pages() may read garbage. This is fine as the > * outer loop will revisit the split folio later. Thanks. hope Andrew could squash it if no more comments about this patchset, or I will resend a new version with comment changes. > > >> + if (folio_test_large(folio)) { >> + pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1; >> + if (folio_test_hugetlb(folio)) { >> + isolate_hugetlb(folio, &source); >> + continue; >> + } >> + } >> >> /* >> * HWPoison pages have elevated reference counts so the migration would >
On 2024/8/19 17:28, Jonathan Cameron wrote: > On Sat, 17 Aug 2024 16:49:37 +0800 > Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Directly use a folio for HugeTLB and THP when calculate the next pfn, >> then remove unused head variable. >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > Hi, > > I was curious so had a read and have a quick comment > on the comment. > > >> --- >> mm/memory_hotplug.c | 22 ++++++++++++++-------- >> 1 file changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >> index df291f2e509d..dc19b0e28fbc 100644 >> --- a/mm/memory_hotplug.c >> +++ b/mm/memory_hotplug.c >> @@ -1773,7 +1773,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end, >> static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) >> { >> unsigned long pfn; >> - struct page *page, *head; >> + struct page *page; >> LIST_HEAD(source); >> static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL, >> DEFAULT_RATELIMIT_BURST); >> @@ -1786,14 +1786,20 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) >> continue; >> page = pfn_to_page(pfn); >> folio = page_folio(page); >> - head = &folio->page; >> >> - if (PageHuge(page)) { >> - pfn = page_to_pfn(head) + compound_nr(head) - 1; >> - isolate_hugetlb(folio, &source); >> - continue; >> - } else if (PageTransHuge(page)) >> - pfn = page_to_pfn(head) + thp_nr_pages(page) - 1; >> + /* >> + * The folio hold no reference or lock, it might be changed >> + * concurrently(eg, split), folio_nr_pages() may read garbage, >> + * but out loop could handle that as it revisits the split >> + * folio later. >> + */ > Perhaps reword this (it confused me a bit) I'm not sure I've retained > the intended meaning! > > * No reference or lock is held on the folio, so it might > * be modified concurrently (e.g. split). As such, > * folio_nr_pages() may read garbage. This is fine as the > * outer loop will revisit the split folio later. With above comment changed, this patch looks good to me. Reviewed-by: Miaohe Lin <linmiaohe@huawei.com> Thanks. .
On 17.08.24 10:49, Kefeng Wang wrote: > Directly use a folio for HugeTLB and THP when calculate the next pfn, > then remove unused head variable. > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > mm/memory_hotplug.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index df291f2e509d..dc19b0e28fbc 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -1773,7 +1773,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end, > static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) > { > unsigned long pfn; > - struct page *page, *head; > + struct page *page; > LIST_HEAD(source); > static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL, > DEFAULT_RATELIMIT_BURST); > @@ -1786,14 +1786,20 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) > continue; > page = pfn_to_page(pfn); > folio = page_folio(page); > - head = &folio->page; > > - if (PageHuge(page)) { > - pfn = page_to_pfn(head) + compound_nr(head) - 1; > - isolate_hugetlb(folio, &source); > - continue; > - } else if (PageTransHuge(page)) > - pfn = page_to_pfn(head) + thp_nr_pages(page) - 1; > + /* > + * The folio hold no reference or lock, it might be changed > + * concurrently(eg, split), folio_nr_pages() may read garbage, > + * but out loop could handle that as it revisits the split > + * folio later. > + */ > + if (folio_test_large(folio)) { > + pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1; > + if (folio_test_hugetlb(folio)) { > + isolate_hugetlb(folio, &source); > + continue; > + } > + } > > /* > * HWPoison pages have elevated reference counts so the migration would With the comment cleanup Acked-by: David Hildenbrand <david@redhat.com>
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index df291f2e509d..dc19b0e28fbc 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1773,7 +1773,7 @@ static int scan_movable_pages(unsigned long start, unsigned long end, static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) { unsigned long pfn; - struct page *page, *head; + struct page *page; LIST_HEAD(source); static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); @@ -1786,14 +1786,20 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) continue; page = pfn_to_page(pfn); folio = page_folio(page); - head = &folio->page; - if (PageHuge(page)) { - pfn = page_to_pfn(head) + compound_nr(head) - 1; - isolate_hugetlb(folio, &source); - continue; - } else if (PageTransHuge(page)) - pfn = page_to_pfn(head) + thp_nr_pages(page) - 1; + /* + * The folio hold no reference or lock, it might be changed + * concurrently(eg, split), folio_nr_pages() may read garbage, + * but out loop could handle that as it revisits the split + * folio later. + */ + if (folio_test_large(folio)) { + pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1; + if (folio_test_hugetlb(folio)) { + isolate_hugetlb(folio, &source); + continue; + } + } /* * HWPoison pages have elevated reference counts so the migration would
Directly use a folio for HugeTLB and THP when calculate the next pfn, then remove unused head variable. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/memory_hotplug.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)