huang ying <huang.ying.caritas@xxxxxxxxx> writes: > On Tue, Aug 16, 2022 at 10:33 AM Alistair Popple <apopple@xxxxxxxxxx> wrote: >> >> >> huang ying <huang.ying.caritas@xxxxxxxxx> writes: >> >> > Hi, Alistair, >> > >> > On Fri, Aug 12, 2022 at 1:23 PM Alistair Popple <apopple@xxxxxxxxxx> wrote: > > [snip] > >> >> > And I don't know why we use ptep_get_and_clear() to clear PTE if >> > (!anon_exclusive). Why don't we need to flush the TLB? >> >> We do the TLB flush at the end if anything was modified: >> >> /* Only flush the TLB if we actually modified any entries */ >> if (unmapped) >> flush_tlb_range(walk->vma, start, end); >> >> Obviously I don't think that will work correctly now given we have to >> read the dirty bits and clear the PTE atomically. I assume it was >> originally written this way for some sort of performance reason. > > Thanks for pointing this out. If there were parallel page table > operations such as mprotect() or munmap(), the delayed TLB flushing > mechanism here may have some problem. Please take a look at the > comments of flush_tlb_batched_pending() and TLB flush batching > implementation in try_to_unmap_one(). We may need to flush TLB with > page table lock held or use a mechanism similar to that in > try_to_unmap_one(). Thanks for the pointers. I agree there is likely also a problem here with the delayed TLB flushing. v2 of this patch deals with this by always flushing the TLB using ptep_flush_clear(), similar to how try_to_migrate_one() works. It looks like it could be worth investigating using batched TLB flushing for both this and try_to_migrate(), but I will leave that for a future optimisation. > Best Regards, > Huang, Ying
