linux-kernel - Re: [PATCH v3 2/3] mm: Implement folio_remove_rmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 1 Aug 2023 01:40:09 -0600
From:   Yu Zhao <yuzhao@...gle.com>
To:     Ryan Roberts <ryan.roberts@....com>
Cc:     Matthew Wilcox <willy@...radead.org>,
        "Huang, Ying" <ying.huang@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Yin Fengwei <fengwei.yin@...el.com>,
        David Hildenbrand <david@...hat.com>,
        Yang Shi <shy828301@...il.com>, Zi Yan <ziy@...dia.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v3 2/3] mm: Implement folio_remove_rmap_range()

On Fri, Jul 28, 2023 at 3:00 AM Ryan Roberts <ryan.roberts@....com> wrote:
>
> On 27/07/2023 17:38, Yu Zhao wrote:
> > On Thu, Jul 27, 2023 at 1:26 AM Ryan Roberts <ryan.roberts@....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@...radead.org> writes:
> >>>>> I think that can make sense.  Because we limit to a single page table,
> >>>>> specifying 'nr = 1 << PMD_ORDER' is the same as 'compound = true'.
> >>>>> 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 up
> >>>>> until now.
> >>>>
> >>>> IIUC, even if 'nr = 1 << PMD_ORDER', we may remove one PMD 'compound'
> >>>> 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've
> >>> split the PMD into PTEs, but all the PTEs are still present in a single
> >>> page table?  OK, I don't have a good answer to that.  Maybe that torpedoes
> >>> the whole idea; I'll think about it.
> >>
> >> This is exactly why I think the approach I've already taken is the correct one;
> >> a 'range' makes no sense when you are dealing with 'compound' pages because 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 cleaner to
> have 2 separate APIs, one for small-page accounting (which can accept a range
> within a folio) and one for large-page accounting (i.e. compound=true in 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 up). I
> was trying to keep the change small and manageable by maintaining the current
> 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_rmap()
> handles today. Perhaps that could also be done by the new API that handles 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 which 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=1), and this IMO is a bad design pattern.
>
> I'm open to doing the work to remove this redundancy, but I'd like to hear
> concensus on this thread that its the right approach first. Although personally
> 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 the old
> one. If you want to operate on a range of pages in a folio, call the new one.