| Message ID | 20220629035426.20013-2-alex.sierra@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add MEMORY_DEVICE_COHERENT for coherent device memory mapping | expand |
On 29.06.22 05:54, Alex Sierra wrote: > is_pinnable_page() and folio_is_pinnable() were renamed to > is_longterm_pinnable_page() and folio_is_longterm_pinnable() > respectively. These functions are used in the FOLL_LONGTERM flag > context. Subject talks about "*_pages" Can you elaborate why the move from mm.h to memremap.h is justified? I'd have called it "is_longterm_pinnable_page", but I am not a native speaker, so no strong opinion :)
On 2022-06-29 03:33, David Hildenbrand wrote: > On 29.06.22 05:54, Alex Sierra wrote: >> is_pinnable_page() and folio_is_pinnable() were renamed to >> is_longterm_pinnable_page() and folio_is_longterm_pinnable() >> respectively. These functions are used in the FOLL_LONGTERM flag >> context. > Subject talks about "*_pages" > > > Can you elaborate why the move from mm.h to memremap.h is justified? Patch 2 adds is_device_coherent_page in memremap.h and updates is_longterm_pinnable_page to call is_device_coherent_page. memremap.h cannot include mm.h because it is itself included by mm.h. So the choice was to move is_longterm_pinnable_page to memremap.h, or move is_device_coherent_page and all its dependencies to mm.h. The latter would have been a bigger change. > > I'd have called it "is_longterm_pinnable_page", but I am not a native > speaker, so no strong opinion :) I think only the patch title has the name backwards. The code uses is_longterm_pinnable_page. Regards, Felix > >
On 30.06.22 00:08, Felix Kuehling wrote: > On 2022-06-29 03:33, David Hildenbrand wrote: >> On 29.06.22 05:54, Alex Sierra wrote: >>> is_pinnable_page() and folio_is_pinnable() were renamed to >>> is_longterm_pinnable_page() and folio_is_longterm_pinnable() >>> respectively. These functions are used in the FOLL_LONGTERM flag >>> context. >> Subject talks about "*_pages" >> >> >> Can you elaborate why the move from mm.h to memremap.h is justified? > > Patch 2 adds is_device_coherent_page in memremap.h and updates > is_longterm_pinnable_page to call is_device_coherent_page. memremap.h > cannot include mm.h because it is itself included by mm.h. So the choice > was to move is_longterm_pinnable_page to memremap.h, or move > is_device_coherent_page and all its dependencies to mm.h. The latter > would have been a bigger change. I really don't think something mm generic that compiles without ZONE_DEVICE belongs into memremap.h. Please find a cleaner way to get this done.
On Wed, 29 Jun 2022 18:08:40 -0400 Felix Kuehling <felix.kuehling@amd.com> wrote: > > > > I'd have called it "is_longterm_pinnable_page", but I am not a native > > speaker, so no strong opinion :) > > I think only the patch title has the name backwards. The code uses > is_longterm_pinnable_page. Patch title was quite buggy ;) I made it "mm: rename is_pinnable_page() to is_longterm_pinnable_page()" in my copy.
On Thu, 30 Jun 2022 00:15:06 +0200 David Hildenbrand <david@redhat.com> wrote: > On 30.06.22 00:08, Felix Kuehling wrote: > > On 2022-06-29 03:33, David Hildenbrand wrote: > >> On 29.06.22 05:54, Alex Sierra wrote: > >>> is_pinnable_page() and folio_is_pinnable() were renamed to > >>> is_longterm_pinnable_page() and folio_is_longterm_pinnable() > >>> respectively. These functions are used in the FOLL_LONGTERM flag > >>> context. > >> Subject talks about "*_pages" > >> > >> > >> Can you elaborate why the move from mm.h to memremap.h is justified? > > > > Patch 2 adds is_device_coherent_page in memremap.h and updates > > is_longterm_pinnable_page to call is_device_coherent_page. memremap.h > > cannot include mm.h because it is itself included by mm.h. So the choice > > was to move is_longterm_pinnable_page to memremap.h, or move > > is_device_coherent_page and all its dependencies to mm.h. The latter > > would have been a bigger change. > > I really don't think something mm generic that compiles without > ZONE_DEVICE belongs into memremap.h. Please find a cleaner way to get > this done. (without having bothered to look at the code...) The solution is always to create a new purpose-specific header file.
diff --git a/include/linux/memremap.h b/include/linux/memremap.h index 8af304f6b504..c272bd0af3c1 100644 --- a/include/linux/memremap.h +++ b/include/linux/memremap.h @@ -150,6 +150,30 @@ static inline bool is_pci_p2pdma_page(const struct page *page) page->pgmap->type == MEMORY_DEVICE_PCI_P2PDMA; } +/* MIGRATE_CMA and ZONE_MOVABLE do not allow pin pages */ +#ifdef CONFIG_MIGRATION +static inline bool is_longterm_pinnable_page(struct page *page) +{ +#ifdef CONFIG_CMA + int mt = get_pageblock_migratetype(page); + + if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE) + return false; +#endif + return !(is_zone_movable_page(page) || + is_zero_pfn(page_to_pfn(page))); +} +#else +static inline bool is_longterm_pinnable_page(struct page *page) +{ + return true; +} +#endif +static inline bool folio_is_longterm_pinnable(struct folio *folio) +{ + return is_longterm_pinnable_page(&folio->page); +} + #ifdef CONFIG_ZONE_DEVICE void *memremap_pages(struct dev_pagemap *pgmap, int nid); void memunmap_pages(struct dev_pagemap *pgmap); diff --git a/include/linux/mm.h b/include/linux/mm.h index cf3d0d673f6b..bc0f201a4cff 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1590,30 +1590,6 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma, return page_maybe_dma_pinned(page); } -/* MIGRATE_CMA and ZONE_MOVABLE do not allow pin pages */ -#ifdef CONFIG_MIGRATION -static inline bool is_pinnable_page(struct page *page) -{ -#ifdef CONFIG_CMA - int mt = get_pageblock_migratetype(page); - - if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE) - return false; -#endif - return !is_zone_movable_page(page) || is_zero_pfn(page_to_pfn(page)); -} -#else -static inline bool is_pinnable_page(struct page *page) -{ - return true; -} -#endif - -static inline bool folio_is_pinnable(struct folio *folio) -{ - return is_pinnable_page(&folio->page); -} - static inline void set_page_zone(struct page *page, enum zone_type zone) { page->flags &= ~(ZONES_MASK << ZONES_PGSHIFT); diff --git a/mm/gup.c b/mm/gup.c index 551264407624..b65fe8bf5af4 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -133,7 +133,7 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags) * path. */ if (unlikely((flags & FOLL_LONGTERM) && - !is_pinnable_page(page))) + !is_longterm_pinnable_page(page))) return NULL; /* @@ -1891,7 +1891,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages, continue; prev_folio = folio; - if (folio_is_pinnable(folio)) + if (folio_is_longterm_pinnable(folio)) continue; /* diff --git a/mm/gup_test.c b/mm/gup_test.c index d974dec19e1c..9d705ba6737e 100644 --- a/mm/gup_test.c +++ b/mm/gup_test.c @@ -1,5 +1,5 @@ #include <linux/kernel.h> -#include <linux/mm.h> +#include <linux/memremap.h> #include <linux/slab.h> #include <linux/uaccess.h> #include <linux/ktime.h> @@ -53,7 +53,7 @@ static void verify_dma_pinned(unsigned int cmd, struct page **pages, dump_page(page, "gup_test failure"); break; } else if (cmd == PIN_LONGTERM_BENCHMARK && - WARN(!is_pinnable_page(page), + WARN(!is_longterm_pinnable_page(page), "pages[%lu] is NOT pinnable but pinned\n", i)) { dump_page(page, "gup_test failure"); diff --git a/mm/hugetlb.c b/mm/hugetlb.c index a57e1be41401..368fd33787b0 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -1135,7 +1135,7 @@ static struct page *dequeue_huge_page_node_exact(struct hstate *h, int nid) lockdep_assert_held(&hugetlb_lock); list_for_each_entry(page, &h->hugepage_freelists[nid], lru) { - if (pin && !is_pinnable_page(page)) + if (pin && !is_longterm_pinnable_page(page)) continue; if (PageHWPoison(page))
is_pinnable_page() and folio_is_pinnable() were renamed to is_longterm_pinnable_page() and folio_is_longterm_pinnable() respectively. These functions are used in the FOLL_LONGTERM flag context. Signed-off-by: Alex Sierra <alex.sierra@amd.com> --- include/linux/memremap.h | 24 ++++++++++++++++++++++++ include/linux/mm.h | 24 ------------------------ mm/gup.c | 4 ++-- mm/gup_test.c | 4 ++-- mm/hugetlb.c | 2 +- 5 files changed, 29 insertions(+), 29 deletions(-)