Re: [PATCH v2 14/46] hugetlb: split PTE markers when doing HGM walks

[Mike Kravetz]

On 02/18/23 00:27, James Houghton wrote:
> Fix how UFFDIO_CONTINUE and UFFDIO_WRITEPROTECT interact in these two
> ways:
>  - UFFDIO_WRITEPROTECT no longer prevents a high-granularity
>    UFFDIO_CONTINUE.
>  - UFFD-WP PTE markers installed with UFFDIO_WRITEPROTECT will be
>    properly propagated when high-granularily UFFDIO_CONTINUEs are
>    performed.
> 
> Note: UFFDIO_WRITEPROTECT is not yet permitted at PAGE_SIZE granularity.
> 
> Signed-off-by: James Houghton <jthoughton@xxxxxxxxxx>
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 810c05feb41f..f74183acc521 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c

Seems relatively straight forward,

Acked-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
-- 
Mike Kravetz

> @@ -506,6 +506,30 @@ static bool has_same_uncharge_info(struct file_region *rg,
>  #endif
>  }
>  
> +static void hugetlb_install_markers_pmd(pmd_t *pmdp, pte_marker marker)
> +{
> +	int i;
> +
> +	for (i = 0; i < PTRS_PER_PMD; ++i)
> +		/*
> +		 * WRITE_ONCE not needed because the pud hasn't been
> +		 * installed yet.
> +		 */
> +		pmdp[i] = __pmd(pte_val(make_pte_marker(marker)));
> +}
> +
> +static void hugetlb_install_markers_pte(pte_t *ptep, pte_marker marker)
> +{
> +	int i;
> +
> +	for (i = 0; i < PTRS_PER_PTE; ++i)
> +		/*
> +		 * WRITE_ONCE not needed because the pmd hasn't been
> +		 * installed yet.
> +		 */
> +		ptep[i] = make_pte_marker(marker);
> +}
> +
>  /*
>   * hugetlb_alloc_pmd -- Allocate or find a PMD beneath a PUD-level hpte.
>   *
> @@ -528,23 +552,32 @@ pmd_t *hugetlb_alloc_pmd(struct mm_struct *mm, struct hugetlb_pte *hpte,
>  	pmd_t *new;
>  	pud_t *pudp;
>  	pud_t pud;
> +	bool is_marker;
> +	pte_marker marker;
>  
>  	if (hpte->level != HUGETLB_LEVEL_PUD)
>  		return ERR_PTR(-EINVAL);
>  
>  	pudp = (pud_t *)hpte->ptep;
>  retry:
> +	is_marker = false;
>  	pud = READ_ONCE(*pudp);
>  	if (likely(pud_present(pud)))
>  		return unlikely(pud_leaf(pud))
>  			? ERR_PTR(-EEXIST)
>  			: pmd_offset(pudp, addr);
> -	else if (!pud_none(pud))
> +	else if (!pud_none(pud)) {
>  		/*
> -		 * Not present and not none means that a swap entry lives here,
> -		 * and we can't get rid of it.
> +		 * Not present and not none means that a swap entry lives here.
> +		 * If it's a PTE marker, we can deal with it. If it's another
> +		 * swap entry, we don't attempt to split it.
>  		 */
> -		return ERR_PTR(-EEXIST);
> +		is_marker = is_pte_marker(__pte(pud_val(pud)));
> +		if (!is_marker)
> +			return ERR_PTR(-EEXIST);
> +
> +		marker = pte_marker_get(pte_to_swp_entry(__pte(pud_val(pud))));
> +	}
>  
>  	new = pmd_alloc_one(mm, addr);
>  	if (!new)
> @@ -557,6 +590,13 @@ pmd_t *hugetlb_alloc_pmd(struct mm_struct *mm, struct hugetlb_pte *hpte,
>  		goto retry;
>  	}
>  
> +	/*
> +	 * Install markers before PUD to avoid races with other
> +	 * page tables walks.
> +	 */
> +	if (is_marker)
> +		hugetlb_install_markers_pmd(new, marker);
> +
>  	mm_inc_nr_pmds(mm);
>  	smp_wmb(); /* See comment in pmd_install() */
>  	pud_populate(mm, pudp, new);
> @@ -576,23 +616,32 @@ pte_t *hugetlb_alloc_pte(struct mm_struct *mm, struct hugetlb_pte *hpte,
>  	pgtable_t new;
>  	pmd_t *pmdp;
>  	pmd_t pmd;
> +	bool is_marker;
> +	pte_marker marker;
>  
>  	if (hpte->level != HUGETLB_LEVEL_PMD)
>  		return ERR_PTR(-EINVAL);
>  
>  	pmdp = (pmd_t *)hpte->ptep;
>  retry:
> +	is_marker = false;
>  	pmd = READ_ONCE(*pmdp);
>  	if (likely(pmd_present(pmd)))
>  		return unlikely(pmd_leaf(pmd))
>  			? ERR_PTR(-EEXIST)
>  			: pte_offset_kernel(pmdp, addr);
> -	else if (!pmd_none(pmd))
> +	else if (!pmd_none(pmd)) {
>  		/*
> -		 * Not present and not none means that a swap entry lives here,
> -		 * and we can't get rid of it.
> +		 * Not present and not none means that a swap entry lives here.
> +		 * If it's a PTE marker, we can deal with it. If it's another
> +		 * swap entry, we don't attempt to split it.
>  		 */
> -		return ERR_PTR(-EEXIST);
> +		is_marker = is_pte_marker(__pte(pmd_val(pmd)));
> +		if (!is_marker)
> +			return ERR_PTR(-EEXIST);
> +
> +		marker = pte_marker_get(pte_to_swp_entry(__pte(pmd_val(pmd))));
> +	}
>  
>  	/*
>  	 * With CONFIG_HIGHPTE, calling `pte_alloc_one` directly may result
> @@ -613,6 +662,9 @@ pte_t *hugetlb_alloc_pte(struct mm_struct *mm, struct hugetlb_pte *hpte,
>  		goto retry;
>  	}
>  
> +	if (is_marker)
> +		hugetlb_install_markers_pte(page_address(new), marker);
> +
>  	mm_inc_nr_ptes(mm);
>  	smp_wmb(); /* See comment in pmd_install() */
>  	pmd_populate(mm, pmdp, new);
> @@ -7384,7 +7436,12 @@ static int __hugetlb_hgm_walk(struct mm_struct *mm, struct vm_area_struct *vma,
>  		if (!pte_present(pte)) {
>  			if (!alloc)
>  				return 0;
> -			if (unlikely(!huge_pte_none(pte)))
> +			/*
> +			 * In hugetlb_alloc_pmd and hugetlb_alloc_pte,
> +			 * we split PTE markers, so we can tolerate
> +			 * PTE markers here.
> +			 */
> +			if (unlikely(!huge_pte_none_mostly(pte)))
>  				return -EEXIST;
>  		} else if (hugetlb_pte_present_leaf(hpte, pte))
>  			return 0;
> -- 
> 2.39.2.637.g21b0678d19-goog
>