| Message ID | 20240506211333.346605-1-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats | expand |
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] > > No functional change intended. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > > This applies on top of "mm: do not update memcg stats for > NR_{FILE/SHMEM}_PMDMAPPED": > https://lore.kernel.org/lkml/20240506192924.271999-1-yosryahmed@google.com/ > > David, I was on the fence about adding a Suggested-by here. You did > suggest adding a helper, but the one with the extra folio_test_anon() > was my idea and I didn't want to blame it on you. So I'll leave this up > to you :) :) fair enough! It's a clear improvement to readability. [...] > > - 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); + __folio_mod_stat(folio, nr, nr_pmdmapped); /* Which will help some of my upcoming patches. Feel free to include that in a v2, otherwise I'll include it in an upcoming patch series. Reviewed-by: David Hildenbrand <david@redhat.com>
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? > > > > > No functional change intended. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > > > This applies on top of "mm: do not update memcg stats for > > NR_{FILE/SHMEM}_PMDMAPPED": > > https://lore.kernel.org/lkml/20240506192924.271999-1-yosryahmed@google.com/ > > > > David, I was on the fence about adding a Suggested-by here. You did > > suggest adding a helper, but the one with the extra folio_test_anon() > > was my idea and I didn't want to blame it on you. So I'll leave this up > > to you :) > > :) fair enough! It's a clear improvement to readability. > > [...] > > > > - 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. Is this practically impossible for some reason, or is adding the folio to the deferred split queue okay either way? > __folio_mod_stat(folio, nr, nr_pmdmapped); > > /* > > > Which will help some of my upcoming patches. > > Feel free to include that in a v2, otherwise I'll include it in an upcoming > patch series. > > > Reviewed-by: David Hildenbrand <david@redhat.com> Thanks!
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. >>> >>> No functional change intended. >>> >>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com> >>> --- >>> >>> This applies on top of "mm: do not update memcg stats for >>> NR_{FILE/SHMEM}_PMDMAPPED": >>> https://lore.kernel.org/lkml/20240506192924.271999-1-yosryahmed@google.com/ >>> >>> David, I was on the fence about adding a Suggested-by here. You did >>> suggest adding a helper, but the one with the extra folio_test_anon() >>> was my idea and I didn't want to blame it on you. So I'll leave this up >>> to you :) >> >> :) fair enough! It's a clear improvement to readability. >> >> [...] >>> >>> - 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. Thanks!
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 :)
diff --git a/mm/rmap.c b/mm/rmap.c index ed7f820369864..9ed995da47099 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1269,6 +1269,28 @@ static void __page_check_anon_rmap(struct folio *folio, struct page *page, page); } +static void __folio_mod_stat(struct folio *folio, int nr, int nr_pmdmapped) +{ + int idx; + + if (nr) { + idx = folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE_MAPPED; + __lruvec_stat_mod_folio(folio, idx, nr); + } + if (nr_pmdmapped) { + if (folio_test_anon(folio)) { + idx = NR_ANON_THPS; + __lruvec_stat_mod_folio(folio, idx, nr_pmdmapped); + } else { + /* NR_*_PMDMAPPED are not maintained per-memcg */ + idx = folio_test_swapbacked(folio) ? + NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED; + __mod_node_page_state(folio_pgdat(folio), idx, + nr_pmdmapped); + } + } +} + static __always_inline void __folio_add_anon_rmap(struct folio *folio, struct page *page, int nr_pages, struct vm_area_struct *vma, unsigned long address, rmap_t flags, enum rmap_level level) @@ -1276,10 +1298,6 @@ static __always_inline void __folio_add_anon_rmap(struct folio *folio, int i, nr, nr_pmdmapped = 0; nr = __folio_add_rmap(folio, page, nr_pages, level, &nr_pmdmapped); - if (nr_pmdmapped) - __lruvec_stat_mod_folio(folio, NR_ANON_THPS, nr_pmdmapped); - if (nr) - __lruvec_stat_mod_folio(folio, NR_ANON_MAPPED, nr); if (unlikely(!folio_test_anon(folio))) { VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio); @@ -1297,6 +1315,8 @@ static __always_inline void __folio_add_anon_rmap(struct folio *folio, __page_check_anon_rmap(folio, page, vma, address); } + __folio_mod_stat(folio, nr, nr_pmdmapped); + if (flags & RMAP_EXCLUSIVE) { switch (level) { case RMAP_LEVEL_PTE: @@ -1393,6 +1413,7 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma, unsigned long address) { int nr = folio_nr_pages(folio); + int nr_pmdmapped = 0; VM_WARN_ON_FOLIO(folio_test_hugetlb(folio), folio); VM_BUG_ON_VMA(address < vma->vm_start || @@ -1425,27 +1446,22 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma, atomic_set(&folio->_large_mapcount, 0); atomic_set(&folio->_nr_pages_mapped, ENTIRELY_MAPPED); SetPageAnonExclusive(&folio->page); - __lruvec_stat_mod_folio(folio, NR_ANON_THPS, nr); + nr_pmdmapped = nr; } - __lruvec_stat_mod_folio(folio, NR_ANON_MAPPED, nr); + __folio_mod_stat(folio, nr, nr_pmdmapped); } static __always_inline void __folio_add_file_rmap(struct folio *folio, struct page *page, int nr_pages, struct vm_area_struct *vma, enum rmap_level level) { - pg_data_t *pgdat = folio_pgdat(folio); int nr, nr_pmdmapped = 0; VM_WARN_ON_FOLIO(folio_test_anon(folio), folio); nr = __folio_add_rmap(folio, page, nr_pages, level, &nr_pmdmapped); - if (nr_pmdmapped) - __mod_node_page_state(pgdat, folio_test_swapbacked(folio) ? - NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED, nr_pmdmapped); - if (nr) - __lruvec_stat_mod_folio(folio, NR_FILE_MAPPED, nr); + __folio_mod_stat(folio, nr, nr_pmdmapped); /* See comments in folio_add_anon_rmap_*() */ if (!folio_test_large(folio)) @@ -1494,10 +1510,8 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, enum rmap_level level) { atomic_t *mapped = &folio->_nr_pages_mapped; - pg_data_t *pgdat = folio_pgdat(folio); int last, nr = 0, nr_pmdmapped = 0; bool partially_mapped = false; - enum node_stat_item idx; __folio_rmap_sanity_checks(folio, page, nr_pages, level); @@ -1541,20 +1555,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, break; } - 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); - /* * Queue anon large folio for deferred split if at least one * page of the folio is unmapped and at least one page @@ -1566,6 +1567,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, list_empty(&folio->_deferred_list)) deferred_split_folio(folio); } + __folio_mod_stat(folio, nr, nr_pmdmapped); /* * It would be tidy to reset folio_test_anon mapping when fully
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. No functional change intended. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- This applies on top of "mm: do not update memcg stats for NR_{FILE/SHMEM}_PMDMAPPED": https://lore.kernel.org/lkml/20240506192924.271999-1-yosryahmed@google.com/ David, I was on the fence about adding a Suggested-by here. You did suggest adding a helper, but the one with the extra folio_test_anon() was my idea and I didn't want to blame it on you. So I'll leave this up to you :) --- mm/rmap.c | 56 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 27 deletions(-)