From: Yosry Ahmed <yosryahmed@google.com>
To: David Hildenbrand <david@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats
Date: Tue, 7 May 2024 11:42:07 -0700 [thread overview]
Message-ID: <CAJD7tkYkhw1p-8qtJt7ux5qq7T-NA4NsUJBaQPZqX-01OP31mg@mail.gmail.com> (raw)
In-Reply-To: <42a18f08-99a5-4bb0-8a1d-771549cac046@redhat.com>
On Tue, May 7, 2024 at 11:38 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 07.05.24 17:54, Yosry Ahmed wrote:
> > On Tue, May 7, 2024 at 1:52 AM 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 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 :)
prev parent reply other threads:[~2024-05-07 18:42 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 21:13 [PATCH mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats Yosry Ahmed
2024-05-07 8:52 ` David Hildenbrand
2024-05-07 15:54 ` Yosry Ahmed
2024-05-07 18:37 ` David Hildenbrand
2024-05-07 18:42 ` Yosry Ahmed [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJD7tkYkhw1p-8qtJt7ux5qq7T-NA4NsUJBaQPZqX-01OP31mg@mail.gmail.com \
--to=yosryahmed@google.com \
--cc=akpm@linux-foundation.org \
--cc=david@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).