Re: Potential race in TLB flush batching?

[Minchan Kim <minchan@kernel.org>]

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.

> 
> > In above example, I think KSM should flush the TLB, not MADV_FREE and
> > soft dirty page hander.
> > 
> 
> That would also be acceptable.
> 
> > > flushing the full affected range as madvise_free holds mmap_sem for
> > > read-only to avoid problems with two parallel madv_free operations. The
> > > second is optional because there are other ways it could also be handled
> > > that may have lower overhead.
> > 
> > Ditto. I cannot understand. Why does two parallel MADV_FREE have a problem?
> > 
> 
> 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?

Thanks.

> this matters considering that madv_free to a region means that parallel
> writers can lose their update anyway. It's less of a concern than the
> KSM angle outlined in Nadav's example which he was able to artifically
> reproduce by slowing operations to increase the race window.
> 
> -- 
> Mel Gorman
> SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>