On Mon, Aug 14, 2017 at 09:52:13PM -0400, Zi Yan wrote:
> From: Zi Yan <zi....@cs.rutgers.edu>
>
> 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....@cs.rutgers.edu>
> ---
> 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
>
>
