On Tue, Sep 26, 2023 at 03:52:19PM +0000, Shameerali Kolothum Thodi wrote: > From: Catalin Marinas [mailto:catalin.marinas@xxxxxxx] > > On Mon, Sep 25, 2023 at 08:04:39AM +0000, Shameerali Kolothum Thodi wrote: > > > From: Oliver Upton [mailto:oliver.upton@xxxxxxxxx] > > > > In both cases the 'old' translation should have DBM cleared. Even if the > > > > PTE were dirty, this is wasted work since we need to do a final scan of > > > > the stage-2 when userspace collects the dirty log. > > > > > > > > Am I missing something? > > > > > > I think we can get rid of the above mark_page_dirty(). I will test it to confirm > > > we are not missing anything here. > > > > Is this the case for the other places of mark_page_dirty() in your > > patches? If stage2_pte_writeable() is true, it must have been made > > writeable earlier by a fault and the underlying page marked as dirty. > > > > One of the other place we have mark_page_dirty() is in the stage2_unmap_walker(). > And during the testing of this series, I have tried to remove that and > found out that it actually causes memory corruption during VM migration. > > From my old debug logs: > > [ 399.288076] stage2_unmap_walker+0x270/0x284 > [ 399.288078] __kvm_pgtable_walk+0x1ec/0x2cc > [ 399.288081] __kvm_pgtable_walk+0xec/0x2cc > [ 399.288084] __kvm_pgtable_walk+0xec/0x2cc > [ 399.288086] kvm_pgtable_walk+0xcc/0x160 > [ 399.288088] kvm_pgtable_stage2_unmap+0x4c/0xbc > [ 399.288091] stage2_apply_range+0xd0/0xec > [ 399.288094] __unmap_stage2_range+0x2c/0x60 > [ 399.288096] kvm_unmap_gfn_range+0x30/0x48 > [ 399.288099] kvm_mmu_notifier_invalidate_range_start+0xe0/0x264 > [ 399.288103] __mmu_notifier_invalidate_range_start+0xa4/0x23c > [ 399.288106] change_protection+0x638/0x900 > [ 399.288109] change_prot_numa+0x64/0xfc > [ 399.288113] task_numa_work+0x2ac/0x450 > [ 399.288117] task_work_run+0x78/0xd0 > > It looks like that the unmap path gets triggered from Numa page migration code > path, so we may need it there. I think I confused things (should have looked at the code). mark_page_dirty() is actually about marking the bitmap for dirty tracking rather than the actual struct page. The latter is hidden somewhere deep down the get_user_pages() code. So yeah, I think we still need mark_page_dirty() in some places as to avoid losing the dirty information with DBM being turned on. -- Catalin
