[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8f08ae90-8062-433d-9798-19a23de35167@lucifer.local>
Date: Fri, 27 Jun 2025 19:39:21 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
Andrew Morton <akpm@...ux-foundation.org>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>,
Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
Mike Rapoport <rppt@...nel.org>,
Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
Zi Yan <ziy@...dia.com>, Matthew Brost <matthew.brost@...el.com>,
Joshua Hahn <joshua.hahnjy@...il.com>, Rakie Kim <rakie.kim@...com>,
Byungchul Park <byungchul@...com>, Gregory Price <gourry@...rry.net>,
Ying Huang <ying.huang@...ux.alibaba.com>,
Alistair Popple <apopple@...dia.com>, Pedro Falcato <pfalcato@...e.de>,
Rik van Riel <riel@...riel.com>, Harry Yoo <harry.yoo@...cle.com>
Subject: Re: [PATCH v1 2/4] mm: smaller folio_pte_batch() improvements
On Fri, Jun 27, 2025 at 07:02:16PM +0200, David Hildenbrand wrote:
> On 27.06.25 18:51, Lorenzo Stoakes wrote:
> > On Fri, Jun 27, 2025 at 01:55:08PM +0200, David Hildenbrand wrote:
> > > Let's clean up a bit:
> > >
> > > (1) No need for start_ptep vs. ptep anymore, we can simply use ptep
> > >
> > > (2) Let's switch to "unsigned int" for everything
> > >
> > > (3) We can simplify the code by leaving the pte unchanged after the
> > > pte_same() check.
> > >
> > > (4) Clarify that we should never exceed a single VMA; it indicates a
> > > problem in the caller.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: David Hildenbrand <david@...hat.com>
Given below, LGTM:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > > -static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
> > > - pte_t *start_ptep, pte_t pte, int max_nr, fpb_t flags,
> > > +static inline unsigned int folio_pte_batch(struct folio *folio, unsigned long addr,
> >
> > Do we need to worry about propagating this type change?
> >
> > mremap_folio_pte_batch() and zap_present_ptes() return the value as an int for example.
> >
> > I mean I doubt we're going to be seeing an overflow here :) but maybe worth
> > propagating this everywhere.
>
> Yeah, I'm planning on cleaning some of that stuff separately. As you say,
> this shouldn't cause harm.
>
> Really, it could only cause harm if someone would be doing
>
> "- folio_pte_batch()" and then assigning it to a "long".
Right yeah.
>
> >
> > > + pte_t *ptep, pte_t pte, unsigned int max_nr, fpb_t flags,
> > > bool *any_writable, bool *any_young, bool *any_dirty)
> > > {
> > > - pte_t expected_pte, *ptep;
> > > - bool writable, young, dirty;
> > > - int nr, cur_nr;
> > > + unsigned int nr, cur_nr;
> > > + pte_t expected_pte;
> > >
> > > if (any_writable)
> > > *any_writable = false;
> > > @@ -267,29 +267,22 @@ static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
> > > max_nr = min_t(unsigned long, max_nr,
> > > folio_pfn(folio) + folio_nr_pages(folio) - pte_pfn(pte));
> > >
> > > - nr = pte_batch_hint(start_ptep, pte);
> > > + nr = pte_batch_hint(ptep, pte);
> > > expected_pte = __pte_batch_clear_ignored(pte_advance_pfn(pte, nr), flags);
> > > - ptep = start_ptep + nr;
> > > + ptep = ptep + nr;
> > >
> > > while (nr < max_nr) {
> > > pte = ptep_get(ptep);
> > > - if (any_writable)
> > > - writable = !!pte_write(pte);
> > > - if (any_young)
> > > - young = !!pte_young(pte);
> > > - if (any_dirty)
> > > - dirty = !!pte_dirty(pte);
> > > - pte = __pte_batch_clear_ignored(pte, flags);
> > >
> > > - if (!pte_same(pte, expected_pte))
> > > + if (!pte_same(__pte_batch_clear_ignored(pte, flags), expected_pte))
> >
> > Doing this here will change the output of any_writable, any_young:
> >
> > static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
> > {
> > ...
> > return pte_wrprotect(pte_mkold(pte));
> > }
> >
> > So we probably need to get these values earlier?
>
>
> Note that __pte_batch_clear_ignored() leaves the pte unchanged, and only
> returns the modified pte. (like pte_mkold() and friends).
>
> So what we read through ptep_get() is still what we have after the
> pte_same() check.
Yeah you're right, sorry, these are just chaging the value that is returned for
comparison against expected_pte.
Then LGTM!
Powered by blists - more mailing lists