Received-SPF: pass (google.com: domain of linux-kernel+bounces-127724-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <50f31489-b5c3-480e-a7db-20edbbb2c9c2@suse.cz>
Date: Tue, 2 Apr 2024 12:26:51 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 3/3] mm,page_owner: Fix accounting of pages when
 migrating
Content-Language: en-US
To: Oscar Salvador <osalvador@suse.de>,
 Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
 Michal Hocko <mhocko@suse.com>, Marco Elver <elver@google.com>,
 Andrey Konovalov <andreyknvl@gmail.com>,
 Alexander Potapenko <glider@google.com>
References: <20240326063036.6242-1-osalvador@suse.de>
 <20240326063036.6242-4-osalvador@suse.de>
From: Vlastimil Babka <vbabka@suse.cz>
In-Reply-To: <20240326063036.6242-4-osalvador@suse.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 3/26/24 7:30 AM, Oscar Salvador wrote:
> Upon migration, new allocated pages are being given the handle of the old
> pages. This is problematic because it means that for the stack which
> allocated the old page, we will be substracting the old page + the new one
> when that page is freed, creating an accounting imbalance.
> 
> There is an interest in keeping it that way, as otherwise the output will
> biased towards migration stacks should those operations occur often, but
> that is not really helpful.
> The link from the new page to the old stack is being performed by calling
> __update_page_owner_handle() in __folio_copy_owner().
> The only thing that is left is to link the migrate stack to the old
> page, so the old page will be subtracted from the migrate stack,
> avoiding by doing so any possible imbalance.
> 
> Fixes: 217b2119b9e2 ("mm,page_owner: implement the tracking of the stacks count")
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
>  mm/page_owner.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 5df0d6892bdc..b4476f45b376 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -366,9 +366,12 @@ void __split_page_owner(struct page *page, int old_order, int new_order)
>  
>  void __folio_copy_owner(struct folio *newfolio, struct folio *old)
>  {
> +	int i;
>  	struct page_ext *old_ext;
>  	struct page_ext *new_ext;
>  	struct page_owner *old_page_owner;
> +	struct page_owner *new_page_owner;
> +	depot_stack_handle_t migrate_handle;
>  
>  	old_ext = page_ext_get(&old->page);
>  	if (unlikely(!old_ext))
> @@ -381,6 +384,8 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
>  	}
>  
>  	old_page_owner = get_page_owner(old_ext);
> +	new_page_owner = get_page_owner(new_ext);
> +	migrate_handle = new_page_owner->handle;
>  	__update_page_owner_handle(new_ext, old_page_owner->handle,
>  				   old_page_owner->order, old_page_owner->gfp_mask,
>  				   old_page_owner->last_migrate_reason,
> @@ -395,6 +400,16 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
>  					old_page_owner->free_pid,
>  					old_page_owner->free_tgid,
>  					old_page_owner->free_ts_nsec);
> +	/*
> +	 * We linked the original stack to the new folio, we need to do the same
> +	 * for the new one and the old folio otherwise there will be an imbalance
> +	 * when subtracting those pages from the stack.
> +	 */
> +	for (i = 0; i < (1 << new_page_owner->order); i++) {
> +		old_page_owner->handle = migrate_handle;
> +		old_ext = page_ext_next(old_ext);
> +		old_page_owner = get_page_owner(old_ext);
> +	}

Can the migration still fail after __folio_copy_owner()? That goes again to
the comment you changed in patch 1/3. If it can, this will kinda create a
mess with the old folio's handles not reflecting reality? (although
refcounts will be ok)

So if that case is possible, could we instead just dec_stack_record_count()
for the handle that allocated the new folio (IIUC "migrate_handle" here) and
inc_stack_record_count() for the original handle that we duplicated from the
old to new. Then if either old is freed (successful migration) or new is
freed (failed migration), we'll have the correct refcounts.

>  
>  	page_ext_put(new_ext);
>  	page_ext_put(old_ext);