linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vlastimil Babka <vbabka@suse.cz>
To: Muchun Song <songmuchun@bytedance.com>, akpm@linux-foundation.org
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/page_alloc: speeding up the iteration of max_order
Date: Thu, 3 Dec 2020 18:37:00 +0100	[thread overview]
Message-ID: <320c8522-4ed5-809f-e6fc-8a185587519c@suse.cz> (raw)
In-Reply-To: <20201202121838.75218-1-songmuchun@bytedance.com>

On 12/2/20 1:18 PM, Muchun Song wrote:
> When we free a page whose order is very close to MAX_ORDER and greater
> than pageblock_order, it wastes some CPU cycles to increase max_order
> to MAX_ORDER one by one and check the pageblock migratetype of that page

But we have to do that. It's not the same page, it's the merged page and the new
buddy is a different pageblock and we need to check if they have compatible
migratetypes and can merge, or we have to bail out. So the patch is wrong.

> repeatedly especially when MAX_ORDER is much larger than pageblock_order.

Do we have such architectures/configurations anyway?

> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/page_alloc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 141f12e5142c..959541234e1d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1041,7 +1041,7 @@ static inline void __free_one_page(struct page *page,
>  		pfn = combined_pfn;
>  		order++;
>  	}
> -	if (max_order < MAX_ORDER) {
> +	if (max_order < MAX_ORDER && order < MAX_ORDER - 1) {
>  		/* If we are here, it means order is >= pageblock_order.
>  		 * We want to prevent merge between freepages on isolate
>  		 * pageblock and normal pageblock. Without this, pageblock
> @@ -1062,6 +1062,8 @@ static inline void __free_one_page(struct page *page,
>  						is_migrate_isolate(buddy_mt)))
>  				goto done_merging;
>  		}
> +		if (unlikely(order != max_order - 1))
> +			max_order = order + 1;

Or maybe I just don't understand what this is doing. When is the new 'if' even
true? We just bailed out of "while (order < max_order - 1)" after the last
"order++", which means it should hold that "order == max_order - 1")?
Your description sounds like you want to increase max_order to MAX_ORDER in one
step, which as I explained would be wrong. But the implementation looks actually
like a no-op.

>  		max_order++;
>  		goto continue_merging;
>  	}
> 


  reply	other threads:[~2020-12-03 17:37 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-02 12:18 Muchun Song
2020-12-03 17:37 ` Vlastimil Babka [this message]
2020-12-04  4:03   ` [External] " Muchun Song
2020-12-04 10:28     ` Vlastimil Babka
2020-12-04 11:11       ` Muchun Song

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=320c8522-4ed5-809f-e6fc-8a185587519c@suse.cz \
    --to=vbabka@suse.cz \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=songmuchun@bytedance.com \
    --subject='Re: [PATCH] mm/page_alloc: speeding up the iteration of max_order' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).