Re: [PATCH v3 1/3] mm,page_owner: Update metada for tail pages

From: Oscar Salvador <osalvador@suse.de>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	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>
Subject: Re: [PATCH v3 1/3] mm,page_owner: Update metada for tail pages
Date: Tue, 2 Apr 2024 13:19:42 +0200	[thread overview]
Message-ID: <ZgvpzvX8E2WOkQmW@localhost.localdomain> (raw)
In-Reply-To: <fede9c3a-5686-4c44-a459-bf36c7093203@suse.cz>

On Tue, Apr 02, 2024 at 12:13:37PM +0200, Vlastimil Babka wrote:
> Subject: metada -> metadata

Ooops.

> > Signed-off-by: Oscar Salvador <osalvador@suse.de>
> 
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

Thanks!

> > @@ -355,31 +375,21 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old)
> > -	 * We don't clear the bit on the old folio as it's going to be freed
> > -	 * after migration. Until then, the info can be useful in case of
> > -	 * a bug, and the overall stats will be off a bit only temporarily.
> > -	 * Also, migrate_misplaced_transhuge_page() can still fail the
> > -	 * migration and then we want the old folio to retain the info. But
> > -	 * in that case we also don't need to explicitly clear the info from
> > -	 * the new page, which will be freed.
> > +	 * Do not proactively clear PAGE_EXT_OWNER{_ALLOCATED} bits as the folio
> > +	 * will be freed after migration. Keep them until then as they may be
> > +	 * useful.
> >  	 */
> 
> The full old comment made sense, the new one sounds like it's talking about
> the old folio ("will be freed after migration") but we're modifying the new
> folio here. IIUC it means the case of migration failing and then the new
> folio MIGHT be freed. So I think you made the comment too much concise to be
> immediately clear?

It probably could be improved by saying that there is no need to clear
the bit from the old folio since that will be done when __reset_page_owner()
gets called on the old folio.

Now, answering your question about whether we can fail or not at this
stage.
I looked into this a few weeks ago and I made my mind that no, we cannot
fail at this stage, and the following is my reasoning.

This is the callchain that leads to folio_copy_owner:

migrate_folio_move
 move_to_new_folio
  migrate_folio
   migrate_folio_extra
    folio_migrate_copy
     folio_copy
     folio_migrate_flags
      folio_copy_owner

folio_copy_owner() gets called only from folio_migrate_flags().
And all the functions that call folio_migrate_flags(), return
MIGRATEPAGE_SUCCESS right after calling it, so it is kinda the last
step of the migration.

So no, we cannot fail at this stage, so we do not have to worry about
undoing this.

-- 
Oscar Salvador
SUSE Labs