Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230720112955.643283-1-ryan.roberts@arm.com> <20230720112955.643283-3-ryan.roberts@arm.com>
 <CAOUHufamqn0b8tN1DppfPi7FRnENzYwcSOVcAKNxkj=CYa+OwQ@mail.gmail.com>
 <ZMFNgmlT1JpI0z5+@casper.infradead.org> <87r0ouw39n.fsf@yhuang6-desk2.ccr.corp.intel.com>
 <ZMHX+O0wmjuPXdTi@casper.infradead.org> <faf8d723-afb8-ac4e-7861-4004bb78c5a9@arm.com>
 <CAOUHufa38H0o0ZoX-KYMSeAiJghMWtjfHM2F4OjUZ-a--_Kt_w@mail.gmail.com> <3c26aa4e-fe11-09d2-c2fb-63546ba80893@arm.com>
In-Reply-To: <3c26aa4e-fe11-09d2-c2fb-63546ba80893@arm.com>
From:   Yu Zhao <yuzhao@google.com>
Date:   Tue, 1 Aug 2023 01:40:09 -0600
Message-ID: <CAOUHufZKVd72hdzUB+++E9+LL6N2WESuHU2AenUDoG=Nv91N5A@mail.gmail.com>
Subject: Re: [PATCH v3 2/3] mm: Implement folio_remove_rmap_range()
To:     Ryan Roberts <ryan.roberts@arm.com>
Cc:     Matthew Wilcox <willy@infradead.org>,
        "Huang, Ying" <ying.huang@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Yin Fengwei <fengwei.yin@intel.com>,
        David Hildenbrand <david@redhat.com>,
        Yang Shi <shy828301@gmail.com>, Zi Yan <ziy@nvidia.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jul 28, 2023 at 3:00=E2=80=AFAM Ryan Roberts <ryan.roberts@arm.com>=
 wrote:
>
> On 27/07/2023 17:38, Yu Zhao wrote:
> > On Thu, Jul 27, 2023 at 1:26=E2=80=AFAM Ryan Roberts <ryan.roberts@arm.=
com> wrote:
> >>
> >> On 27/07/2023 03:35, Matthew Wilcox wrote:
> >>> On Thu, Jul 27, 2023 at 09:29:24AM +0800, Huang, Ying wrote:
> >>>> Matthew Wilcox <willy@infradead.org> writes:
> >>>>> I think that can make sense.  Because we limit to a single page tab=
le,
> >>>>> specifying 'nr =3D 1 << PMD_ORDER' is the same as 'compound =3D tru=
e'.
> >>>>> Just make it folio, page, nr, vma.  I'd actually prefer it as (vma,
> >>>>> folio, page, nr), but that isn't the convention we've had in rmap u=
p
> >>>>> until now.
> >>>>
> >>>> IIUC, even if 'nr =3D 1 << PMD_ORDER', we may remove one PMD 'compou=
nd'
> >>>> mapping, or 'nr' PTE mapping.  So, we will still need 'compound' (or
> >>>> some better name) as parameter.
> >>>
> >>> Oh, this is removing ... so you're concerned with the case where we'v=
e
> >>> split the PMD into PTEs, but all the PTEs are still present in a sing=
le
> >>> page table?  OK, I don't have a good answer to that.  Maybe that torp=
edoes
> >>> the whole idea; I'll think about it.
> >>
> >> This is exactly why I think the approach I've already taken is the cor=
rect one;
> >> a 'range' makes no sense when you are dealing with 'compound' pages be=
cause you
> >> are accounting the entire folio. So surely its better to reflect that =
by only
> >> accounting small pages in the range version of the API.
> >
> > If the argument is the compound case is a separate one, then why not a
> > separate API for it?
> >
> > I don't really care about whether we think 'range' makes sense for
> > 'compound' or not. What I'm saying is:
> > 1. if they are considered one general case, then one API with the
> > compound parameter.
> > 2. if they are considered two specific cases, there should be two APIs.
> > This common design pattern is cleaner IMO.
>
> Option 2 definitely makes sense to me and I agree that it would be cleane=
r to
> have 2 separate APIs, one for small-page accounting (which can accept a r=
ange
> within a folio) and one for large-page accounting (i.e. compound=3Dtrue i=
n today's
> API).
>
> But...
>
> 1) That's not how the rest of the rmap API does it

Yes, but that's how we convert things: one step a time.

> 2) This would be a much bigger change since I'm removing an existing API =
and
> replacing it with a completely new one (there are ~20 call sites to fix u=
p). I
> was trying to keep the change small and manageable by maintaining the cur=
rent
> API but moving all the small-page logic to the new API, so the old API is=
 a
> wrapper in that case.

I don't get how it'd be "much bigger". Isn't it just a straightforward
replacement?

> 3) You would also need an API for the hugetlb case, which page_remove_rma=
p()
> handles today. Perhaps that could also be done by the new API that handle=
s the
> compound case. But then you are mixing and matching your API styles - one=
 caters
> for 1 specific case, and the other caters for 2 cases and figures out whi=
ch one.

You are talking about cases *inside* the APIs, and that's irrelevant
to the number of APIs: we only need two -- one supports a range within
a folio and the other takes a folio as a single unit.

> > Right now we have an overlap (redundancy) -- people would have to do
> > two code searches: one for page_remove_rmap() and the other for
> > folio_remove_rmap_range(nr=3D1), and this IMO is a bad design pattern.
>
> I'm open to doing the work to remove this redundancy, but I'd like to hea=
r
> concensus on this thread that its the right approach first. Although pers=
onally
> I don't see a problem with what I've already done; If you want to operate=
 on a
> page (inc the old concept of a "compound page" and a hugetlb page) call t=
he old
> one. If you want to operate on a range of pages in a folio, call the new =
one.