| Message ID | 20190725184253.21160-3-lpf.vector@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] mm/page_alloc: use unsigned int for "order" in should_compact_retry() | expand |
On 25/07/2019 20.42, Pengfei Li wrote: > Because "order" will never be negative in __rmqueue_fallback(), > so just make "order" unsigned int. > And modify trace_mm_page_alloc_extfrag() accordingly. > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 75c18f4fd66a..1432cbcd87cd 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2631,8 +2631,8 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac, > * condition simpler. > */ > static __always_inline bool > -__rmqueue_fallback(struct zone *zone, int order, int start_migratetype, > - unsigned int alloc_flags) > +__rmqueue_fallback(struct zone *zone, unsigned int order, > + int start_migratetype, unsigned int alloc_flags) > { Please read the last paragraph of the comment above this function, run git blame to figure out when that was introduced, and then read the full commit description. Here be dragons. At the very least, this patch is wrong in that it makes that comment inaccurate. Rasmus
On Fri, Jul 26, 2019 at 5:36 PM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > On 25/07/2019 20.42, Pengfei Li wrote: > > Because "order" will never be negative in __rmqueue_fallback(), > > so just make "order" unsigned int. > > And modify trace_mm_page_alloc_extfrag() accordingly. > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 75c18f4fd66a..1432cbcd87cd 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -2631,8 +2631,8 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac, > > * condition simpler. > > */ > > static __always_inline bool > > -__rmqueue_fallback(struct zone *zone, int order, int start_migratetype, > > - unsigned int alloc_flags) > > +__rmqueue_fallback(struct zone *zone, unsigned int order, > > + int start_migratetype, unsigned int alloc_flags) > > { > > Please read the last paragraph of the comment above this function, run > git blame to figure out when that was introduced, and then read the full > commit description. Thanks for your comments. I have read the commit info of commit b002529d2563 ("mm/page_alloc.c: eliminate unsigned confusion in __rmqueue_fallback"). And I looked at the discussion at https://lkml.org/lkml/2017/6/21/684 in detail. > Here be dragons. At the very least, this patch is > wrong in that it makes that comment inaccurate. I wonder if you noticed the commit 6bb154504f8b ("mm, page_alloc: spread allocations across zones before introducing fragmentation"). Commit 6bb154504f8b introduces a local variable min_order in __rmqueue_fallback(). And you can see for (current_order = MAX_ORDER - 1; current_order >= min_order; --current_order) { The “current_order” and "min_order" are int, so here is ok. Since __rmqueue_fallback() is only called by __rmqueue() and "order" is unsigned int in __rmqueue(), then I think that making "order" is also unsigned int is good. Maybe I should also modify the comments here? > > Rasmus Thank you again for your review. -- Pengfei
diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h index eb57e3037deb..31f4d09aa31f 100644 --- a/include/trace/events/kmem.h +++ b/include/trace/events/kmem.h @@ -277,7 +277,7 @@ TRACE_EVENT(mm_page_pcpu_drain, TRACE_EVENT(mm_page_alloc_extfrag, TP_PROTO(struct page *page, - int alloc_order, int fallback_order, + unsigned int alloc_order, int fallback_order, int alloc_migratetype, int fallback_migratetype), TP_ARGS(page, @@ -286,7 +286,7 @@ TRACE_EVENT(mm_page_alloc_extfrag, TP_STRUCT__entry( __field( unsigned long, pfn ) - __field( int, alloc_order ) + __field( unsigned int, alloc_order ) __field( int, fallback_order ) __field( int, alloc_migratetype ) __field( int, fallback_migratetype ) @@ -303,7 +303,7 @@ TRACE_EVENT(mm_page_alloc_extfrag, get_pageblock_migratetype(page)); ), - TP_printk("page=%p pfn=%lu alloc_order=%d fallback_order=%d pageblock_order=%d alloc_migratetype=%d fallback_migratetype=%d fragmenting=%d change_ownership=%d", + TP_printk("page=%p pfn=%lu alloc_order=%u fallback_order=%d pageblock_order=%d alloc_migratetype=%d fallback_migratetype=%d fragmenting=%d change_ownership=%d", pfn_to_page(__entry->pfn), __entry->pfn, __entry->alloc_order, diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 75c18f4fd66a..1432cbcd87cd 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2631,8 +2631,8 @@ static bool unreserve_highatomic_pageblock(const struct alloc_context *ac, * condition simpler. */ static __always_inline bool -__rmqueue_fallback(struct zone *zone, int order, int start_migratetype, - unsigned int alloc_flags) +__rmqueue_fallback(struct zone *zone, unsigned int order, + int start_migratetype, unsigned int alloc_flags) { struct free_area *area; int current_order;
Because "order" will never be negative in __rmqueue_fallback(), so just make "order" unsigned int. And modify trace_mm_page_alloc_extfrag() accordingly. Signed-off-by: Pengfei Li <lpf.vector@gmail.com> --- include/trace/events/kmem.h | 6 +++--- mm/page_alloc.c | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)