Messages in this thread | | | Subject | Re: [PATCH 1/2] mm/mprotect: use mmu_gather | From | Nadav Amit <> | Date | Sun, 10 Oct 2021 20:45:17 -0700 |
| |
> On Sep 25, 2021, at 1:54 PM, Nadav Amit <nadav.amit@gmail.com> wrote: > > From: Nadav Amit <namit@vmware.com> > > change_pXX_range() currently does not use mmu_gather, but instead > implements its own deferred TLB flushes scheme. This both complicates > the code, as developers need to be aware of different invalidation > schemes, and prevents opportunities to avoid TLB flushes or perform them > in finer granularity. > > Use mmu_gather in change_pXX_range(). As the pages are not released, > only record the flushed range using tlb_flush_pXX_range().
Andrea pointed out that I do not take care of THP. Actually, there is indeed a missing TLB flush on THP, but it is not required due to the pmdp_invalidate(). Anyhow, the patch needs to address it cleanly, and to try to avoid the flush on pmdp_invalidate(), which at least on x86 does not appear to be necessary.
There is an additional bug, as tlb_change_page_size() needs to be called.
-- Jerome,
While I am reviewing my (bad) code, I wanted to understand whether update of migration entries requires a TLB flush, because I do not think I got that right either.
I thought they should not, but I now am not very sure. I am very confused by the following code in migrate_vma_collect_pmd():
pte_unmap_unlock(ptep - 1, ptl);
/* Only flush the TLB if we actually modified any entries */ if (unmapped) flush_tlb_range(walk->vma, start, end);
According to this code flush_tlb_range() is called without the ptl. So theoretically there is a possible race:
CPU0 CPU1 ---- ---- migrate_vma_collect_pmd() set_pte_at() [ present-> non-present]
pte_unmap_unlock()
madvise(MADV_DONTNEED) zap_pte_range()
[ PTE non-present => no flush ]
So my questions:
1. Is there a reason the above scenario is invalid? 2. Does one need to flush a migration entry he updates it?
Thanks, Nadav
| |