Received-SPF: pass (google.com: domain of linux-kernel+bounces-209550-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 11 Jun 2024 18:12:14 +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: <20240611091214.GA16469@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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d650c29b-129f-4fac-9a9d-ea1fbdae2c3a@intel.com>
User-Agent: Mutt/1.9.4 (2018-02-28)

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.
> 
> > 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.
> 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
> mode that enforced a rule that said:
> 
>   1. A LUF'd PTE can't be rewritten until after a luf_flush() occurs
>   2. A LUF'd page's position in the page cache can't be replaced until
>      after a luf_flush()

I'm thinking a debug mode doing the following *pseudo* code - check the
logic only since the grammer might be wrong:

   0-a) Introduce new fields in page_ext:

	#ifdef LUF_DEBUG
	struct list_head __percpu luf_node;
	#endif

   0-b) Introduce new fields in struct address_space:

	#ifdef LUF_DEBUG
	struct list_head __percpu luf_node;
	#endif

   0-c) Introduce new fields in struct task_struct:

	#ifdef LUF_DEBUG
	cpumask_t luf_pending_cpus;
	#endif

   0-d) Define percpu list_head to link luf'd folios:

	#ifdef LUF_DEBUG
	DEFINE_PER_CPU(struct list_head, luf_folios);
	DEFINE_PER_CPU(struct list_head, luf_address_spaces);
	#endif

   1) When skipping tlb flush in reclaim or migration for a folio:

	#ifdef LUF_DEBUG
	ext = get_page_ext_for_luf_debug(folio);
	as = folio_mapping(folio);

	for_each_cpu(cpu, skip_cpus) {
		list_add(per_cpu_ptr(ext->luf_node, cpu),
			 per_cpu_ptr(luf_folios, cpu));
		if (as)
			list_add(per_cpu_ptr(as->luf_node, cpu),
				 per_cpu_ptr(luf_address_spaces, cpu));
	}
	put_page_ext(ext);
	#endif

   2) When performing tlb flush in try_to_unmap_flush():
      Remind luf only works on unmapping during reclaim and migration.

	#ifdef LUF_DEBUG
	for_each_cpu(cpu, now_flushing_cpus) {
		for_each_node_safe(folio, per_cpu_ptr(luf_folios)) {
			ext = get_page_ext_for_luf_debug(folio);
			list_del_init(per_cpu_ptr(ext->luf_node, cpu))
			put_page_ext(ext);
		}

		for_each_node_safe(as, per_cpu_ptr(luf_address_spaces))
			list_del_init(per_cpu_ptr(as->luf_node, cpu))

		cpumask_clear_cpu(cpu, current->luf_pending_cpus);
	}
	#endif

   3) In pte_mkwrite():

	#ifdef LUF_DEBUG
	ext = get_page_ext_for_luf_debug(folio);

	for_each_cpu(cpu, online_cpus)
		if (!list_empty(per_cpu_ptr(ext->luf_node, cpu)))
			cpumask_set_cpu(cpu, current->luf_pending_cpus);
	put_page_ext(ext);
	#endif

   4) On returning to user:

	#ifdef LUF_DEBUG
	WARN_ON(!cpumask_empty(current->luf_pending_cpus));
	#endif

   5) On right after every a_ops->write_end() call:

	#ifdef LUF_DEBUG
	as = get_address_space_to_write_to();
	for_each_cpu(cpu, online_cpus)
		if (!list_empty(per_cpu_ptr(as->luf_node, cpu)))
			cpumask_set_cpu(cpu, current->luf_pending_cpus);
	#endif

	luf_flush_or_its_optimized_version();

	#ifdef LUF_DEBUG
	WARN_ON(!cpumask_empty(current->luf_pending_cpus));
	#endif

I will implement the debug mode this way with all serialized.  Do you
think it works for what we want?

	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.