Re: [PATCH v4 4/6] mm: migrate: support poisoned recover from migrate folio

[Jane Chu <jane.chu@oracle.com>]

On 6/3/2024 2:24 AM, Kefeng Wang wrote:

> The folio migration is widely used in kernel, memory compaction, memory
> hotplug, soft offline page, numa balance, memory demote/promotion, etc,
> but once access a poisoned source folio when migrating, the kerenl will
> panic.
>
> There is a mechanism in the kernel to recover from uncorrectable memory
> errors, ARCH_HAS_COPY_MC, which is already used in other core-mm paths,
> eg, CoW, khugepaged, coredump, ksm copy, see copy_mc_to_{user,kernel},
> copy_mc_{user_}highpage callers.
>
> In order to support poisoned folio copy recover from migrate folio, we
> chose to make folio migration tolerant of memory failures and return
> error for folio migration, because folio migration is no guarantee
> of success, this could avoid the similar panic shown below.
>
>    CPU: 1 PID: 88343 Comm: test_softofflin Kdump: loaded Not tainted 6.6.0
>    pc : copy_page+0x10/0xc0
>    lr : copy_highpage+0x38/0x50

I'm curious at how you manage to test this case .  I mean, you trigger a 
soft_offline,

and a source  page with UE was being migrated, next, folio_copy() 
triggers an MCE and

system panic.  Did you use a bad dimm?

>    ...
>    Call trace:
>     copy_page+0x10/0xc0
>     folio_copy+0x78/0x90
>     migrate_folio_extra+0x54/0xa0
>     move_to_new_folio+0xd8/0x1f0
>     migrate_folio_move+0xb8/0x300
>     migrate_pages_batch+0x528/0x788
>     migrate_pages_sync+0x8c/0x258
>     migrate_pages+0x440/0x528
>     soft_offline_in_use_page+0x2ec/0x3c0
>     soft_offline_page+0x238/0x310
>     soft_offline_page_store+0x6c/0xc0
>     dev_attr_store+0x20/0x40
>     sysfs_kf_write+0x4c/0x68
>     kernfs_fop_write_iter+0x130/0x1c8
>     new_sync_write+0xa4/0x138
>     vfs_write+0x238/0x2d8
>     ksys_write+0x74/0x110
>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   mm/migrate.c | 23 ++++++++++++++++++-----
>   1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index e930376c261a..28aa9da95781 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -663,16 +663,29 @@ static int __migrate_folio(struct address_space *mapping, struct folio *dst,
>   			   struct folio *src, void *src_private,
>   			   enum migrate_mode mode)
>   {
> -	int rc;
> +	int ret, expected_cnt = folio_expected_refs(mapping, src);
>   
> -	rc = folio_migrate_mapping(mapping, dst, src, 0);
> -	if (rc != MIGRATEPAGE_SUCCESS)
> -		return rc;
> +	if (!mapping) {
> +		if (folio_ref_count(src) != expected_cnt)
> +			return -EAGAIN;
> +	} else {
> +		if (!folio_ref_freeze(src, expected_cnt))
> +			return -EAGAIN;
> +	}
> +

Let me take a guess, the reason you split up folio_migrate_copy() is that

folio_mc_copy() should be done before the 'src' folio's ->flags is 
changed, right?

Is there any other reason?  Could you add a comment please?

> +	ret = folio_mc_copy(dst, src);
> +	if (unlikely(ret)) {
> +		if (mapping)
> +			folio_ref_unfreeze(src, expected_cnt);
> +		return ret;
> +	}
> +
> +	__folio_migrate_mapping(mapping, dst, src, expected_cnt);
>   
>   	if (src_private)
>   		folio_attach_private(dst, folio_detach_private(src));
>   
> -	folio_migrate_copy(dst, src);
> +	folio_migrate_flags(dst, src);
>   	return MIGRATEPAGE_SUCCESS;
>   }
>   

thanks,

-jane