* [PATCH v2 1/2] mm: page_alloc: simplify pageblock migratetype check in __free_one_page(). @ 2022-04-01 18:11 Zi Yan 2022-04-01 18:11 ` [PATCH v2 2/2] mm: wrap __find_buddy_pfn() with a necessary buddy page validation Zi Yan 2022-04-01 22:14 ` [PATCH v2 1/2] mm: page_alloc: simplify pageblock migratetype check in __free_one_page() Vlastimil Babka 0 siblings, 2 replies; 8+ messages in thread From: Zi Yan @ 2022-04-01 18:11 UTC (permalink / raw) To: linux-mm Cc: Linus Torvalds, Steven Rostedt, David Hildenbrand, Vlastimil Babka, Mel Gorman, Mike Rapoport, Oscar Salvador, Andrew Morton, linux-kernel, Zi Yan From: Zi Yan <ziy@nvidia.com> Move pageblock migratetype check code in the while loop to simplify the logic. It also saves redundant buddy page checking code. Suggested-by: Vlastimil Babka <vbabka@suse.cz> Link: https://lore.kernel.org/linux-mm/27ff69f9-60c5-9e59-feb2-295250077551@suse.cz/ Signed-off-by: Zi Yan <ziy@nvidia.com> --- mm/page_alloc.c | 46 +++++++++++++++++----------------------------- 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 856473e54155..2ea106146686 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -1054,7 +1054,6 @@ static inline void __free_one_page(struct page *page, int migratetype, fpi_t fpi_flags) { struct capture_control *capc = task_capc(zone); - unsigned int max_order = pageblock_order; unsigned long buddy_pfn; unsigned long combined_pfn; struct page *buddy; @@ -1070,8 +1069,7 @@ static inline void __free_one_page(struct page *page, VM_BUG_ON_PAGE(pfn & ((1 << order) - 1), page); VM_BUG_ON_PAGE(bad_range(zone, page), page); -continue_merging: - while (order < max_order) { + while (order < MAX_ORDER - 1) { if (compaction_capture(capc, page, order, migratetype)) { __mod_zone_freepage_state(zone, -(1 << order), migratetype); @@ -1082,6 +1080,22 @@ static inline void __free_one_page(struct page *page, if (!page_is_buddy(page, buddy, order)) goto done_merging; + + if (unlikely(order >= pageblock_order)) { + /* + * We want to prevent merge between freepages on pageblock + * without fallbacks and normal pageblock. Without this, + * pageblock isolation could cause incorrect freepage or CMA + * accounting or HIGHATOMIC accounting. + */ + int buddy_mt = get_pageblock_migratetype(buddy); + + if (migratetype != buddy_mt + && (!migratetype_is_mergeable(migratetype) || + !migratetype_is_mergeable(buddy_mt))) + goto done_merging; + } + /* * Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page, * merge with it and move up one order. @@ -1095,32 +1109,6 @@ static inline void __free_one_page(struct page *page, pfn = combined_pfn; order++; } - if (order < MAX_ORDER - 1) { - /* If we are here, it means order is >= pageblock_order. - * We want to prevent merge between freepages on pageblock - * without fallbacks and normal pageblock. Without this, - * pageblock isolation could cause incorrect freepage or CMA - * accounting or HIGHATOMIC accounting. - * - * We don't want to hit this code for the more frequent - * low-order merging. - */ - int buddy_mt; - - buddy_pfn = __find_buddy_pfn(pfn, order); - buddy = page + (buddy_pfn - pfn); - - if (!page_is_buddy(page, buddy, order)) - goto done_merging; - buddy_mt = get_pageblock_migratetype(buddy); - - if (migratetype != buddy_mt - && (!migratetype_is_mergeable(migratetype) || - !migratetype_is_mergeable(buddy_mt))) - goto done_merging; - max_order = order + 1; - goto continue_merging; - } done_merging: set_buddy_order(page, order); -- 2.35.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] mm: wrap __find_buddy_pfn() with a necessary buddy page validation. 2022-04-01 18:11 [PATCH v2 1/2] mm: page_alloc: simplify pageblock migratetype check in __free_one_page() Zi Yan @ 2022-04-01 18:11 ` Zi Yan 2022-04-01 18:29 ` Linus Torvalds 2022-04-01 22:14 ` [PATCH v2 1/2] mm: page_alloc: simplify pageblock migratetype check in __free_one_page() Vlastimil Babka 1 sibling, 1 reply; 8+ messages in thread From: Zi Yan @ 2022-04-01 18:11 UTC (permalink / raw) To: linux-mm Cc: Linus Torvalds, Steven Rostedt, David Hildenbrand, Vlastimil Babka, Mel Gorman, Mike Rapoport, Oscar Salvador, Andrew Morton, linux-kernel, Zi Yan From: Zi Yan <ziy@nvidia.com> Whenever the buddy of a page is found from __find_buddy_pfn(), page_is_buddy() should be used to check its validity. Add a helper function find_buddy_page_pfn() to find the buddy page and do the check together. Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Link: https://lore.kernel.org/linux-mm/CAHk-=wji_AmYygZMTsPMdJ7XksMt7kOur8oDfDdniBRMjm4VkQ@mail.gmail.com/ Signed-off-by: Zi Yan <ziy@nvidia.com> --- mm/internal.h | 23 +---------------- mm/page_alloc.c | 63 ++++++++++++++++++++++++++++++++++++++------- mm/page_isolation.c | 8 ++---- 3 files changed, 56 insertions(+), 38 deletions(-) diff --git a/mm/internal.h b/mm/internal.h index 876e66237c89..f43565ed5ff6 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -211,28 +211,7 @@ struct alloc_context { bool spread_dirty_pages; }; -/* - * Locate the struct page for both the matching buddy in our - * pair (buddy1) and the combined O(n+1) page they form (page). - * - * 1) Any buddy B1 will have an order O twin B2 which satisfies - * the following equation: - * B2 = B1 ^ (1 << O) - * For example, if the starting buddy (buddy2) is #8 its order - * 1 buddy is #10: - * B2 = 8 ^ (1 << 1) = 8 ^ 2 = 10 - * - * 2) Any buddy B will have an order O+1 parent P which - * satisfies the following equation: - * P = B & ~(1 << O) - * - * Assumption: *_mem_map is contiguous at least up to MAX_ORDER - */ -static inline unsigned long -__find_buddy_pfn(unsigned long page_pfn, unsigned int order) -{ - return page_pfn ^ (1 << order); -} +extern struct page *find_buddy_page_pfn(struct page *page, unsigned int order); extern struct page *__pageblock_pfn_to_page(unsigned long start_pfn, unsigned long end_pfn, struct zone *zone); diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 2ea106146686..8aedc6fbfdd0 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -998,6 +998,51 @@ static inline void del_page_from_free_list(struct page *page, struct zone *zone, zone->free_area[order].nr_free--; } +/* + * Locate the struct page for both the matching buddy in our + * pair (buddy1) and the combined O(n+1) page they form (page). + * + * 1) Any buddy B1 will have an order O twin B2 which satisfies + * the following equation: + * B2 = B1 ^ (1 << O) + * For example, if the starting buddy (buddy2) is #8 its order + * 1 buddy is #10: + * B2 = 8 ^ (1 << 1) = 8 ^ 2 = 10 + * + * 2) Any buddy B will have an order O+1 parent P which + * satisfies the following equation: + * P = B & ~(1 << O) + * + * Assumption: *_mem_map is contiguous at least up to MAX_ORDER + */ +static inline unsigned long +__find_buddy_pfn(unsigned long page_pfn, unsigned int order) +{ + return page_pfn ^ (1 << order); +} + + +/* + * Find the buddy of @page and validate it. + * @page: The input page + * @order: The order of the input page + * + * The found buddy can be a non PageBuddy, out of @page's zone, or its order is + * not the same as @page. The validation is necessary before use it. + * + * Return: the found buddy page or NULL if not found. + */ +struct page *find_buddy_page_pfn(struct page *page, unsigned int order) +{ + unsigned long pfn = page_to_pfn(page); + unsigned long buddy_pfn = __find_buddy_pfn(pfn, order); + struct page *buddy = page + (buddy_pfn - pfn); + + if (page_is_buddy(page, buddy, order)) + return buddy; + return NULL; +} + /* * If this is not the largest possible page, check if the buddy * of the next-highest order is free. If it is, it's possible @@ -1010,18 +1055,16 @@ static inline bool buddy_merge_likely(unsigned long pfn, unsigned long buddy_pfn, struct page *page, unsigned int order) { - struct page *higher_page, *higher_buddy; - unsigned long combined_pfn; + struct page *higher_page; + unsigned long higher_page_pfn; if (order >= MAX_ORDER - 2) return false; - combined_pfn = buddy_pfn & pfn; - higher_page = page + (combined_pfn - pfn); - buddy_pfn = __find_buddy_pfn(combined_pfn, order + 1); - higher_buddy = higher_page + (buddy_pfn - combined_pfn); + higher_page_pfn = buddy_pfn & pfn; + higher_page = page + (higher_page_pfn - pfn); - return page_is_buddy(higher_page, higher_buddy, order + 1); + return find_buddy_page_pfn(higher_page, order + 1) != NULL; } /* @@ -1075,11 +1118,11 @@ static inline void __free_one_page(struct page *page, migratetype); return; } - buddy_pfn = __find_buddy_pfn(pfn, order); - buddy = page + (buddy_pfn - pfn); - if (!page_is_buddy(page, buddy, order)) + buddy = find_buddy_page_pfn(page, order); + if (!buddy) goto done_merging; + buddy_pfn = page_to_pfn(buddy); if (unlikely(order >= pageblock_order)) { /* diff --git a/mm/page_isolation.c b/mm/page_isolation.c index f67c4c70f17f..7326c9f57d55 100644 --- a/mm/page_isolation.c +++ b/mm/page_isolation.c @@ -70,7 +70,6 @@ static void unset_migratetype_isolate(struct page *page, unsigned migratetype) unsigned long flags, nr_pages; bool isolated_page = false; unsigned int order; - unsigned long pfn, buddy_pfn; struct page *buddy; zone = page_zone(page); @@ -89,11 +88,8 @@ static void unset_migratetype_isolate(struct page *page, unsigned migratetype) if (PageBuddy(page)) { order = buddy_order(page); if (order >= pageblock_order && order < MAX_ORDER - 1) { - pfn = page_to_pfn(page); - buddy_pfn = __find_buddy_pfn(pfn, order); - buddy = page + (buddy_pfn - pfn); - - if (!is_migrate_isolate_page(buddy)) { + buddy = find_buddy_page_pfn(page, order); + if (buddy && !is_migrate_isolate_page(buddy)) { isolated_page = !!__isolate_free_page(page, order); /* * Isolating a free page in an isolated pageblock -- 2.35.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] mm: wrap __find_buddy_pfn() with a necessary buddy page validation. 2022-04-01 18:11 ` [PATCH v2 2/2] mm: wrap __find_buddy_pfn() with a necessary buddy page validation Zi Yan @ 2022-04-01 18:29 ` Linus Torvalds 2022-04-01 18:56 ` Zi Yan 0 siblings, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2022-04-01 18:29 UTC (permalink / raw) To: Zi Yan Cc: Linux-MM, Steven Rostedt, David Hildenbrand, Vlastimil Babka, Mel Gorman, Mike Rapoport, Oscar Salvador, Andrew Morton, Linux Kernel Mailing List On Fri, Apr 1, 2022 at 11:11 AM Zi Yan <zi.yan@sent.com> wrote: > > Whenever the buddy of a page is found from __find_buddy_pfn(), > page_is_buddy() should be used to check its validity. Add a helper > function find_buddy_page_pfn() to find the buddy page and do the check > together. Well, this certainly looks nicer, except now: > +extern struct page *find_buddy_page_pfn(struct page *page, unsigned int order); that 'pfn' no longer makes sense in the name, and > @@ -1075,11 +1118,11 @@ static inline void __free_one_page(struct page *page, > migratetype); > return; > } > - buddy_pfn = __find_buddy_pfn(pfn, order); > - buddy = page + (buddy_pfn - pfn); > > - if (!page_is_buddy(page, buddy, order)) > + buddy = find_buddy_page_pfn(page, order); > + if (!buddy) > goto done_merging; > + buddy_pfn = page_to_pfn(buddy); This case now does two "page_to_pfn()" calls (one inside find_buddy_page_pfn(), and one explicitly on the buddy). And those page_to_pfn() things can actually be fairly expensive. It *looks* like just a subtraction, but it's a pointer subtraction that can end up generating a divide by a non-power-of-two size. NORMALLY we try very hard to make 'sizeof struct page' be exactly 8 words, but I do not believe this is actually guaranteed. And yeah, the divide-by-a-constant can be turned into a multiply, but even that is not necessarily always cheap. Now, two out of three use-cases didn't actually want the buddy_pfn(), but this one use-case does look like it might be performance-critical and a problem. Linus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] mm: wrap __find_buddy_pfn() with a necessary buddy page validation. 2022-04-01 18:29 ` Linus Torvalds @ 2022-04-01 18:56 ` Zi Yan 2022-04-01 19:01 ` Linus Torvalds 0 siblings, 1 reply; 8+ messages in thread From: Zi Yan @ 2022-04-01 18:56 UTC (permalink / raw) To: Linus Torvalds Cc: Linux-MM, Steven Rostedt, David Hildenbrand, Vlastimil Babka, Mel Gorman, Mike Rapoport, Oscar Salvador, Andrew Morton, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 7650 bytes --] On 1 Apr 2022, at 14:29, Linus Torvalds wrote: > On Fri, Apr 1, 2022 at 11:11 AM Zi Yan <zi.yan@sent.com> wrote: >> >> Whenever the buddy of a page is found from __find_buddy_pfn(), >> page_is_buddy() should be used to check its validity. Add a helper >> function find_buddy_page_pfn() to find the buddy page and do the check >> together. > > Well, this certainly looks nicer, except now: > >> +extern struct page *find_buddy_page_pfn(struct page *page, unsigned int order); > > that 'pfn' no longer makes sense in the name, and > >> @@ -1075,11 +1118,11 @@ static inline void __free_one_page(struct page *page, >> migratetype); >> return; >> } >> - buddy_pfn = __find_buddy_pfn(pfn, order); >> - buddy = page + (buddy_pfn - pfn); >> >> - if (!page_is_buddy(page, buddy, order)) >> + buddy = find_buddy_page_pfn(page, order); >> + if (!buddy) >> goto done_merging; >> + buddy_pfn = page_to_pfn(buddy); > > This case now does two "page_to_pfn()" calls (one inside > find_buddy_page_pfn(), and one explicitly on the buddy). > > And those page_to_pfn() things can actually be fairly expensive. It > *looks* like just a subtraction, but it's a pointer subtraction that > can end up generating a divide by a non-power-of-two size. > > NORMALLY we try very hard to make 'sizeof struct page' be exactly 8 > words, but I do not believe this is actually guaranteed. > > And yeah, the divide-by-a-constant can be turned into a multiply, but > even that is not necessarily always cheap. > > Now, two out of three use-cases didn't actually want the buddy_pfn(), > but this one use-case does look like it might be performance-critical > and a problem. Yeah, I should have listened to your initial suggestion. Thanks for the explanation. How about the patch below? If it looks good, I will send v3. diff --git a/mm/internal.h b/mm/internal.h index 876e66237c89..754a666e5785 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -211,28 +211,8 @@ struct alloc_context { bool spread_dirty_pages; }; -/* - * Locate the struct page for both the matching buddy in our - * pair (buddy1) and the combined O(n+1) page they form (page). - * - * 1) Any buddy B1 will have an order O twin B2 which satisfies - * the following equation: - * B2 = B1 ^ (1 << O) - * For example, if the starting buddy (buddy2) is #8 its order - * 1 buddy is #10: - * B2 = 8 ^ (1 << 1) = 8 ^ 2 = 10 - * - * 2) Any buddy B will have an order O+1 parent P which - * satisfies the following equation: - * P = B & ~(1 << O) - * - * Assumption: *_mem_map is contiguous at least up to MAX_ORDER - */ -static inline unsigned long -__find_buddy_pfn(unsigned long page_pfn, unsigned int order) -{ - return page_pfn ^ (1 << order); -} +extern struct page *find_buddy_page_pfn(struct page *page, unsigned long pfn, + unsigned int order, unsigned long *buddy_pfn); extern struct page *__pageblock_pfn_to_page(unsigned long start_pfn, unsigned long end_pfn, struct zone *zone); diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 2ea106146686..8195b4f64ec5 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -998,6 +998,57 @@ static inline void del_page_from_free_list(struct page *page, struct zone *zone, zone->free_area[order].nr_free--; } +/* + * Locate the struct page for both the matching buddy in our + * pair (buddy1) and the combined O(n+1) page they form (page). + * + * 1) Any buddy B1 will have an order O twin B2 which satisfies + * the following equation: + * B2 = B1 ^ (1 << O) + * For example, if the starting buddy (buddy2) is #8 its order + * 1 buddy is #10: + * B2 = 8 ^ (1 << 1) = 8 ^ 2 = 10 + * + * 2) Any buddy B will have an order O+1 parent P which + * satisfies the following equation: + * P = B & ~(1 << O) + * + * Assumption: *_mem_map is contiguous at least up to MAX_ORDER + */ +static inline unsigned long +__find_buddy_pfn(unsigned long page_pfn, unsigned int order) +{ + return page_pfn ^ (1 << order); +} + +/* + * Find the buddy of @page and validate it. + * @page: The input page + * @pfn: The pfn of the page, it saves a call to page_to_pfn() when the + * function is used in the performance-critical __free_one_page(). + * @order: The order of the page + * @buddy_pfn: The output pointer to the buddy pfn, it also saves a call to + * page_to_pfn(). + * + * The found buddy can be a non PageBuddy, out of @page's zone, or its order is + * not the same as @page. The validation is necessary before use it. + * + * Return: the found buddy page or NULL if not found. + */ +struct page *find_buddy_page_pfn(struct page *page, unsigned long pfn, + unsigned int order, unsigned long *buddy_pfn) +{ + struct page *buddy; + + *buddy_pfn = __find_buddy_pfn(pfn, order); + buddy = page + (*buddy_pfn - pfn); + + if (page_is_buddy(page, buddy, order)) + return buddy; + return NULL; +} + /* * If this is not the largest possible page, check if the buddy * of the next-highest order is free. If it is, it's possible @@ -1010,18 +1061,17 @@ static inline bool buddy_merge_likely(unsigned long pfn, unsigned long buddy_pfn, struct page *page, unsigned int order) { - struct page *higher_page, *higher_buddy; - unsigned long combined_pfn; + struct page *higher_page; + unsigned long higher_page_pfn; if (order >= MAX_ORDER - 2) return false; - combined_pfn = buddy_pfn & pfn; - higher_page = page + (combined_pfn - pfn); - buddy_pfn = __find_buddy_pfn(combined_pfn, order + 1); - higher_buddy = higher_page + (buddy_pfn - combined_pfn); + higher_page_pfn = buddy_pfn & pfn; + higher_page = page + (higher_page_pfn - pfn); - return page_is_buddy(higher_page, higher_buddy, order + 1); + return find_buddy_page_pfn(higher_page, higher_page_pfn, order + 1, + &buddy_pfn) != NULL; } /* @@ -1075,10 +1125,9 @@ static inline void __free_one_page(struct page *page, migratetype); return; } - buddy_pfn = __find_buddy_pfn(pfn, order); - buddy = page + (buddy_pfn - pfn); - if (!page_is_buddy(page, buddy, order)) + buddy = find_buddy_page_pfn(page, pfn, order, &buddy_pfn); + if (!buddy) goto done_merging; if (unlikely(order >= pageblock_order)) { diff --git a/mm/page_isolation.c b/mm/page_isolation.c index f67c4c70f17f..4fbd27ba91c6 100644 --- a/mm/page_isolation.c +++ b/mm/page_isolation.c @@ -70,7 +70,7 @@ static void unset_migratetype_isolate(struct page *page, unsigned migratetype) unsigned long flags, nr_pages; bool isolated_page = false; unsigned int order; - unsigned long pfn, buddy_pfn; + unsigned long buddy_pfn; struct page *buddy; zone = page_zone(page); @@ -89,11 +89,9 @@ static void unset_migratetype_isolate(struct page *page, unsigned migratetype) if (PageBuddy(page)) { order = buddy_order(page); if (order >= pageblock_order && order < MAX_ORDER - 1) { - pfn = page_to_pfn(page); - buddy_pfn = __find_buddy_pfn(pfn, order); - buddy = page + (buddy_pfn - pfn); - - if (!is_migrate_isolate_page(buddy)) { + buddy = find_buddy_page_pfn(page, page_to_pfn(page), + order, &buddy_pfn); + if (buddy && !is_migrate_isolate_page(buddy)) { isolated_page = !!__isolate_free_page(page, order); /* * Isolating a free page in an isolated pageblock -- Best Regards, Yan, Zi [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 854 bytes --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] mm: wrap __find_buddy_pfn() with a necessary buddy page validation. 2022-04-01 18:56 ` Zi Yan @ 2022-04-01 19:01 ` Linus Torvalds 2022-04-01 19:25 ` Zi Yan 0 siblings, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2022-04-01 19:01 UTC (permalink / raw) To: Zi Yan Cc: Linux-MM, Steven Rostedt, David Hildenbrand, Vlastimil Babka, Mel Gorman, Mike Rapoport, Oscar Salvador, Andrew Morton, Linux Kernel Mailing List On Fri, Apr 1, 2022 at 11:56 AM Zi Yan <ziy@nvidia.com> wrote: > > How about the patch below? If it looks good, I will send v3. I can't see anything worrisome, but by now I've looked at several versions and who knows what I'm missing. Making it inline and allowing a NULL 'buddy_pfn' pointer for the cases that don't care might be an option, but I don't think it matters hugely. Linus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] mm: wrap __find_buddy_pfn() with a necessary buddy page validation. 2022-04-01 19:01 ` Linus Torvalds @ 2022-04-01 19:25 ` Zi Yan 2022-04-01 22:33 ` Vlastimil Babka 0 siblings, 1 reply; 8+ messages in thread From: Zi Yan @ 2022-04-01 19:25 UTC (permalink / raw) To: Linus Torvalds Cc: Linux-MM, Steven Rostedt, David Hildenbrand, Vlastimil Babka, Mel Gorman, Mike Rapoport, Oscar Salvador, Andrew Morton, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 1701 bytes --] On 1 Apr 2022, at 15:01, Linus Torvalds wrote: > On Fri, Apr 1, 2022 at 11:56 AM Zi Yan <ziy@nvidia.com> wrote: >> >> How about the patch below? If it looks good, I will send v3. > > I can't see anything worrisome, but by now I've looked at several > versions and who knows what I'm missing. > > Making it inline and allowing a NULL 'buddy_pfn' pointer for the cases > that don't care might be an option, but I don't think it matters > hugely. What do you mean by inlining it? Moving the function and __find_buddy_pfn() to mm/internal.h and mark both inline? Something like below to allow a NULL 'buddy_pfn'? buddy_pfn is needed to store the result of __find_buddy_pfn(). The code does not look as nice as before. struct page *find_buddy_page_pfn(struct page *page, unsigned long pfn, unsigned int order, unsigned long *buddy_pfn) { struct page *buddy; if (buddy_pfn) { *buddy_pfn = __find_buddy_pfn(pfn, order); buddy = page + (*buddy_pfn - pfn); } else buddy = page + (__find_buddy_pfn(pfn, order) - pfn); if (page_is_buddy(page, buddy, order)) return buddy; return NULL; } or struct page *find_buddy_page_pfn(struct page *page, unsigned long pfn, unsigned int order, unsigned long *buddy_pfn) { struct page *buddy; unsigned long local_buddy_pfn = __find_buddy_pfn(pfn, order); buddy = page + (local_buddy_pfn - pfn); if (buddy_pfn) *buddy_pfn = local_buddy_pfn; if (page_is_buddy(page, buddy, order)) return buddy; return NULL; } -- Best Regards, Yan, Zi [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 854 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] mm: wrap __find_buddy_pfn() with a necessary buddy page validation. 2022-04-01 19:25 ` Zi Yan @ 2022-04-01 22:33 ` Vlastimil Babka 0 siblings, 0 replies; 8+ messages in thread From: Vlastimil Babka @ 2022-04-01 22:33 UTC (permalink / raw) To: Zi Yan, Linus Torvalds Cc: Linux-MM, Steven Rostedt, David Hildenbrand, Mel Gorman, Mike Rapoport, Oscar Salvador, Andrew Morton, Linux Kernel Mailing List On 4/1/22 21:25, Zi Yan wrote: > On 1 Apr 2022, at 15:01, Linus Torvalds wrote: > >> On Fri, Apr 1, 2022 at 11:56 AM Zi Yan <ziy@nvidia.com> wrote: >>> >>> How about the patch below? If it looks good, I will send v3. >> >> I can't see anything worrisome, but by now I've looked at several >> versions and who knows what I'm missing. >> >> Making it inline and allowing a NULL 'buddy_pfn' pointer for the cases >> that don't care might be an option, but I don't think it matters >> hugely. > > What do you mean by inlining it? Moving the function and __find_buddy_pfn() > to mm/internal.h and mark both inline? I would prefer that for the sake of __free_one_page(). > Something like below to allow a NULL 'buddy_pfn'? buddy_pfn is needed > to store the result of __find_buddy_pfn(). The code does not look > as nice as before. > > struct page *find_buddy_page_pfn(struct page *page, unsigned long pfn, > unsigned int order, unsigned long *buddy_pfn) > { > struct page *buddy; > > if (buddy_pfn) { > *buddy_pfn = __find_buddy_pfn(pfn, order); > buddy = page + (*buddy_pfn - pfn); > } else > buddy = page + (__find_buddy_pfn(pfn, order) - pfn); > > if (page_is_buddy(page, buddy, order)) > return buddy; > return NULL; > } > > or > > struct page *find_buddy_page_pfn(struct page *page, unsigned long pfn, > unsigned int order, unsigned long *buddy_pfn) > { > struct page *buddy; > unsigned long local_buddy_pfn = __find_buddy_pfn(pfn, order); > > buddy = page + (local_buddy_pfn - pfn); > if (buddy_pfn) > *buddy_pfn = local_buddy_pfn; > > if (page_is_buddy(page, buddy, order)) > return buddy; > return NULL; > } The latter looks better. I would also use a name e.g. '__buddy_pfn' instead of 'local_'. Thanks. > > -- > Best Regards, > Yan, Zi ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] mm: page_alloc: simplify pageblock migratetype check in __free_one_page(). 2022-04-01 18:11 [PATCH v2 1/2] mm: page_alloc: simplify pageblock migratetype check in __free_one_page() Zi Yan 2022-04-01 18:11 ` [PATCH v2 2/2] mm: wrap __find_buddy_pfn() with a necessary buddy page validation Zi Yan @ 2022-04-01 22:14 ` Vlastimil Babka 1 sibling, 0 replies; 8+ messages in thread From: Vlastimil Babka @ 2022-04-01 22:14 UTC (permalink / raw) To: Zi Yan, linux-mm Cc: Linus Torvalds, Steven Rostedt, David Hildenbrand, Mel Gorman, Mike Rapoport, Oscar Salvador, Andrew Morton, linux-kernel On 4/1/22 20:11, Zi Yan wrote: > From: Zi Yan <ziy@nvidia.com> > > Move pageblock migratetype check code in the while loop to simplify the > logic. It also saves redundant buddy page checking code. > > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > Link: https://lore.kernel.org/linux-mm/27ff69f9-60c5-9e59-feb2-295250077551@suse.cz/ > Signed-off-by: Zi Yan <ziy@nvidia.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/page_alloc.c | 46 +++++++++++++++++----------------------------- > 1 file changed, 17 insertions(+), 29 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 856473e54155..2ea106146686 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -1054,7 +1054,6 @@ static inline void __free_one_page(struct page *page, > int migratetype, fpi_t fpi_flags) > { > struct capture_control *capc = task_capc(zone); > - unsigned int max_order = pageblock_order; > unsigned long buddy_pfn; > unsigned long combined_pfn; > struct page *buddy; > @@ -1070,8 +1069,7 @@ static inline void __free_one_page(struct page *page, > VM_BUG_ON_PAGE(pfn & ((1 << order) - 1), page); > VM_BUG_ON_PAGE(bad_range(zone, page), page); > > -continue_merging: > - while (order < max_order) { > + while (order < MAX_ORDER - 1) { > if (compaction_capture(capc, page, order, migratetype)) { > __mod_zone_freepage_state(zone, -(1 << order), > migratetype); > @@ -1082,6 +1080,22 @@ static inline void __free_one_page(struct page *page, > > if (!page_is_buddy(page, buddy, order)) > goto done_merging; > + > + if (unlikely(order >= pageblock_order)) { > + /* > + * We want to prevent merge between freepages on pageblock > + * without fallbacks and normal pageblock. Without this, > + * pageblock isolation could cause incorrect freepage or CMA > + * accounting or HIGHATOMIC accounting. > + */ > + int buddy_mt = get_pageblock_migratetype(buddy); > + > + if (migratetype != buddy_mt > + && (!migratetype_is_mergeable(migratetype) || > + !migratetype_is_mergeable(buddy_mt))) > + goto done_merging; > + } > + > /* > * Our buddy is free or it is CONFIG_DEBUG_PAGEALLOC guard page, > * merge with it and move up one order. > @@ -1095,32 +1109,6 @@ static inline void __free_one_page(struct page *page, > pfn = combined_pfn; > order++; > } > - if (order < MAX_ORDER - 1) { > - /* If we are here, it means order is >= pageblock_order. > - * We want to prevent merge between freepages on pageblock > - * without fallbacks and normal pageblock. Without this, > - * pageblock isolation could cause incorrect freepage or CMA > - * accounting or HIGHATOMIC accounting. > - * > - * We don't want to hit this code for the more frequent > - * low-order merging. > - */ > - int buddy_mt; > - > - buddy_pfn = __find_buddy_pfn(pfn, order); > - buddy = page + (buddy_pfn - pfn); > - > - if (!page_is_buddy(page, buddy, order)) > - goto done_merging; > - buddy_mt = get_pageblock_migratetype(buddy); > - > - if (migratetype != buddy_mt > - && (!migratetype_is_mergeable(migratetype) || > - !migratetype_is_mergeable(buddy_mt))) > - goto done_merging; > - max_order = order + 1; > - goto continue_merging; > - } > > done_merging: > set_buddy_order(page, order); ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-04-01 22:43 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-01 18:11 [PATCH v2 1/2] mm: page_alloc: simplify pageblock migratetype check in __free_one_page() Zi Yan 2022-04-01 18:11 ` [PATCH v2 2/2] mm: wrap __find_buddy_pfn() with a necessary buddy page validation Zi Yan 2022-04-01 18:29 ` Linus Torvalds 2022-04-01 18:56 ` Zi Yan 2022-04-01 19:01 ` Linus Torvalds 2022-04-01 19:25 ` Zi Yan 2022-04-01 22:33 ` Vlastimil Babka 2022-04-01 22:14 ` [PATCH v2 1/2] mm: page_alloc: simplify pageblock migratetype check in __free_one_page() Vlastimil Babka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).