Received-SPF: pass (google.com: domain of linux-kernel+bounces-200023-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 4 Jun 2024 13:43:48 +0900
From: Byungchul Park <byungchul@sk.com>
To: Dave Hansen <dave.hansen@intel.com>
Cc: David Hildenbrand <david@redhat.com>,
	Byungchul Park <lkml.byungchul.park@gmail.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	kernel_team@skhynix.com, akpm@linux-foundation.org,
	ying.huang@intel.com, vernhao@tencent.com,
	mgorman@techsingularity.net, hughd@google.com, willy@infradead.org,
	peterz@infradead.org, luto@kernel.org, tglx@linutronix.de,
	mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
	rjgolo@gmail.com
Subject: Re: [PATCH v11 09/12] mm: implement LUF(Lazy Unmap Flush) defering
 tlb flush when folios get unmapped
Message-ID: <20240604044348.GA40889@system.software.com>
References: <20240531092001.30428-1-byungchul@sk.com>
 <20240531092001.30428-10-byungchul@sk.com>
 <fab1dd64-c652-4160-93b4-7b483a8874da@intel.com>
 <CAHyrMpxETdVewTH3MCS4qPyD6Xf1zRUfWZf-8SCdpCFj2Pj_Wg@mail.gmail.com>
 <f17f33e8-1c1f-460f-8c5a-713476f524a3@intel.com>
 <26dc4594-430b-483c-a26c-7e68bade74b0@redhat.com>
 <20240603093505.GA12549@system.software.com>
 <d650c29b-129f-4fac-9a9d-ea1fbdae2c3a@intel.com>
 <20240604015348.GB26609@system.software.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240604015348.GB26609@system.software.com>
User-Agent: Mutt/1.9.4 (2018-02-28)

On Tue, Jun 04, 2024 at 10:53:48AM +0900, Byungchul Park wrote:
> On Mon, Jun 03, 2024 at 06:23:46AM -0700, Dave Hansen wrote:
> > On 6/3/24 02:35, Byungchul Park wrote:
> > ...> In luf's point of view, the points where the deferred flush should be
> > > performed are simply:
> > > 
> > > 	1. when changing the vma maps, that might be luf'ed.
> > > 	2. when updating data of the pages, that might be luf'ed.
> > 
> > It's simple, but the devil is in the details as always.
> 
> Agree with that.
> 
> > > All we need to do is to indentify the points:
> > > 
> > > 	1. when changing the vma maps, that might be luf'ed.
> > > 
> > > 	   a) mmap and munmap e.i. fault handler or unmap_region().
> > > 	   b) permission to writable e.i. mprotect or fault handler.
> > > 	   c) what I'm missing.
> > 
> > I'd say it even more generally: anything that installs a PTE which is
> > inconsistent with the original PTE.  That, of course, includes writes.
> > But it also includes crazy things that we do like uprobes.  Take a look
> > at __replace_page().
> > 
> > I think the page_vma_mapped_walk() checks plus the ptl keep LUF at bay
> > there.  But it needs some really thorough review.
> > 
> > But the bigger concern is that, if there was a problem, I can't think of
> > a systematic way to find it.
> > 
> > > 	2. when updating data of the pages, that might be luf'ed.
> > > 
> > > 	   a) updating files through vfs e.g. file_end_write().
> > > 	   b) updating files through writable maps e.i. 1-a) or 1-b).
> > > 	   c) what I'm missing.
> > 
> > Filesystems or block devices that change content without a "write" from
> > the local system.  Network filesystems and block devices come to mind.
> 
> AFAIK, every network filesystem eventully "updates" its connected local
> filesystem.  It could be still handled at the point where updating the
> local file system.
> 
> > I honestly don't know what all the rules are around these, but they
> > could certainly be troublesome.
> > 
> > There appear to be some interactions for NFS between file locking and
> > page cache flushing.
> > 
> > But, stepping back ...
> > 
> > I'd honestly be a lot more comfortable if there was even a debugging LUF
> 
> I'd better provide a method for better debugging.  Lemme know whatever
> it is we need.
> 
> > mode that enforced a rule that said:

Do you means a debugging mode that can WARN or inform the situation that
we don't want?  If yes, sure.  Now that I get this, I will re-read all
you guys' talk.

	Byungchul

> Why "debugging mode"?  The following rules should be enforced always.
> 
> >   1. A LUF'd PTE can't be rewritten until after a luf_flush() occurs
> 
> "luf_flush() should be followed when.." is more correct because
> "luf_flush() -> another luf -> the pte gets rewritten" can happen.  So
> it should be "the pte gets rewritten -> another luf by any chance ->
> luf_flush()", that is still safe.
> 
> >   2. A LUF'd page's position in the page cache can't be replaced until
> >      after a luf_flush()
> 
> "luf_flush() should be followed when.." is more correct too.
> 
> These two rules are exactly same as what I described but more specific.
> I like your way to describe the rules.
> 
> 	Byungchul
> 
> > or *some* other independent set of rules that can tell us when something
> > goes wrong.  That uprobes code, for instance, seems like it will work.
> > But I can also imagine writing it ten other ways where it would break
> > when combined with LUF.