Catalin Marinas <catalin.marinas@xxxxxxx> writes:
> On Thu, Aug 10, 2017 at 06:09:01PM +0100, Punit Agrawal wrote:
>> --- a/arch/arm64/mm/hugetlbpage.c
>> +++ b/arch/arm64/mm/hugetlbpage.c
>> @@ -68,6 +68,62 @@ static int find_num_contig(struct mm_struct *mm, unsigned long addr,
>> return CONT_PTES;
>> }
>>
>> +/*
>> + * Changing some bits of contiguous entries requires us to follow a
>> + * Break-Before-Make approach, breaking the whole contiguous set
>> + * before we can change any entries. See ARM DDI 0487A.k_iss10775,
>> + * "Misprogramming of the Contiguous bit", page D4-1762.
>> + *
>> + * This helper performs the break step.
>> + */
>> +static pte_t get_clear_flush(struct mm_struct *mm,
>> + unsigned long addr,
>> + pte_t *ptep,
>> + unsigned long pgsize,
>> + unsigned long ncontig)
>> +{
>> + unsigned long i, saddr = addr;
>> + struct vm_area_struct vma = { .vm_mm = mm };
>> + pte_t orig_pte = huge_ptep_get(ptep);
>> +
>> + /*
>> + * If we already have a faulting entry then we don't need
>> + * to break before make (there won't be a tlb entry cached).
>> + */
>> + if (!pte_present(orig_pte))
>> + return orig_pte;
>
> I first thought we could relax this check to pte_valid() as we don't
> care about the PROT_NONE case for hardware page table updates. However,
> I realised that we call this where we expect the pte to be entirely
> cleared but we simply skip it if !present (e.g. swap entry). Is this
> correct?
I've checked back and come to the conclusion that get_clear_flush() will
not get called with swap entries.
In the case of huge_ptep_get_and_clear() below, the callers
(__unmap_hugepage_range() and hugetlb_change_protection()) check for
swap entries before calling. Similarly
I'll relax the check to pte_valid().
>
>> +
>> + for (i = 0; i < ncontig; i++, addr += pgsize, ptep++) {
>> + pte_t pte = ptep_get_and_clear(mm, addr, ptep);
>> +
>> + /*
>> + * If HW_AFDBM is enabled, then the HW could turn on
>> + * the dirty bit for any page in the set, so check
>> + * them all. All hugetlb entries are already young.
>> + */
>> + if (IS_ENABLED(CONFIG_ARM64_HW_AFDBM) && pte_dirty(pte))
>> + orig_pte = pte_mkdirty(orig_pte);
>> + }
>> +
>> + flush_tlb_range(&vma, saddr, addr);
>> + return orig_pte;
>> +}
>
> It would be better if you do something like
>
> bool valid = pte_valid(org_pte);
> ...
> if (valid)
> flush_tlb_range(...);
With the above change of pte_present() to pte_valid() this isn't needed
anymore.
>
>> +
>> +static void clear_flush(struct mm_struct *mm,
>> + unsigned long addr,
>> + pte_t *ptep,
>> + unsigned long pgsize,
>> + unsigned long ncontig)
>> +{
>> + unsigned long i, saddr = addr;
>> + struct vm_area_struct vma = { .vm_mm = mm };
>> +
>> + for (i = 0; i < ncontig; i++, addr += pgsize, ptep++)
>> + pte_clear(mm, addr, ptep);
>> +
>> + flush_tlb_range(&vma, saddr, addr);
>> +}
>> +
>> void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
>> pte_t *ptep, pte_t pte)
>> {
>> @@ -93,6 +149,8 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
>> dpfn = pgsize >> PAGE_SHIFT;
>> hugeprot = pte_pgprot(pte);
>>
>> + clear_flush(mm, addr, ptep, pgsize, ncontig);
>> +
>> for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn) {
>> pr_debug("%s: set pte %p to 0x%llx\n", __func__, ptep,
>> pte_val(pfn_pte(pfn, hugeprot)));
>> @@ -194,7 +252,7 @@ pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct *vma,
>> pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
>> unsigned long addr, pte_t *ptep)
>> {
>> - int ncontig, i;
>> + int ncontig;
>> size_t pgsize;
>> pte_t orig_pte = huge_ptep_get(ptep);
>>
>> @@ -202,17 +260,8 @@ pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
>> return ptep_get_and_clear(mm, addr, ptep);
>>
>> ncontig = find_num_contig(mm, addr, ptep, &pgsize);
>> - for (i = 0; i < ncontig; i++, addr += pgsize, ptep++) {
>> - /*
>> - * If HW_AFDBM is enabled, then the HW could
>> - * turn on the dirty bit for any of the page
>> - * in the set, so check them all.
>> - */
>> - if (pte_dirty(ptep_get_and_clear(mm, addr, ptep)))
>> - orig_pte = pte_mkdirty(orig_pte);
>> - }
>>
>> - return orig_pte;
>> + return get_clear_flush(mm, addr, ptep, pgsize, ncontig);
>> }
>
> E.g. here you don't always clear the pte if a swap entry.
>
>>
>> int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>> @@ -222,6 +271,7 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>> int ncontig, i, changed = 0;
>> size_t pgsize = 0;
>> unsigned long pfn = pte_pfn(pte), dpfn;
>> + pte_t orig_pte;
>> pgprot_t hugeprot;
>>
>> if (!pte_cont(pte))
>> @@ -229,12 +279,18 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>>
>> ncontig = find_num_contig(vma->vm_mm, addr, ptep, &pgsize);
>> dpfn = pgsize >> PAGE_SHIFT;
>> - hugeprot = pte_pgprot(pte);
>>
>> - for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn) {
>> - changed |= ptep_set_access_flags(vma, addr, ptep,
>> - pfn_pte(pfn, hugeprot), dirty);
>> - }
>> + orig_pte = get_clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
>> + if (!pte_same(orig_pte, pte))
>> + changed = 1;
>> +
>> + /* Make sure we don't lose the dirty state */
>> + if (pte_dirty(orig_pte))
>> + pte = pte_mkdirty(pte);
>> +
>> + hugeprot = pte_pgprot(pte);
>> + for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn)
>> + set_pte_at(vma->vm_mm, addr, ptep, pfn_pte(pfn, hugeprot));
>>
>> return changed;
>> }
>> @@ -244,6 +300,9 @@ void huge_ptep_set_wrprotect(struct mm_struct *mm,
>> {
>> int ncontig, i;
>> size_t pgsize;
>> + pte_t pte = pte_wrprotect(huge_ptep_get(ptep)), orig_pte;
>
> I'm not particularly fond of too many function calls in the variable
> initialisation part. I would rather keep pte_wrprotect further down
> where you also make it "dirty".
>
>> + unsigned long pfn = pte_pfn(pte), dpfn;
>> + pgprot_t hugeprot;
>>
>> if (!pte_cont(*ptep)) {
>> ptep_set_wrprotect(mm, addr, ptep);
>> @@ -251,14 +310,21 @@ void huge_ptep_set_wrprotect(struct mm_struct *mm,
>> }
>>
>> ncontig = find_num_contig(mm, addr, ptep, &pgsize);
>> - for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
>> - ptep_set_wrprotect(mm, addr, ptep);
>> + dpfn = pgsize >> PAGE_SHIFT;
>> +
>> + orig_pte = get_clear_flush(mm, addr, ptep, pgsize, ncontig);
>
> Can you not use just set pte here instead of deriving it from *ptep
> early on?
>
> pte = get_clear_flush(mm, addr, ptep, pgsize, ncontig);
> pte = pte_wrprotect(pte);
>
I've simplified this locally now.
I'll run through a few tests and post a new version.
Thanks for review.
Punit
>> + if (pte_dirty(orig_pte))
>> + pte = pte_mkdirty(pte);
>> +
>> + hugeprot = pte_pgprot(pte);
>> + for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn)
>> + set_pte_at(mm, addr, ptep, pfn_pte(pfn, hugeprot));
>> }
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
