Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 24 Feb 2021 17:46:08 +0100
From:   Gerald Schaefer <gerald.schaefer@linux.ibm.com>
To:     Peter Xu <peterx@redhat.com>
Cc:     Mike Kravetz <mike.kravetz@oracle.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org,
        shu wang <malate_wangshu@hotmail.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        Matthew Wilcox <willy@infradead.org>,
        Michel Lespinasse <walken@google.com>,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [RFC PATCH 1/5] hugetlb: add hugetlb helpers for soft dirty
 support
Message-ID: <20210224174608.7c9ca5ed@thinkpad>
In-Reply-To: <20210217162415.GA6519@xz-x1>
References: <20210211000322.159437-1-mike.kravetz@oracle.com>
        <20210211000322.159437-2-mike.kravetz@oracle.com>
        <20210217162415.GA6519@xz-x1>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.