Re: [v3 2/3] mm: Defer TLB flush by keeping both src and dst folios at migration

[Byungchul Park <byungchul@sk.com>]

On Thu, Nov 09, 2023 at 10:16:57AM +0000, Nadav Amit wrote:
> 
> 
> > On Nov 8, 2023, at 6:12 AM, Byungchul Park <byungchul@sk.com> wrote:
> > 
> > !! External Email
> > 
> > On Mon, Oct 30, 2023 at 09:51:30PM +0900, Byungchul Park wrote:
> >>>> diff --git a/mm/memory.c b/mm/memory.c
> >>>> index 6c264d2f969c..75dc48b6e15f 100644
> >>>> --- a/mm/memory.c
> >>>> +++ b/mm/memory.c
> >>>> @@ -3359,6 +3359,19 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
> >>>>  if (vmf->page)
> >>>>          folio = page_folio(vmf->page);
> >>>> 
> >>>> + /*
> >>>> +  * This folio has its read copy to prevent inconsistency while
> >>>> +  * deferring TLB flushes. However, the problem might arise if
> >>>> +  * it's going to become writable.
> >>>> +  *
> >>>> +  * To prevent it, give up the deferring TLB flushes and perform
> >>>> +  * TLB flush right away.
> >>>> +  */
> >>>> + if (folio && migrc_pending_folio(folio)) {
> >>>> +         migrc_unpend_folio(folio);
> >>>> +         migrc_try_flush_free_folios(NULL);
> >>> 
> >>> So many potential function calls… Probably they should have been combined
> >>> into one and at least migrc_pending_folio() should have been an inline
> >>> function in the header.
> >> 
> >> I will try to change it as you mention.
> >> 
> >>>> + }
> >>>> +
> >>> 
> >>> What about mprotect? I thought David has changed it so it can set writable
> >>> PTEs.
> >> 
> >> I will check it out.
> > 
> > I found mprotect stuff is already performing TLB flushes needed for it.
> > So some redundant TLB flushes might happen by migrc but it's not that
> > harmful I think. Thanks.
> 
> Let me explain the scenario I am concerned with. Assume page P is RO, and
> moves from Psrc to Pdst. Pointer “p” points to P. Initially (*p == 0).
> 
> Let’s also assume we also have an atomic variable “a”. Initially (a == 0).
> 
> I hope I got the migration function names right, but I hope the problem
> itself can be clear regardless. 
> 
> CPU0			CPU1			CPU2		CPU3
> ----			----			----		----
> 			(user-mode)		(user-mode)		
> 
> 			Access *p
> 			[Psrc cached in TLB]
>  
> migrate_pages_batch()
> -> migrate_folio_unmap()
> 
> [ PTE updated, 
>   still no flush ]
> 
> 								mprotect(p,
> 									RW)

Here,

mprotect()
   do_mprotect_pkey()
      tlb_finish_mmu()
         tlb_flush_mmu()

I thought TLB flush for mprotect() is performed by tlb_flush_mmu() so
any cached TLB entries on other CPUs can have chance to update. Could
you correct me if I get it wrong? Thanks.

	Byungchul

> 
> 								[ Psrc is
> 								  RW ]
> 
> 								[ flush
> 								  deferred]
> 
> 
> 						*p = 1  # Pdst
> 						
> 						xchg(&a, 1)
> 			mfence
> 			if (a == 1)
> 			  assert(*p == 1);
> 
> 
> 				
> Now at this point the assertion might fail. CPU2 wrote into Pdst, whereas
> CPU1 reads from Psrc. But based on x86 memory model, userspace might not
> expect this scenario to be possible, hence leading to bugs.