Re: [RFC PATCH 1/4] mm: madvise: read loop's step size beforehand in madvise_inject_error(), prepare for THP support.

[Naoya Horiguchi]

On Mon, Aug 14, 2017 at 09:52:13PM -0400, Zi Yan wrote:
> From: Zi Yan <zi.yan@xxxxxxxxxxxxxx>
> 
> The loop in madvise_inject_error() reads its step size from a page
> after it is soft-offlined. It works because the page is:
> 1) a hugetlb page: the page size does not change;
> 2) a base page: the page size does not change;
> 3) a THP: soft-offline always splits THPs, thus, it is OK to use
>    PAGE_SIZE as step size.
> 
> It will be a problem when soft-offline supports THP migrations.
> When a THP is migrated without split during soft-offlining, the THP
> is split after migration, thus, before and after soft-offlining page
> sizes do not match. This causes a THP to be unnecessarily soft-lined,
> at most, 511 times, wasting free space.

Hi Zi Yan,

Thank you for the suggestion.

I think that when madvise(MADV_SOFT_OFFLINE) is called with some range
over more than one 4kB page, the caller clearly intends to call
soft_offline_page() over all 4kB pages within the range in order to
simulate the multiple soft-offline events. Please note that the caller
only knows that specific pages are half-broken, and expect that all such
pages are offlined. So the end result should be same, whether the given
range is backed by thp or not.

> 
> Signed-off-by: Zi Yan <zi.yan@xxxxxxxxxxxxxx>
> ---
>  mm/madvise.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 47d8d8a25eae..49f6774db259 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -612,19 +612,22 @@ static long madvise_remove(struct vm_area_struct *vma,
>  static int madvise_inject_error(int behavior,
>  		unsigned long start, unsigned long end)
>  {
> -	struct page *page;
> +	struct page *page = NULL;
> +	unsigned long page_size = PAGE_SIZE;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
>  
> -	for (; start < end; start += PAGE_SIZE <<
> -				compound_order(compound_head(page))) {
> +	for (; start < end; start += page_size) {
>  		int ret;
>  
>  		ret = get_user_pages_fast(start, 1, 0, &page);
>  		if (ret != 1)
>  			return ret;
>  
> +		page_size = (PAGE_SIZE << compound_order(compound_head(page))) -
> +			(PAGE_SIZE * (page - compound_head(page)));
> +

Assigning a value which is not 4kB or some hugepage size into page_size
might be confusing because that's not what the name says. You can introduce
'next' virtual address and ALIGN() might be helpful to calculate it.

Thanks,
Naoya Horiguchi


>  		if (PageHWPoison(page)) {
>  			put_page(page);
>  			continue;
> @@ -637,6 +640,12 @@ static int madvise_inject_error(int behavior,
>  			ret = soft_offline_page(page, MF_COUNT_INCREASED);
>  			if (ret)
>  				return ret;
> +			/*
> +			 * Non hugetlb pages either have PAGE_SIZE
> +			 * or are split into PAGE_SIZE
> +			 */
> +			if (!PageHuge(page))
> +				page_size = PAGE_SIZE;
>  			continue;
>  		}
>  		pr_info("Injecting memory failure for pfn %#lx at process virtual address %#lx\n",
> @@ -645,6 +654,12 @@ static int madvise_inject_error(int behavior,
>  		ret = memory_failure(page_to_pfn(page), 0, MF_COUNT_INCREASED);
>  		if (ret)
>  			return ret;
> +		/*
> +		 * Non hugetlb pages either have PAGE_SIZE
> +		 * or are split into PAGE_SIZE
> +		 */
> +		if (!PageHuge(page))
> +			page_size = PAGE_SIZE;
>  	}
>  	return 0;
>  }
> -- 
> 2.13.2
> 
>