Re: [PATCH v3 3/7] mm: Add write-protect and clean utilities for address space ranges

From: Linus Torvalds <torvalds@linux-foundation.org>
To: "Thomas Hellström (VMware)" <thomas_os@shipmail.org>
Cc: Linux-MM <linux-mm@kvack.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Thomas Hellstrom" <thellstrom@vmware.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Matthew Wilcox" <willy@infradead.org>,
	"Will Deacon" <will.deacon@arm.com>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Rik van Riel" <riel@surriel.com>,
	"Minchan Kim" <minchan@kernel.org>,
	"Michal Hocko" <mhocko@suse.com>,
	"Huang Ying" <ying.huang@intel.com>,
	"Jérôme Glisse" <jglisse@redhat.com>,
	"Kirill A . Shutemov" <kirill@shutemov.name>
Subject: Re: [PATCH v3 3/7] mm: Add write-protect and clean utilities for address space ranges
Date: Wed, 2 Oct 2019 13:27:41 -0700	[thread overview]
Message-ID: <CAHk-=wgH=T5mcDxTaC6QbBN=iJD3d_amzcb2+GxbcV7XuEYL2A@mail.gmail.com> (raw)
In-Reply-To: <516814a5-a616-b15e-ac87-26ede681da85@shipmail.org>

On Wed, Oct 2, 2019 at 12:09 PM Thomas Hellström (VMware)
<thomas_os@shipmail.org> wrote:
>
> Yes I typically tend towards using a "namespace_object_operation" naming
> scheme, with "as_dirty" being the namespace here,

We discourage that kind of mindless namespacing for core stuff.

It makes sense in a driver or a filesystem: when there are 20+
different filesystems all implementing the same operation, you can't
have descriptive and natural names that are unique. So having a
namespace prefix for the filesystem or driver makes sense. But even
then it tends ot be just a simple name, not the op it does.

> Looking at Matthew's suggestion but lining up with
> "unmap_mapping_range()", perhaps we could use "clean_mapping_range" and
> "wp_mapping_range"?

Yes, I agree with Willy that "dirty" is kind of implicit when the
operation is to clean something, so the above sounds sane to me.

The wp part I'm not entirely sure about: you're not actually
write-protecting the range. You're doing something different. You're
only doing it for shared writable mappings. And I'd rather see
separate 'struct mm_walk_ops' than shared ones that then have a flag
in a differfent structure to change behavior.

Yes, yes, some of the levels share the same logic, but that just means
that you can use the same function pointer for that level in the
different "clean" vs "wp" mm_walk_op.

Also, looking closer at this patch, this makes me go "Whaa?":

+       pte = (mm == &init_mm) ?
+               pte_offset_kernel(pmd, addr) :
+               pte_offset_map_lock(mm, pmd, addr, &ptl);

because I don't think that's sensible. When do you have a vma in kernel space?

Also, why do you loop inside the pmd_entry function, instead of just
having a pte_entry function?

           Linus