[RFC PATCH 0/5] Add hugetlb soft dirty support

[RFC PATCH 0/5] Add hugetlb soft dirty support

[Mike Kravetz]

Shu Wang opened bugzilla 211287 [1] saying,
When a memory region is mapped with huge pages, the softdirty bit is set
only right after the huge pages is mapped. After the memory mapping, if
the softdirty bit is cleared and the memory is written again, the softdirty
bit is not set when reading from the process's pagemap.

Their use case for hugetlb soft dirty is,
We maps both PMEM and hugetlb DRAM to process’s address space to provide
large amount of memory to the application. Periodically, we clear the soft
dirty bit by writing ‘4’ to the clear_refs. Then, we check the page’s soft
dirty bit from pagemap and use this information to track write activity to
memory. We migrate the data between DRAM and PMEM based the write activity
for better performance.

A quick check of the code revealed,
The /proc clear_refs code handles THP pages and 'normal' pages.  There is
no check/processing for hugetlb pages.  All the code will do is clear the
VM_SOFTDIRTY vma flag and vma_set_page_prot.  None of the individual ptes
or page flags are touched.

The only soft dirty checking done by the /proc pagemap code is at the vma
level.  It checks the VM_SOFTDIRTY flag.   It does not look for soft dirty
on the individual ptes.

That explains the behavior described in the bugzilla report.

This series adds soft dirty support for hugetlb.  It has only passed
some very basic testing and likely needs more work.  However, Axel
Rasmussen's "userfaultfd: add minor fault handling" and Peter Xu's
"userfaultfd-wp: Support shmem and hugetlbfs" work touch the same areas
and have similiar issues.  Therefore, code is being sent earlier than
normal so that efforts in common areas can be coordinated.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=211287

Mike Kravetz (5):
  hugetlb: add hugetlb helpers for soft dirty support
  hugetlb: enhance hugetlb fault processing to support soft dirty
  mm proc/task_mmu.c: add soft dirty pte checks for hugetlb
  hugetlb: don't permit pmd sharing if soft dirty in use
  mm proc/task_mmu.c: add hugetlb specific routine for clear_refs

 arch/s390/include/asm/hugetlb.h |  30 +++++++++
 fs/proc/task_mmu.c              | 114 ++++++++++++++++++++++++++++++++
 include/asm-generic/hugetlb.h   |  30 +++++++++
 include/linux/hugetlb.h         |   1 +
 mm/hugetlb.c                    |  40 ++++++-----
 5 files changed, 199 insertions(+), 16 deletions(-)

-- 
2.29.2

[RFC PATCH 1/5] hugetlb: add hugetlb helpers for soft dirty support

[Mike Kravetz]

Add interfaces to set and clear soft dirty in hugetlb ptes.  Make
hugetlb interfaces needed for /proc clear_refs available outside
hugetlb.c.

arch/s390 has it's own version of most routines in asm-generic/hugetlb.h,
so add new routines there as well.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 arch/s390/include/asm/hugetlb.h | 30 ++++++++++++++++++++++++++++++
 include/asm-generic/hugetlb.h   | 30 ++++++++++++++++++++++++++++++
 include/linux/hugetlb.h         |  1 +
 mm/hugetlb.c                    | 10 +---------
 4 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
index 60f9241e5e4a..b7d26248fb1c 100644
--- a/arch/s390/include/asm/hugetlb.h
+++ b/arch/s390/include/asm/hugetlb.h
@@ -105,6 +105,11 @@ static inline pte_t huge_pte_mkdirty(pte_t pte)
 	return pte_mkdirty(pte);
 }
 
+static inline pte_t huge_pte_mkyoung(pte_t pte)
+{
+	return pte_mkyoung(pte);
+}
+
 static inline pte_t huge_pte_wrprotect(pte_t pte)
 {
 	return pte_wrprotect(pte);
@@ -115,9 +120,34 @@ static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
 	return pte_modify(pte, newprot);
 }
 
+static inline bool huge_pte_soft_dirty(pte_t pte)
+{
+	return pte_soft_dirty(pte);
+}
+
+static inline pte_t huge_pte_clear_soft_dirty(pte_t pte)
+{
+	return pte_clear_soft_dirty(pte);
+}
+
+static inline pte_t huge_pte_swp_clear_soft_dirty(pte_t pte)
+{
+	return pte_swp_clear_soft_dirty(pte);
+}
+
 static inline bool gigantic_page_runtime_supported(void)
 {
 	return true;
 }
 
+#if !defined(__HAVE_ARCH_FLUSH_HUGETLB_TLB_RANGE) && !defined(MODULE)
+#include <asm/tlbflush.h>
+
+static inline void flush_hugetlb_tlb_range(struct vm_area_struct *vma,
+					unsigned long start, unsigned long end)
+{
+	flush_tlb_range(vma, start, end);
+}
+#endif
+
 #endif /* _ASM_S390_HUGETLB_H */
diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h
index 8e1e6244a89d..d8a78dab63bf 100644
--- a/include/asm-generic/hugetlb.h
+++ b/include/asm-generic/hugetlb.h
@@ -27,11 +27,31 @@ static inline pte_t huge_pte_mkdirty(pte_t pte)
 	return pte_mkdirty(pte);
 }
 
+static inline pte_t huge_pte_mkyoung(pte_t pte)
+{
+	return pte_mkyoung(pte);
+}
+
 static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
 {
 	return pte_modify(pte, newprot);
 }
 
+static inline bool huge_pte_soft_dirty(pte_t pte)
+{
+	return pte_soft_dirty(pte);
+}
+
+static inline pte_t huge_pte_clear_soft_dirty(pte_t pte)
+{
+	return pte_clear_soft_dirty(pte);
+}
+
+static inline pte_t huge_pte_swp_clear_soft_dirty(pte_t pte)
+{
+	return pte_swp_clear_soft_dirty(pte);
+}
+
 #ifndef __HAVE_ARCH_HUGE_PTE_CLEAR
 static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
 		    pte_t *ptep, unsigned long sz)
@@ -133,4 +153,14 @@ static inline bool gigantic_page_runtime_supported(void)
 }
 #endif /* __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED */
 
+#if !defined(__HAVE_ARCH_FLUSH_HUGETLB_TLB_RANGE) && !defined(MODULE)
+#include <asm/tlbflush.h>
+
+static inline void flush_hugetlb_tlb_range(struct vm_area_struct *vma,
+					unsigned long start, unsigned long end)
+{
+	flush_tlb_range(vma, start, end);
+}
+#endif
+
 #endif /* _ASM_GENERIC_HUGETLB_H */
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index b5807f23caf8..7b6c35c5df99 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -187,6 +187,7 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
 		unsigned long address, unsigned long end, pgprot_t newprot);
 
 bool is_hugetlb_entry_migration(pte_t pte);
+bool is_hugetlb_entry_hwpoisoned(pte_t pte);
 
 #else /* !CONFIG_HUGETLB_PAGE */
 
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 4bdb58ab14cb..47f3123afd1a 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3759,7 +3759,7 @@ bool is_hugetlb_entry_migration(pte_t pte)
 		return false;
 }
 
-static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
+bool is_hugetlb_entry_hwpoisoned(pte_t pte)
 {
 	swp_entry_t swp;
 
@@ -4965,14 +4965,6 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	return i ? i : err;
 }
 
-#ifndef __HAVE_ARCH_FLUSH_HUGETLB_TLB_RANGE
-/*
- * ARCHes with special requirements for evicting HUGETLB backing TLB entries can
- * implement this.
- */
-#define flush_hugetlb_tlb_range(vma, addr, end)	flush_tlb_range(vma, addr, end)
-#endif
-
 unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
 		unsigned long address, unsigned long end, pgprot_t newprot)
 {
-- 
2.29.2

Re: [RFC PATCH 1/5] hugetlb: add hugetlb helpers for soft dirty support

[Peter Xu]

On Wed, Feb 10, 2021 at 04:03:18PM -0800, Mike Kravetz wrote:
> Add interfaces to set and clear soft dirty in hugetlb ptes.  Make
> hugetlb interfaces needed for /proc clear_refs available outside
> hugetlb.c.
> 
> arch/s390 has it's own version of most routines in asm-generic/hugetlb.h,
> so add new routines there as well.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  arch/s390/include/asm/hugetlb.h | 30 ++++++++++++++++++++++++++++++
>  include/asm-generic/hugetlb.h   | 30 ++++++++++++++++++++++++++++++
>  include/linux/hugetlb.h         |  1 +
>  mm/hugetlb.c                    | 10 +---------
>  4 files changed, 62 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
> index 60f9241e5e4a..b7d26248fb1c 100644
> --- a/arch/s390/include/asm/hugetlb.h
> +++ b/arch/s390/include/asm/hugetlb.h
> @@ -105,6 +105,11 @@ static inline pte_t huge_pte_mkdirty(pte_t pte)
>  	return pte_mkdirty(pte);
>  }
>  
> +static inline pte_t huge_pte_mkyoung(pte_t pte)
> +{
> +	return pte_mkyoung(pte);
> +}
> +
>  static inline pte_t huge_pte_wrprotect(pte_t pte)
>  {
>  	return pte_wrprotect(pte);
> @@ -115,9 +120,34 @@ static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
>  	return pte_modify(pte, newprot);
>  }
>  
> +static inline bool huge_pte_soft_dirty(pte_t pte)
> +{
> +	return pte_soft_dirty(pte);
> +}
> +
> +static inline pte_t huge_pte_clear_soft_dirty(pte_t pte)
> +{
> +	return pte_clear_soft_dirty(pte);
> +}
> +
> +static inline pte_t huge_pte_swp_clear_soft_dirty(pte_t pte)
> +{
> +	return pte_swp_clear_soft_dirty(pte);
> +}
> +

Indeed asm/hugetlb.h of s390 didn't include asm-generic/hugetlb.h as what was
normally done by asm/hugetlb.h of other archs.  Do you know why it's special?
E.g. huge_pte_wrprotect() of s390 version is actually the same of the default
version.

When I looked at the huge_pte_wrprotect() I also see that there seems to have
no real user of __HAVE_ARCH_HUGE_PTE_WRPROTECT.  Not sure whether it can be
dropped.  My gut feeling is that s390 should also include asm-generic/hugetlb.h
but only redefine the helper only if necessary, since I see no point defining
the same helper multiple times.

>  static inline bool gigantic_page_runtime_supported(void)
>  {
>  	return true;
>  }
>  
> +#if !defined(__HAVE_ARCH_FLUSH_HUGETLB_TLB_RANGE) && !defined(MODULE)
> +#include <asm/tlbflush.h>
> +
> +static inline void flush_hugetlb_tlb_range(struct vm_area_struct *vma,
> +					unsigned long start, unsigned long end)
> +{
> +	flush_tlb_range(vma, start, end);
> +}
> +#endif

Similar question here, only ppc defined __HAVE_ARCH_FLUSH_HUGETLB_TLB_RANGE, so
IIUC it means s390 should simply use the default version, and it'll be great if
we don't need to redefine it here.

Thanks,

-- 
Peter Xu

Re: [RFC PATCH 1/5] hugetlb: add hugetlb helpers for soft dirty support

[Mike Kravetz]

On 2/17/21 8:24 AM, Peter Xu wrote:
> On Wed, Feb 10, 2021 at 04:03:18PM -0800, Mike Kravetz wrote:
>> Add interfaces to set and clear soft dirty in hugetlb ptes.  Make
>> hugetlb interfaces needed for /proc clear_refs available outside
>> hugetlb.c.
>>
>> arch/s390 has it's own version of most routines in asm-generic/hugetlb.h,
>> so add new routines there as well.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  arch/s390/include/asm/hugetlb.h | 30 ++++++++++++++++++++++++++++++
>>  include/asm-generic/hugetlb.h   | 30 ++++++++++++++++++++++++++++++
>>  include/linux/hugetlb.h         |  1 +
>>  mm/hugetlb.c                    | 10 +---------
>>  4 files changed, 62 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
>> index 60f9241e5e4a..b7d26248fb1c 100644
>> --- a/arch/s390/include/asm/hugetlb.h
>> +++ b/arch/s390/include/asm/hugetlb.h
>> @@ -105,6 +105,11 @@ static inline pte_t huge_pte_mkdirty(pte_t pte)
>>  	return pte_mkdirty(pte);
>>  }
>>  
>> +static inline pte_t huge_pte_mkyoung(pte_t pte)
>> +{
>> +	return pte_mkyoung(pte);
>> +}
>> +
>>  static inline pte_t huge_pte_wrprotect(pte_t pte)
>>  {
>>  	return pte_wrprotect(pte);
>> @@ -115,9 +120,34 @@ static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
>>  	return pte_modify(pte, newprot);
>>  }
>>  
>> +static inline bool huge_pte_soft_dirty(pte_t pte)
>> +{
>> +	return pte_soft_dirty(pte);
>> +}
>> +
>> +static inline pte_t huge_pte_clear_soft_dirty(pte_t pte)
>> +{
>> +	return pte_clear_soft_dirty(pte);
>> +}
>> +
>> +static inline pte_t huge_pte_swp_clear_soft_dirty(pte_t pte)
>> +{
>> +	return pte_swp_clear_soft_dirty(pte);
>> +}
>> +
> 
> Indeed asm/hugetlb.h of s390 didn't include asm-generic/hugetlb.h as what was
> normally done by asm/hugetlb.h of other archs.  Do you know why it's special?
> E.g. huge_pte_wrprotect() of s390 version is actually the same of the default
> version.
> 
> When I looked at the huge_pte_wrprotect() I also see that there seems to have
> no real user of __HAVE_ARCH_HUGE_PTE_WRPROTECT.  Not sure whether it can be
> dropped.  My gut feeling is that s390 should also include asm-generic/hugetlb.h
> but only redefine the helper only if necessary, since I see no point defining
> the same helper multiple times.

I do not know why s390 is special in this way.  However, I did cc some s390
people and the list.  Perhaps they know?

> 
>>  static inline bool gigantic_page_runtime_supported(void)
>>  {
>>  	return true;
>>  }
>>  
>> +#if !defined(__HAVE_ARCH_FLUSH_HUGETLB_TLB_RANGE) && !defined(MODULE)
>> +#include <asm/tlbflush.h>
>> +
>> +static inline void flush_hugetlb_tlb_range(struct vm_area_struct *vma,
>> +					unsigned long start, unsigned long end)
>> +{
>> +	flush_tlb_range(vma, start, end);
>> +}
>> +#endif
> 
> Similar question here, only ppc defined __HAVE_ARCH_FLUSH_HUGETLB_TLB_RANGE, so
> IIUC it means s390 should simply use the default version, and it'll be great if
> we don't need to redefine it here.

Actually, your patch "mm/hugetlb: Move flush_hugetlb_tlb_range() into
hugetlb.h" makes this change unnecessary.  But, the question about ppc
remains.
-- 
Mike Kravetz

Re: [RFC PATCH 1/5] hugetlb: add hugetlb helpers for soft dirty support

[Gerald Schaefer]

On Wed, 17 Feb 2021 11:24:15 -0500
Peter Xu <peterx@redhat.com> wrote:

> On Wed, Feb 10, 2021 at 04:03:18PM -0800, Mike Kravetz wrote:
> > Add interfaces to set and clear soft dirty in hugetlb ptes.  Make
> > hugetlb interfaces needed for /proc clear_refs available outside
> > hugetlb.c.
> > 
> > arch/s390 has it's own version of most routines in asm-generic/hugetlb.h,
> > so add new routines there as well.
> > 
> > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> > ---
> >  arch/s390/include/asm/hugetlb.h | 30 ++++++++++++++++++++++++++++++
> >  include/asm-generic/hugetlb.h   | 30 ++++++++++++++++++++++++++++++
> >  include/linux/hugetlb.h         |  1 +
> >  mm/hugetlb.c                    | 10 +---------
> >  4 files changed, 62 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
> > index 60f9241e5e4a..b7d26248fb1c 100644
> > --- a/arch/s390/include/asm/hugetlb.h
> > +++ b/arch/s390/include/asm/hugetlb.h
> > @@ -105,6 +105,11 @@ static inline pte_t huge_pte_mkdirty(pte_t pte)
> >  	return pte_mkdirty(pte);
> >  }
> >  
> > +static inline pte_t huge_pte_mkyoung(pte_t pte)
> > +{
> > +	return pte_mkyoung(pte);
> > +}
> > +
> >  static inline pte_t huge_pte_wrprotect(pte_t pte)
> >  {
> >  	return pte_wrprotect(pte);
> > @@ -115,9 +120,34 @@ static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
> >  	return pte_modify(pte, newprot);
> >  }
> >  
> > +static inline bool huge_pte_soft_dirty(pte_t pte)
> > +{
> > +	return pte_soft_dirty(pte);
> > +}
> > +
> > +static inline pte_t huge_pte_clear_soft_dirty(pte_t pte)
> > +{
> > +	return pte_clear_soft_dirty(pte);
> > +}
> > +
> > +static inline pte_t huge_pte_swp_clear_soft_dirty(pte_t pte)
> > +{
> > +	return pte_swp_clear_soft_dirty(pte);
> > +}
> > +
> 
> Indeed asm/hugetlb.h of s390 didn't include asm-generic/hugetlb.h as what was
> normally done by asm/hugetlb.h of other archs.  Do you know why it's special?
> E.g. huge_pte_wrprotect() of s390 version is actually the same of the default
> version.

That is for "historical reasons", and yes, it doesn't look like it makes a lot
of sense any more.

The history part:

When s390 hugetlb support was introduced in 2008, there was no
asm-generic/hugetlb.h, and also no huge_pte_xxx primitives at all. They were
actually introduced because of s390, since the hugetlb common code did not
make any difference between pte and pmd types, see commit 7f2e9525ba55
("hugetlbfs: common code update for s390").

Back then, only few architectures with hugetlb support existed, and instead
of creating an asm-generic/hugetlb.h, I just added the primitives to the
individual arch include files.

5 years later, more huge_pte_xxx primitives were needed, and it appeared
to make sense to introduce asm-generic/hugetlb.h, see commit 106c992a5ebe
("mm/hugetlb: add more arch-defined huge_pte functions"). However, for s390,
all those primitives still needed special care, so we were / are the only
architecture not including that.

Then we fundamentally changed the way how we deal with that "hugetlb code
is treating pmds as ptes" issue. Instead of caring about that in all
huge_pte_xxx primitives, huge_ptep_get() will now return a nicely faked pte
for s390, i.e. something that looks like a pte would look like, and not the
real pmd/pud value. With that, hugetlb code can do all its pte handling on
that fake pte, and the conversion back to a proper pmd/pud is done in
set_huge_pte().

This is also why it will go very wrong on s390 if you directly look at
or manipulate a huge pte (i.e. pmd or pud) via its pointer, and not use
huge_ptep_get() and set_huge_pte().

Since that change, most of the huge_pte_xxx primitives are now the default
pte_xxx primitives also for s390, but apparently nobody thought about moving
to asm-generic/hugetlb.h.

> When I looked at the huge_pte_wrprotect() I also see that there seems to have
> no real user of __HAVE_ARCH_HUGE_PTE_WRPROTECT.  Not sure whether it can be
> dropped.  My gut feeling is that s390 should also include asm-generic/hugetlb.h
> but only redefine the helper only if necessary, since I see no point defining
> the same helper multiple times.

Your gut feeling seems right, I will look into cleaning that up. But don't
let that keep you from adding things there for now.

The __HAVE_ARCH_HUGE_PTE_WRPROTECT is not related to any s390-specifics,
I think it was already unused when it was introduced with commit c4916a008665a
("hugetlb: introduce generic version of huge_pte_wrprotect"). Maybe it was
just added for completeness or future support, because the corresponding
__HAVE_ARCH_HUGE_PTEP_SET_WRPROTECT did / does have some users.

Re: [RFC PATCH 1/5] hugetlb: add hugetlb helpers for soft dirty support

[Gerald Schaefer]

On Wed, 24 Feb 2021 17:46:08 +0100
Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote:

[...]
> Then we fundamentally changed the way how we deal with that "hugetlb code
> is treating pmds as ptes" issue. Instead of caring about that in all
> huge_pte_xxx primitives, huge_ptep_get() will now return a nicely faked pte
> for s390, i.e. something that looks like a pte would look like, and not the
> real pmd/pud value. With that, hugetlb code can do all its pte handling on
> that fake pte, and the conversion back to a proper pmd/pud is done in
> set_huge_pte().

BTW, in case anybody is wondering, this conversion from and to pmd for s390
will also care about the soft dirty bit, even though it was not really used
before for hugetlb. So Mikes approach to add the default primitives for s390
should work fine.

[RFC PATCH 2/5] hugetlb: enhance hugetlb fault processing to support soft dirty

[Mike Kravetz]

hugetlb fault processing code would COW all write faults where the
pte was not writable.  Soft dirty will write protect ptes as part
of it's tracking mechanism.  The existing hugetlb_cow  code will do
the right thing for PRIVATE mappings as it checks map_count.  However,
for SHARED mappings it would actually allocate and install a COW page.
Modify the code to not call hugetlb_cow for SHARED mappings and just
update the pte.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/hugetlb.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 47f3123afd1a..b561b6867ec1 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4584,8 +4584,10 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 	 * spinlock. For private mappings, we also lookup the pagecache
 	 * page now as it is used to determine if a reservation has been
 	 * consumed.
+	 * Only non-shared mappings are sent to hugetlb_cow.
 	 */
-	if ((flags & FAULT_FLAG_WRITE) && !huge_pte_write(entry)) {
+	if ((flags & FAULT_FLAG_WRITE) && !huge_pte_write(entry) &&
+					!(vma->vm_flags & VM_SHARED)) {
 		if (vma_needs_reservation(h, vma, haddr) < 0) {
 			ret = VM_FAULT_OOM;
 			goto out_mutex;
@@ -4593,9 +4595,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 		/* Just decrements count, does not deallocate */
 		vma_end_reservation(h, vma, haddr);
 
-		if (!(vma->vm_flags & VM_MAYSHARE))
-			pagecache_page = hugetlbfs_pagecache_page(h,
-								vma, haddr);
+		pagecache_page = hugetlbfs_pagecache_page(h, vma, haddr);
 	}
 
 	ptl = huge_pte_lock(h, mm, ptep);
@@ -4620,9 +4620,18 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 
 	if (flags & FAULT_FLAG_WRITE) {
 		if (!huge_pte_write(entry)) {
-			ret = hugetlb_cow(mm, vma, address, ptep,
-					  pagecache_page, ptl);
-			goto out_put_page;
+			if (!(vma->vm_flags & VM_SHARED)) {
+				ret = hugetlb_cow(mm, vma, address, ptep,
+						pagecache_page, ptl);
+				goto out_put_page;
+			}
+
+			/* write protected for soft dirty processing */
+			if ((vma->vm_flags & VM_WRITE) &&
+					(vma->vm_flags & VM_SHARED))
+				entry = huge_pte_mkwrite(entry);
+
+			entry = huge_pte_mkdirty(entry);
 		}
 		entry = huge_pte_mkdirty(entry);
 	}
-- 
2.29.2

Re: [RFC PATCH 2/5] hugetlb: enhance hugetlb fault processing to support soft dirty

[Peter Xu]

On Wed, Feb 10, 2021 at 04:03:19PM -0800, Mike Kravetz wrote:
> hugetlb fault processing code would COW all write faults where the
> pte was not writable.  Soft dirty will write protect ptes as part
> of it's tracking mechanism.  The existing hugetlb_cow  code will do
> the right thing for PRIVATE mappings as it checks map_count.  However,
> for SHARED mappings it would actually allocate and install a COW page.
> Modify the code to not call hugetlb_cow for SHARED mappings and just
> update the pte.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  mm/hugetlb.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 47f3123afd1a..b561b6867ec1 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4584,8 +4584,10 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  	 * spinlock. For private mappings, we also lookup the pagecache
>  	 * page now as it is used to determine if a reservation has been
>  	 * consumed.
> +	 * Only non-shared mappings are sent to hugetlb_cow.
>  	 */
> -	if ((flags & FAULT_FLAG_WRITE) && !huge_pte_write(entry)) {
> +	if ((flags & FAULT_FLAG_WRITE) && !huge_pte_write(entry) &&
> +					!(vma->vm_flags & VM_SHARED)) {
>  		if (vma_needs_reservation(h, vma, haddr) < 0) {
>  			ret = VM_FAULT_OOM;
>  			goto out_mutex;
> @@ -4593,9 +4595,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  		/* Just decrements count, does not deallocate */
>  		vma_end_reservation(h, vma, haddr);
>  
> -		if (!(vma->vm_flags & VM_MAYSHARE))
> -			pagecache_page = hugetlbfs_pagecache_page(h,
> -								vma, haddr);
> +		pagecache_page = hugetlbfs_pagecache_page(h, vma, haddr);

Pure question: I see that the check actually changed from VM_MAYSHARE into
VM_SHARE, then I noticed I'm actually unclear on the difference..  Say, when
VM_MAYSHARE is set, could VM_SHARED be cleared in any case?  Or say, is this
change intended?

I see that vma_set_page_prot() tried to remove VM_SHARED if soft dirty enabled
(which should cause vma_wants_writenotify() to return true, iiuc), however
that's temporary just to calculate vm_page_prot, and it's not applied to the
vma->vm_flags.  I failed to find a place where VM_SHARED of the vma is cleared
while VM_MAYSHARE is set..

>  	}
>  
>  	ptl = huge_pte_lock(h, mm, ptep);
> @@ -4620,9 +4620,18 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  
>  	if (flags & FAULT_FLAG_WRITE) {
>  		if (!huge_pte_write(entry)) {
> -			ret = hugetlb_cow(mm, vma, address, ptep,
> -					  pagecache_page, ptl);
> -			goto out_put_page;
> +			if (!(vma->vm_flags & VM_SHARED)) {
> +				ret = hugetlb_cow(mm, vma, address, ptep,
> +						pagecache_page, ptl);
> +				goto out_put_page;
> +			}
> +
> +			/* write protected for soft dirty processing */
> +			if ((vma->vm_flags & VM_WRITE) &&

This VM_WRITE check seems to be redundant.  As example, do_user_addr_fault() of
x86 code will check this right after vma lookup by access_error().  So when
reach here if "flags & FAULT_FLAG_WRITE", then VM_WRITE must be set, imho.

> +					(vma->vm_flags & VM_SHARED))
> +				entry = huge_pte_mkwrite(entry);

Same question to VM_SHARED, since "(vma->vm_flags & VM_SHARED)" is just checked
above and we'll go hugetlb_cow() otherwise.

> +
> +			entry = huge_pte_mkdirty(entry);

There's another huge_pte_mkdirty() right below; likely we could merge them somehow?

Thanks,

>  		}
>  		entry = huge_pte_mkdirty(entry);
>  	}
> -- 
> 2.29.2
> 

-- 
Peter Xu

Re: [RFC PATCH 2/5] hugetlb: enhance hugetlb fault processing to support soft dirty

[Mike Kravetz]

On 2/17/21 11:32 AM, Peter Xu wrote:
> On Wed, Feb 10, 2021 at 04:03:19PM -0800, Mike Kravetz wrote:
>> hugetlb fault processing code would COW all write faults where the
>> pte was not writable.  Soft dirty will write protect ptes as part
>> of it's tracking mechanism.  The existing hugetlb_cow  code will do
>> the right thing for PRIVATE mappings as it checks map_count.  However,
>> for SHARED mappings it would actually allocate and install a COW page.
>> Modify the code to not call hugetlb_cow for SHARED mappings and just
>> update the pte.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  mm/hugetlb.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 47f3123afd1a..b561b6867ec1 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -4584,8 +4584,10 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>>  	 * spinlock. For private mappings, we also lookup the pagecache
>>  	 * page now as it is used to determine if a reservation has been
>>  	 * consumed.
>> +	 * Only non-shared mappings are sent to hugetlb_cow.
>>  	 */
>> -	if ((flags & FAULT_FLAG_WRITE) && !huge_pte_write(entry)) {
>> +	if ((flags & FAULT_FLAG_WRITE) && !huge_pte_write(entry) &&
>> +					!(vma->vm_flags & VM_SHARED)) {
>>  		if (vma_needs_reservation(h, vma, haddr) < 0) {
>>  			ret = VM_FAULT_OOM;
>>  			goto out_mutex;
>> @@ -4593,9 +4595,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>>  		/* Just decrements count, does not deallocate */
>>  		vma_end_reservation(h, vma, haddr);
>>  
>> -		if (!(vma->vm_flags & VM_MAYSHARE))
>> -			pagecache_page = hugetlbfs_pagecache_page(h,
>> -								vma, haddr);
>> +		pagecache_page = hugetlbfs_pagecache_page(h, vma, haddr);
> 
> Pure question: I see that the check actually changed from VM_MAYSHARE into
> VM_SHARE, then I noticed I'm actually unclear on the difference..  Say, when
> VM_MAYSHARE is set, could VM_SHARED be cleared in any case?  Or say, is this
> change intended?

The change was not intended.  I will use VM_MAYSHARE.

> 
> I see that vma_set_page_prot() tried to remove VM_SHARED if soft dirty enabled
> (which should cause vma_wants_writenotify() to return true, iiuc), however
> that's temporary just to calculate vm_page_prot, and it's not applied to the
> vma->vm_flags.  I failed to find a place where VM_SHARED of the vma is cleared
> while VM_MAYSHARE is set..

I am not 100% sure about differences.  Here is a snippet from do_mmap() where
you can have VM_MAYSHARE and not VM_SHARED

                        vm_flags |= VM_SHARED | VM_MAYSHARE;
                        if (!(file->f_mode & FMODE_WRITE))
                                vm_flags &= ~(VM_MAYWRITE | VM_SHARED);
                        fallthrough;

> 
>>  	}
>>  
>>  	ptl = huge_pte_lock(h, mm, ptep);
>> @@ -4620,9 +4620,18 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>>  
>>  	if (flags & FAULT_FLAG_WRITE) {
>>  		if (!huge_pte_write(entry)) {
>> -			ret = hugetlb_cow(mm, vma, address, ptep,
>> -					  pagecache_page, ptl);
>> -			goto out_put_page;
>> +			if (!(vma->vm_flags & VM_SHARED)) {
>> +				ret = hugetlb_cow(mm, vma, address, ptep,
>> +						pagecache_page, ptl);
>> +				goto out_put_page;
>> +			}
>> +
>> +			/* write protected for soft dirty processing */
>> +			if ((vma->vm_flags & VM_WRITE) &&
> 
> This VM_WRITE check seems to be redundant.  As example, do_user_addr_fault() of
> x86 code will check this right after vma lookup by access_error().  So when
> reach here if "flags & FAULT_FLAG_WRITE", then VM_WRITE must be set, imho.

Thanks, that sounds reasonable.  I will check to make sure and drop the
redundant check.

> 
>> +					(vma->vm_flags & VM_SHARED))
>> +				entry = huge_pte_mkwrite(entry);
> 
> Same question to VM_SHARED, since "(vma->vm_flags & VM_SHARED)" is just checked
> above and we'll go hugetlb_cow() otherwise.

Yes, certainly redundant here.

> 
>> +
>> +			entry = huge_pte_mkdirty(entry);
> 
> There's another huge_pte_mkdirty() right below; likely we could merge them somehow?
> 

Yes,

Thanks for taking a look!

-- 
Mike Kravetz

> Thanks,
> 
>>  		}
>>  		entry = huge_pte_mkdirty(entry);
>>  	}
>> -- 
>> 2.29.2
>>
> 

[RFC PATCH 3/5] mm proc/task_mmu.c: add soft dirty pte checks for hugetlb

[Mike Kravetz]

Pagemap was only using the vma flag PM_SOFT_DIRTY for hugetlb vmas.
This is insufficient.  Check the individual pte entries.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/proc/task_mmu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 602e3a52884d..829b35016aaa 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1507,6 +1507,10 @@ static int pagemap_hugetlb_range(pte_t *ptep, unsigned long hmask,
 		flags |= PM_SOFT_DIRTY;
 
 	pte = huge_ptep_get(ptep);
+
+	if (huge_pte_soft_dirty(pte))
+		flags |= PM_SOFT_DIRTY;
+
 	if (pte_present(pte)) {
 		struct page *page = pte_page(pte);
 
-- 
2.29.2

Re: [RFC PATCH 3/5] mm proc/task_mmu.c: add soft dirty pte checks for hugetlb

[Peter Xu]

On Wed, Feb 10, 2021 at 04:03:20PM -0800, Mike Kravetz wrote:
> Pagemap was only using the vma flag PM_SOFT_DIRTY for hugetlb vmas.
> This is insufficient.  Check the individual pte entries.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  fs/proc/task_mmu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 602e3a52884d..829b35016aaa 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1507,6 +1507,10 @@ static int pagemap_hugetlb_range(pte_t *ptep, unsigned long hmask,
>  		flags |= PM_SOFT_DIRTY;
>  
>  	pte = huge_ptep_get(ptep);
> +
> +	if (huge_pte_soft_dirty(pte))
> +		flags |= PM_SOFT_DIRTY;

Should this be put into pte_present() chunk below?  Since I feel like we'd need
huge_pte_swp_soft_dirty() for !pte_present().  Say, _PAGE_SOFT_DIRTY and
_PAGE_SWP_SOFT_DIRTY can be different.

> +
>  	if (pte_present(pte)) {
>  		struct page *page = pte_page(pte);
>  
> -- 
> 2.29.2
> 

-- 
Peter Xu

Re: [RFC PATCH 3/5] mm proc/task_mmu.c: add soft dirty pte checks for hugetlb

[Mike Kravetz]

On 2/17/21 11:35 AM, Peter Xu wrote:
> On Wed, Feb 10, 2021 at 04:03:20PM -0800, Mike Kravetz wrote:
>> Pagemap was only using the vma flag PM_SOFT_DIRTY for hugetlb vmas.
>> This is insufficient.  Check the individual pte entries.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  fs/proc/task_mmu.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 602e3a52884d..829b35016aaa 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -1507,6 +1507,10 @@ static int pagemap_hugetlb_range(pte_t *ptep, unsigned long hmask,
>>  		flags |= PM_SOFT_DIRTY;
>>  
>>  	pte = huge_ptep_get(ptep);
>> +
>> +	if (huge_pte_soft_dirty(pte))
>> +		flags |= PM_SOFT_DIRTY;
> 
> Should this be put into pte_present() chunk below?  Since I feel like we'd need
> huge_pte_swp_soft_dirty() for !pte_present().  Say, _PAGE_SOFT_DIRTY and
> _PAGE_SWP_SOFT_DIRTY can be different.
> 

Yes this should be moved below, and it should check for both.

Thanks,
-- 
Mike Kravetz

>> +
>>  	if (pte_present(pte)) {
>>  		struct page *page = pte_page(pte);
>>  
>> -- 
>> 2.29.2
>>
> 

[RFC PATCH 4/5] hugetlb: don't permit pmd sharing if soft dirty in use

[Mike Kravetz]

If page modifications are being monitoried with the soft dirty mechanism,
then this information is contained in page table entries.  Tracking is
enabled on a 'per-process' basis.  hugetlb pmd sharing allows processes
to share page table entries.  This makes pmd sharing incompatible with
soft dirty monitoring.  So, pmd sharing must be disabled if soft dirty
tracking is in use.

The VM_SOFTDIRTY flag is set in all vma's by default.  When soft dirty
monitoring is started, the flag is cleared.  We can therefore allow pmd
sharing in vmas with the VM_SOFTDIRTY flag set.  Check this when deciding
to perform pmd sharing.

A subsequent patch will add code to allow soft dirty monitoring for hugetlb
vmas.  Any existing pmd sharing will be undone at that time.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/hugetlb.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index b561b6867ec1..cf4aa63be9b1 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5285,6 +5285,13 @@ static bool vma_shareable(struct vm_area_struct *vma, unsigned long addr)
 	unsigned long base = addr & PUD_MASK;
 	unsigned long end = base + PUD_SIZE;
 
+	/*
+	 * Do not allow pmd sharing if soft dirty tracking is in use.
+	 */
+	if (IS_ENABLED(CONFIG_MEM_SOFT_DIRTY) &&
+				!(vma->vm_flags & VM_SOFTDIRTY))
+		return false;
+
 	/*
 	 * check on proper vm_flags and page table alignment
 	 */
-- 
2.29.2

Re: [RFC PATCH 4/5] hugetlb: don't permit pmd sharing if soft dirty in use

[Peter Xu]

On Wed, Feb 10, 2021 at 04:03:21PM -0800, Mike Kravetz wrote:
> If page modifications are being monitoried with the soft dirty mechanism,
> then this information is contained in page table entries.  Tracking is
> enabled on a 'per-process' basis.  hugetlb pmd sharing allows processes
> to share page table entries.  This makes pmd sharing incompatible with
> soft dirty monitoring.  So, pmd sharing must be disabled if soft dirty
> tracking is in use.
> 
> The VM_SOFTDIRTY flag is set in all vma's by default.  When soft dirty
> monitoring is started, the flag is cleared.  We can therefore allow pmd
> sharing in vmas with the VM_SOFTDIRTY flag set.  Check this when deciding
> to perform pmd sharing.
> 
> A subsequent patch will add code to allow soft dirty monitoring for hugetlb
> vmas.  Any existing pmd sharing will be undone at that time.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  mm/hugetlb.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index b561b6867ec1..cf4aa63be9b1 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5285,6 +5285,13 @@ static bool vma_shareable(struct vm_area_struct *vma, unsigned long addr)
>  	unsigned long base = addr & PUD_MASK;
>  	unsigned long end = base + PUD_SIZE;
>  
> +	/*
> +	 * Do not allow pmd sharing if soft dirty tracking is in use.
> +	 */
> +	if (IS_ENABLED(CONFIG_MEM_SOFT_DIRTY) &&
> +				!(vma->vm_flags & VM_SOFTDIRTY))
> +		return false;
> +

Reviewed-by: Peter Xu <peterx@redhat.com>

The indent of using three tabs is a bit weird to me, though.. I do see this
happen somewhere else in the repo too, so maybe it's a way to indent that I
don't know.

Thanks,

-- 
Peter Xu

[RFC PATCH 5/5] mm proc/task_mmu.c: add hugetlb specific routine for clear_refs

[Mike Kravetz]

There was is no hugetlb specific routine for clearing soft dirty and
other referrences.  The 'default' routines would only clear the
VM_SOFTDIRTY flag in the vma.

Add new routine specifically for hugetlb vmas.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/proc/task_mmu.c | 110 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 829b35016aaa..f06cf9b131a8 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1116,6 +1116,115 @@ static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
 }
 #endif
 
+#ifdef CONFIG_HUGETLB_PAGE
+static inline bool huge_pte_is_pinned(struct vm_area_struct *vma,
+					unsigned long addr, pte_t pte)
+{
+	struct page *page;
+
+	if (likely(!atomic_read(&vma->vm_mm->has_pinned)))
+		return false;
+	page = pte_page(pte);
+	if (!page)
+		return false;
+	return page_maybe_dma_pinned(page);
+}
+
+static int clear_refs_hugetlb_range(pte_t *ptep, unsigned long hmask,
+				unsigned long addr, unsigned long end,
+				struct mm_walk *walk)
+{
+	struct clear_refs_private *cp = walk->private;
+	struct vm_area_struct *vma = walk->vma;
+	struct hstate *h = hstate_vma(walk->vma);
+	unsigned long adj_start = addr, adj_end = end;
+	spinlock_t *ptl;
+	pte_t old_pte, pte;
+
+	/*
+	 * clear_refs should only operate on complete vmas.  Therefore,
+	 * values passed here should be huge page aligned and huge page
+	 * size in length.  Quick validation before taking any action in
+	 * case upstream code is changed.
+	 */
+	if ((addr & hmask) != addr || end - addr != huge_page_size(h)) {
+		WARN_ONCE(1, "%s passed unaligned address\n", __func__);
+		return 1;
+	}
+
+	ptl = huge_pte_lock(hstate_vma(vma), walk->mm, ptep);
+
+	/* Soft dirty and pmd sharing do not mix */
+
+	pte = huge_ptep_get(ptep);
+	if (!pte_present(pte))
+		goto out;
+	if (unlikely(is_hugetlb_entry_hwpoisoned(pte)))
+		goto out;
+
+	if (cp->type == CLEAR_REFS_SOFT_DIRTY) {
+		if (huge_pte_is_pinned(vma, addr, pte))
+			goto out;
+
+		/*
+		 * soft dirty and pmd sharing do not work together as
+		 * per-process is tracked in ptes, and pmd sharing allows
+		 * processed to share ptes.  We unshare any pmds here.
+		 */
+		adjust_range_if_pmd_sharing_possible(vma, &adj_start, &adj_end);
+		flush_cache_range(vma, adj_start, adj_end);
+		/*
+		 * Only atttempt unshare if sharing possible.  If we unshare,
+		 * then pte's for a PUD sized area are effectively removed for
+		 * this process.  That clears soft dirty.
+		 */
+		if (adj_start != addr || adj_end != end) {
+			struct mmu_notifier_range range;
+			int unshared;
+
+			mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR,
+					0, vma, vma->vm_mm, adj_start, adj_end);
+			mmu_notifier_invalidate_range_start(&range);
+			i_mmap_lock_write(vma->vm_file->f_mapping);
+			unshared = huge_pmd_unshare(vma->vm_mm, vma,
+								&addr, ptep);
+			i_mmap_unlock_write(vma->vm_file->f_mapping);
+			mmu_notifier_invalidate_range_end(&range);
+			if (unshared)
+				goto unshare_done;
+		}
+
+		if (is_hugetlb_entry_migration(pte)) {
+			pte = huge_pte_swp_clear_soft_dirty(pte);
+			set_huge_pte_at(walk->mm, addr, ptep, pte);
+		} else {
+			old_pte = huge_ptep_modify_prot_start(vma, addr, ptep);
+			pte = pte_mkhuge(huge_pte_wrprotect(pte));
+			pte = arch_make_huge_pte(pte, vma, NULL, 0);
+			pte = huge_pte_clear_soft_dirty(pte);
+			pte = huge_pte_mkyoung(pte);
+			huge_ptep_modify_prot_commit(vma, addr, ptep,
+							old_pte, pte);
+		}
+
+unshare_done:
+		flush_hugetlb_tlb_range(vma, addr, end);
+	}
+
+	/* reference bits in hugetlb pages are not reset/used */
+out:
+	spin_unlock(ptl);
+	return 0;
+}
+#else /* CONFIG_HUGETLB_PAGE */
+static int clear_refs_hugetlb_range(pte_t *ptep, unsigned long hmask,
+				unsigned long addr, unsigned long end,
+				struct mm_walk *walk)
+{
+	return 1;
+}
+#endif /* CONFIG_HUGETLB_PAGE */
+
 static int clear_refs_pte_range(pmd_t *pmd, unsigned long addr,
 				unsigned long end, struct mm_walk *walk)
 {
@@ -1198,6 +1307,7 @@ static int clear_refs_test_walk(unsigned long start, unsigned long end,
 }
 
 static const struct mm_walk_ops clear_refs_walk_ops = {
+	.hugetlb_entry		= clear_refs_hugetlb_range,
 	.pmd_entry		= clear_refs_pte_range,
 	.test_walk		= clear_refs_test_walk,
 };
-- 
2.29.2

Re: [RFC PATCH 5/5] mm proc/task_mmu.c: add hugetlb specific routine for clear_refs

[Peter Xu]

On Wed, Feb 10, 2021 at 04:03:22PM -0800, Mike Kravetz wrote:
> There was is no hugetlb specific routine for clearing soft dirty and
> other referrences.  The 'default' routines would only clear the
> VM_SOFTDIRTY flag in the vma.
> 
> Add new routine specifically for hugetlb vmas.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  fs/proc/task_mmu.c | 110 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 829b35016aaa..f06cf9b131a8 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1116,6 +1116,115 @@ static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
>  }
>  #endif
>  
> +#ifdef CONFIG_HUGETLB_PAGE
> +static inline bool huge_pte_is_pinned(struct vm_area_struct *vma,
> +					unsigned long addr, pte_t pte)
> +{
> +	struct page *page;
> +
> +	if (likely(!atomic_read(&vma->vm_mm->has_pinned)))
> +		return false;
> +	page = pte_page(pte);
> +	if (!page)
> +		return false;
> +	return page_maybe_dma_pinned(page);
> +}
> +
> +static int clear_refs_hugetlb_range(pte_t *ptep, unsigned long hmask,
> +				unsigned long addr, unsigned long end,
> +				struct mm_walk *walk)
> +{
> +	struct clear_refs_private *cp = walk->private;
> +	struct vm_area_struct *vma = walk->vma;
> +	struct hstate *h = hstate_vma(walk->vma);
> +	unsigned long adj_start = addr, adj_end = end;
> +	spinlock_t *ptl;
> +	pte_t old_pte, pte;
> +
> +	/*
> +	 * clear_refs should only operate on complete vmas.  Therefore,
> +	 * values passed here should be huge page aligned and huge page
> +	 * size in length.  Quick validation before taking any action in
> +	 * case upstream code is changed.
> +	 */
> +	if ((addr & hmask) != addr || end - addr != huge_page_size(h)) {
> +		WARN_ONCE(1, "%s passed unaligned address\n", __func__);
> +		return 1;
> +	}

I wouldn't worry too much on the interface change - The one who will change the
interface should guarantee all existing hooks will still work, isn't it? :)

It's slightly confusing to me on why "clear_refs should only operate on
complete vmas" is related to the check, though.

> +
> +	ptl = huge_pte_lock(hstate_vma(vma), walk->mm, ptep);
> +
> +	/* Soft dirty and pmd sharing do not mix */

Right, this seems to be a placeholder for unsharing code.

Though maybe we can do that earlier in pre_vma() hook?  That should be per-vma
rather than handling one specific huge page here, hence more efficient imho.

this reminded me that I should also better move hugetlb_unshare_all_pmds() of
my other patch into hugetlb.c, so that this code can call it.  Currently it's a
static function in userfaultfd.c.

> +
> +	pte = huge_ptep_get(ptep);
> +	if (!pte_present(pte))
> +		goto out;
> +	if (unlikely(is_hugetlb_entry_hwpoisoned(pte)))
> +		goto out;
> +
> +	if (cp->type == CLEAR_REFS_SOFT_DIRTY) {

Maybe move this check into clear_refs_test_walk()?  We can bail out earlier if:

      (is_vm_hugetlb_page(vma) && (type != CLEAR_REFS_SOFT_DIRTY))

> +		if (huge_pte_is_pinned(vma, addr, pte))
> +			goto out;

Out of topic of this patchset, but it's definitely a pity that we can't track
soft dirty for pinned pages.  Currently the assumption of the pte code path is:
"if this page can be DMA written then we won't know whether data changed after
all, then tracking dirty is meaningless", however that's prone to change when
new hardwares coming, say, IOMMU could start to trap DMA writes already.

But again that's another story.. and we should just follow what we do with
non-hugetlbfs for sure here, until some day if we'd like to revive soft dirty
tracking with pinned pages.

> +
> +		/*
> +		 * soft dirty and pmd sharing do not work together as
> +		 * per-process is tracked in ptes, and pmd sharing allows
> +		 * processed to share ptes.  We unshare any pmds here.
> +		 */
> +		adjust_range_if_pmd_sharing_possible(vma, &adj_start, &adj_end);

Ideally when reach here, huge pmd sharing won't ever exist, right?  Then do we
still need to adjust the range at all?

Thanks,

-- 
Peter Xu

Re: [RFC PATCH 5/5] mm proc/task_mmu.c: add hugetlb specific routine for clear_refs

[Mike Kravetz]

On 2/17/21 12:25 PM, Peter Xu wrote:
> On Wed, Feb 10, 2021 at 04:03:22PM -0800, Mike Kravetz wrote:
>> There was is no hugetlb specific routine for clearing soft dirty and
>> other referrences.  The 'default' routines would only clear the
>> VM_SOFTDIRTY flag in the vma.
>>
>> Add new routine specifically for hugetlb vmas.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  fs/proc/task_mmu.c | 110 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 110 insertions(+)
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 829b35016aaa..f06cf9b131a8 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -1116,6 +1116,115 @@ static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
>>  }
>>  #endif
>>  
>> +#ifdef CONFIG_HUGETLB_PAGE
>> +static inline bool huge_pte_is_pinned(struct vm_area_struct *vma,
>> +					unsigned long addr, pte_t pte)
>> +{
>> +	struct page *page;
>> +
>> +	if (likely(!atomic_read(&vma->vm_mm->has_pinned)))
>> +		return false;
>> +	page = pte_page(pte);
>> +	if (!page)
>> +		return false;
>> +	return page_maybe_dma_pinned(page);
>> +}
>> +
>> +static int clear_refs_hugetlb_range(pte_t *ptep, unsigned long hmask,
>> +				unsigned long addr, unsigned long end,
>> +				struct mm_walk *walk)
>> +{
>> +	struct clear_refs_private *cp = walk->private;
>> +	struct vm_area_struct *vma = walk->vma;
>> +	struct hstate *h = hstate_vma(walk->vma);
>> +	unsigned long adj_start = addr, adj_end = end;
>> +	spinlock_t *ptl;
>> +	pte_t old_pte, pte;
>> +
>> +	/*
>> +	 * clear_refs should only operate on complete vmas.  Therefore,
>> +	 * values passed here should be huge page aligned and huge page
>> +	 * size in length.  Quick validation before taking any action in
>> +	 * case upstream code is changed.
>> +	 */
>> +	if ((addr & hmask) != addr || end - addr != huge_page_size(h)) {
>> +		WARN_ONCE(1, "%s passed unaligned address\n", __func__);
>> +		return 1;
>> +	}
> 
> I wouldn't worry too much on the interface change - The one who will change the
> interface should guarantee all existing hooks will still work, isn't it? :)

Yeah, I can drop this.

> It's slightly confusing to me on why "clear_refs should only operate on
> complete vmas" is related to the check, though.

Mostly me thinking that since it is operating on complete (hugetlb) vma,
then we know vms is huge page aligned and a multiple of huge page in size.
So, all passed addressed should be huge page aligned as well.

> 
>> +
>> +	ptl = huge_pte_lock(hstate_vma(vma), walk->mm, ptep);
>> +
>> +	/* Soft dirty and pmd sharing do not mix */
> 
> Right, this seems to be a placeholder for unsharing code.

Sorry, comment was left over from earlier code.  Unsharing is actually
done below, I forgot to remove comment.

> Though maybe we can do that earlier in pre_vma() hook?  That should be per-vma
> rather than handling one specific huge page here, hence more efficient imho.

Yes, let me look into that.  The code below is certianly not the most
efficient.

> this reminded me that I should also better move hugetlb_unshare_all_pmds() of
> my other patch into hugetlb.c, so that this code can call it.  Currently it's a
> static function in userfaultfd.c.
> 
>> +
>> +	pte = huge_ptep_get(ptep);
>> +	if (!pte_present(pte))
>> +		goto out;
>> +	if (unlikely(is_hugetlb_entry_hwpoisoned(pte)))
>> +		goto out;
>> +
>> +	if (cp->type == CLEAR_REFS_SOFT_DIRTY) {
> 
> Maybe move this check into clear_refs_test_walk()?  We can bail out earlier if:
> 
>       (is_vm_hugetlb_page(vma) && (type != CLEAR_REFS_SOFT_DIRTY))
> 

Yes, we can do that.  I was patterning this after the other 'clear_refs'
routines.  But, they can clear things besides soft dirty.  Since soft
dirty is the only thing handled for hugetlb, we can bail earlier.

>> +		if (huge_pte_is_pinned(vma, addr, pte))
>> +			goto out;
> 
> Out of topic of this patchset, but it's definitely a pity that we can't track
> soft dirty for pinned pages.  Currently the assumption of the pte code path is:
> "if this page can be DMA written then we won't know whether data changed after
> all, then tracking dirty is meaningless", however that's prone to change when
> new hardwares coming, say, IOMMU could start to trap DMA writes already.
> 
> But again that's another story.. and we should just follow what we do with
> non-hugetlbfs for sure here, until some day if we'd like to revive soft dirty
> tracking with pinned pages.
> 
>> +
>> +		/*
>> +		 * soft dirty and pmd sharing do not work together as
>> +		 * per-process is tracked in ptes, and pmd sharing allows
>> +		 * processed to share ptes.  We unshare any pmds here.
>> +		 */
>> +		adjust_range_if_pmd_sharing_possible(vma, &adj_start, &adj_end);
> 
> Ideally when reach here, huge pmd sharing won't ever exist, right?  Then do we
> still need to adjust the range at all?

Right, we should be able to do it earlier.

Thanks again for taking a look at this.
-- 
Mike Kravetz

> 
> Thanks,
>