Received-SPF: pass (google.com: domain of linux-kernel+bounces-171996-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240506211333.346605-1-yosryahmed@google.com>
 <1a408ed1-7e81-457e-a205-db274b4d6b78@redhat.com> <CAJD7tkam8BvLvOrw_FfgZ8XOsfdu-Hv2oCv_s+LAMcGvd44hTA@mail.gmail.com>
 <42a18f08-99a5-4bb0-8a1d-771549cac046@redhat.com>
In-Reply-To: <42a18f08-99a5-4bb0-8a1d-771549cac046@redhat.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Tue, 7 May 2024 11:42:07 -0700
Message-ID: <CAJD7tkYkhw1p-8qtJt7ux5qq7T-NA4NsUJBaQPZqX-01OP31mg@mail.gmail.com>
Subject: Re: [PATCH mm-unstable] mm: rmap: abstract updating per-node and
 per-memcg stats
To: David Hildenbrand <david@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 7, 2024 at 11:38=E2=80=AFAM David Hildenbrand <david@redhat.com=
> wrote:
>
> On 07.05.24 17:54, Yosry Ahmed wrote:
> > On Tue, May 7, 2024 at 1:52=E2=80=AFAM David Hildenbrand <david@redhat.=
com> wrote:
> >>
> >> On 06.05.24 23:13, Yosry Ahmed wrote:
> >>> A lot of intricacies go into updating the stats when adding or removi=
ng
> >>> mappings: which stat index to use and which function. Abstract this a=
way
> >>> into a new static helper in rmap.c, __folio_mod_stat().
> >>>
> >>> This adds an unnecessary call to folio_test_anon() in
> >>> __folio_add_anon_rmap() and __folio_add_file_rmap(). However, the fol=
io
> >>> struct should already be in the cache at this point, so it shouldn't
> >>> cause any noticeable overhead.
> >>
> >> Depending on the inlining, we might have more branches that could be a=
voided
> >> (especially in folio_add_new_anon_rmap()).
> >>
> >> [the rmap code is more performance-sensitive and relevant than you mig=
ht think]
> >
> > I thought about making the helper __always_inline. Would that be better=
?
>
> Let's leave it like that. I might do some actual measurements to see if
> it makes a difference at all.

That would be interesting to find out for sure.

[..]
> >>>
> >>> -     if (nr_pmdmapped) {
> >>> -             /* NR_{FILE/SHMEM}_PMDMAPPED are not maintained per-mem=
cg */
> >>> -             if (folio_test_anon(folio))
> >>> -                     __lruvec_stat_mod_folio(folio, NR_ANON_THPS, -n=
r_pmdmapped);
> >>> -             else
> >>> -                     __mod_node_page_state(pgdat,
> >>> -                                     folio_test_swapbacked(folio) ?
> >>> -                                     NR_SHMEM_PMDMAPPED : NR_FILE_PM=
DMAPPED,
> >>> -                                     -nr_pmdmapped);
> >>> -     }
> >>>        if (nr) {
> >>> -             idx =3D folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FI=
LE_MAPPED;
> >>> -             __lruvec_stat_mod_folio(folio, idx, -nr);
> >>> -
> >>
> >>
> >> We can now even do:
> >>
> >> diff --git a/mm/rmap.c b/mm/rmap.c
> >> index 9ed995da4709..7a147195e512 100644
> >> --- a/mm/rmap.c
> >> +++ b/mm/rmap.c
> >> @@ -1555,18 +1555,17 @@ static __always_inline void __folio_remove_rma=
p(struct folio *folio,
> >>                   break;
> >>           }
> >>
> >> -       if (nr) {
> >> -               /*
> >> -                * Queue anon large folio for deferred split if at lea=
st one
> >> -                * page of the folio is unmapped and at least one page
> >> -                * is still mapped.
> >> -                *
> >> -                * Check partially_mapped first to ensure it is a larg=
e folio.
> >> -                */
> >> -               if (folio_test_anon(folio) && partially_mapped &&
> >> -                   list_empty(&folio->_deferred_list))
> >> -                       deferred_split_folio(folio);
> >> -       }
> >> +       /*
> >> +        * Queue anon large folio for deferred split if at least one
> >> +        * page of the folio is unmapped and at least one page
> >> +        * is still mapped.
> >> +        *
> >> +        * Check partially_mapped first to ensure it is a large folio.
> >> +        */
> >> +       if (folio_test_anon(folio) && partially_mapped &&
> >> +           list_empty(&folio->_deferred_list))
> >> +               deferred_split_folio(folio);
> >> +
> >
> > Dumb question: why is it okay to remove the 'if (nr)' condition here?
> > It seems to me by looking at the code in case RMAP_LEVEL_PMD that it
> > is possible for partially_mapped to be true while nr =3D=3D 0.
>
> Not a dumb question at all, and I cannot immediately tell if we might
> have to move the "nr" check to the RMAP_LEVEL_PMD case (I feel like
> we're good, but will have to double check). So let's keep it as is for
> now and I'll perform that change separately.

SGTM, thanks for checking and for the review.

It appears to me that no changes are required here then :)