Re: [PATCH mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats

[Yosry Ahmed]

On Tue, May 7, 2024 at 11:38 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 07.05.24 17:54, Yosry Ahmed wrote:
> > On Tue, May 7, 2024 at 1:52 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
> >>
> >> On 06.05.24 23:13, Yosry Ahmed wrote:
> >>> A lot of intricacies go into updating the stats when adding or removing
> >>> mappings: which stat index to use and which function. Abstract this away
> >>> 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 folio
> >>> 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 avoided
> >> (especially in folio_add_new_anon_rmap()).
> >>
> >> [the rmap code is more performance-sensitive and relevant than you might 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-memcg */
> >>> -             if (folio_test_anon(folio))
> >>> -                     __lruvec_stat_mod_folio(folio, NR_ANON_THPS, -nr_pmdmapped);
> >>> -             else
> >>> -                     __mod_node_page_state(pgdat,
> >>> -                                     folio_test_swapbacked(folio) ?
> >>> -                                     NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED,
> >>> -                                     -nr_pmdmapped);
> >>> -     }
> >>>        if (nr) {
> >>> -             idx = folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE_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_rmap(struct folio *folio,
> >>                   break;
> >>           }
> >>
> >> -       if (nr) {
> >> -               /*
> >> -                * 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);
> >> -       }
> >> +       /*
> >> +        * 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 == 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 :)