On Tue, Jul 25, 2017 at 06:11:15PM +0900, Minchan Kim wrote: > On Tue, Jul 25, 2017 at 09:51:32AM +0100, Mel Gorman wrote: > > On Tue, Jul 25, 2017 at 04:37:48PM +0900, Minchan Kim wrote: > > > > Ok, as you say you have reproduced this with corruption, I would suggest > > > > one path for dealing with it although you'll need to pass it by the > > > > original authors. > > > > > > > > When unmapping ranges, there is a check for dirty PTEs in > > > > zap_pte_range() that forces a flush for dirty PTEs which aims to avoid > > > > writable stale PTEs from CPU0 in a scenario like you laid out above. > > > > > > > > madvise_free misses a similar class of check so I'm adding Minchan Kim > > > > to the cc as the original author of much of that code. Minchan Kim will > > > > need to confirm but it appears that two modifications would be required. > > > > The first should pass in the mmu_gather structure to > > > > madvise_free_pte_range (at minimum) and force flush the TLB under the > > > > PTL if a dirty PTE is encountered. The second is that it should consider > > > > > > OTL: I couldn't read this lengthy discussion so I miss miss something. > > > > > > About MADV_FREE, I do not understand why it should flush TLB in MADV_FREE > > > context. MADV_FREE's semantic allows "write(ie, dirty)" so if other thread > > > in parallel which has stale pte does "store" to make the pte dirty, > > > it's okay since try_to_unmap_one in shrink_page_list catches the dirty. > > > > > > > In try_to_unmap_one it's fine. It's not necessarily fine in KSM. Given > > that the key is that data corruption is avoided, you could argue with a > > comment that madv_free doesn't necesssarily have to flush it as long as > > KSM does even if it's clean due to batching. > > Yes, I think it should be done in side where have a concern. > Maybe, mm_struct can carry a flag which indicates someone is > doing the TLB bacthing and then KSM side can flush it by the flag. > It would reduce unncessary flushing. > If you're confident that it's only necessary on the KSM side to avoid the problem then I'm ok with that. Update KSM in that case with a comment explaining the madv_free race and why the flush is unconditionally necessary. madv_free only came up because it was a critical part of having KSM miss a TLB flush. > > Like madvise(), madv_free can potentially return with a stale PTE visible > > to the caller that observed a pte_none at the time of madv_free and uses > > a stale PTE that potentially allows a lost write. It's debatable whether > > That is the part I cannot understand. > How does it lost "the write"? MADV_FREE doesn't discard the memory so > finally, the write should be done sometime. > Could you tell me more? > I'm relying on the fact you are the madv_free author to determine if it's really necessary. The race in question is CPU 0 running madv_free and updating some PTEs while CPU 1 is also running madv_free and looking at the same PTEs. CPU 1 may have writable TLB entries for a page but fail the pte_dirty check (because CPU 0 has updated it already) and potentially fail to flush. Hence, when madv_free on CPU 1 returns, there are still potentially writable TLB entries and the underlying PTE is still present so that a subsequent write does not necessarily propagate the dirty bit to the underlying PTE any more. Reclaim at some unknown time at the future may then see that the PTE is still clean and discard the page even though a write has happened in the meantime. I think this is possible but I could have missed some protection in madv_free that prevents it happening. -- Mel Gorman SUSE Labs -- 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>
