| Message ID | 1557038457-25924-1-git-send-email-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/memcontrol: avoid unnecessary PageTransHuge() when counting compound page | expand |
On Sun 05-05-19 14:40:57, Yafang Shao wrote: > If CONFIG_TRANSPARENT_HUGEPAGE is not set, hpage_nr_pages() is always 1; > if CONFIG_TRANSPARENT_HUGEPAGE is set, hpage_nr_pages() will > call PageTransHuge() to judge whether the page is compound page or not. > So we can use the result of hpage_nr_pages() to avoid uneccessary > PageTransHuge(). The changelog doesn't describe motivation. Does this result in a better code/performance? > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > mm/memcontrol.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 2535e54..65c6f7c 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -6306,7 +6306,6 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage) > { > struct mem_cgroup *memcg; > unsigned int nr_pages; > - bool compound; > unsigned long flags; > > VM_BUG_ON_PAGE(!PageLocked(oldpage), oldpage); > @@ -6328,8 +6327,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage) > return; > > /* Force-charge the new page. The old one will be freed soon */ > - compound = PageTransHuge(newpage); > - nr_pages = compound ? hpage_nr_pages(newpage) : 1; > + nr_pages = hpage_nr_pages(newpage); > > page_counter_charge(&memcg->memory, nr_pages); > if (do_memsw_account()) > @@ -6339,7 +6337,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage) > commit_charge(newpage, memcg, false); > > local_irq_save(flags); > - mem_cgroup_charge_statistics(memcg, newpage, compound, nr_pages); > + mem_cgroup_charge_statistics(memcg, newpage, nr_pages > 1, nr_pages); > memcg_check_events(memcg, newpage); > local_irq_restore(flags); > } > @@ -6533,6 +6531,7 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry) > struct mem_cgroup *memcg, *swap_memcg; > unsigned int nr_entries; > unsigned short oldid; > + bool compound; > > VM_BUG_ON_PAGE(PageLRU(page), page); > VM_BUG_ON_PAGE(page_count(page), page); > @@ -6553,8 +6552,9 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry) > */ > swap_memcg = mem_cgroup_id_get_online(memcg); > nr_entries = hpage_nr_pages(page); > + compound = nr_entries > 1; > /* Get references for the tail pages, too */ > - if (nr_entries > 1) > + if (compound) > mem_cgroup_id_get_many(swap_memcg, nr_entries - 1); > oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg), > nr_entries); > @@ -6579,8 +6579,7 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry) > * only synchronisation we have for updating the per-CPU variables. > */ > VM_BUG_ON(!irqs_disabled()); > - mem_cgroup_charge_statistics(memcg, page, PageTransHuge(page), > - -nr_entries); > + mem_cgroup_charge_statistics(memcg, page, compound, -nr_entries); > memcg_check_events(memcg, page); > > if (!mem_cgroup_is_root(memcg)) > -- > 1.8.3.1
On Mon, May 6, 2019 at 9:59 PM Michal Hocko <mhocko@suse.com> wrote: > > On Sun 05-05-19 14:40:57, Yafang Shao wrote: > > If CONFIG_TRANSPARENT_HUGEPAGE is not set, hpage_nr_pages() is always 1; > > if CONFIG_TRANSPARENT_HUGEPAGE is set, hpage_nr_pages() will > > call PageTransHuge() to judge whether the page is compound page or not. > > So we can use the result of hpage_nr_pages() to avoid uneccessary > > PageTransHuge(). > > The changelog doesn't describe motivation. Does this result in a better > code/performance? > It is a better code, I think. Regarding the performance, I don't think it is easy to measure. Thanks Yafang
On Mon 06-05-19 23:22:11, Yafang Shao wrote: > On Mon, May 6, 2019 at 9:59 PM Michal Hocko <mhocko@suse.com> wrote: > > > > On Sun 05-05-19 14:40:57, Yafang Shao wrote: > > > If CONFIG_TRANSPARENT_HUGEPAGE is not set, hpage_nr_pages() is always 1; > > > if CONFIG_TRANSPARENT_HUGEPAGE is set, hpage_nr_pages() will > > > call PageTransHuge() to judge whether the page is compound page or not. > > > So we can use the result of hpage_nr_pages() to avoid uneccessary > > > PageTransHuge(). > > > > The changelog doesn't describe motivation. Does this result in a better > > code/performance? > > > > It is a better code, I think. > Regarding the performance, I don't think it is easy to measure. I am not convinced the patch is worth it. The code aesthetic is a matter of taste. On the other hand, the change will be an additional step in the git history so git blame take an additional step to get to the original commit which is a bit annoying. Also every change, even a trivially looking one, can cause surprising side effects. These are all arguments make a change to the code. So unless the resulting code is really much more cleaner, easier to read or maintain, or it is a part of a larger series that makes further steps easier,then I would prefer not touching the code.
Michal Hocko writes: >On Mon 06-05-19 23:22:11, Yafang Shao wrote: >> It is a better code, I think. >> Regarding the performance, I don't think it is easy to measure. > >I am not convinced the patch is worth it. The code aesthetic is a matter >of taste. On the other hand, the change will be an additional step in >the git history so git blame take an additional step to get to the >original commit which is a bit annoying. Also every change, even a >trivially looking one, can cause surprising side effects. These are all >arguments make a change to the code. > >So unless the resulting code is really much more cleaner, easier to read >or maintain, or it is a part of a larger series that makes further steps >easier,then I would prefer not touching the code. Aside from what Michal already said, which I agree with, when skimming code reading PageTransHuge has much clearer intent to me than checking nr_pages. We already have a non-trivial number of checks which are unclear at first glance in the mm code and, while this isn't nearly as bad as some of those, and might not make the situation much worse, I also don't think changing to nr_pages checks makes the situation any better, either.
On Tue, May 7, 2019 at 10:21 PM Chris Down <chris@chrisdown.name> wrote: > > Michal Hocko writes: > >On Mon 06-05-19 23:22:11, Yafang Shao wrote: > >> It is a better code, I think. > >> Regarding the performance, I don't think it is easy to measure. > > > >I am not convinced the patch is worth it. The code aesthetic is a matter > >of taste. On the other hand, the change will be an additional step in > >the git history so git blame take an additional step to get to the > >original commit which is a bit annoying. Also every change, even a > >trivially looking one, can cause surprising side effects. These are all > >arguments make a change to the code. > > > >So unless the resulting code is really much more cleaner, easier to read > >or maintain, or it is a part of a larger series that makes further steps > >easier,then I would prefer not touching the code. > > Aside from what Michal already said, which I agree with, when skimming code > reading PageTransHuge has much clearer intent to me than checking nr_pages. We > already have a non-trivial number of checks which are unclear at first glance > in the mm code and, while this isn't nearly as bad as some of those, and might > not make the situation much worse, I also don't think changing to nr_pages > checks makes the situation any better, either. I agree with dropping this patch, but I don't agree with your opinion that PageTransHuge() can make the code clear. The motivation I send this patch is because 'compound' and 'PageTransHuge' confused me. I prefer to remove the paratmeter 'compound' compeletely from some functions(i.e. mem_cgroup_commit_charge, mem_cgroup_migrate, mem_cgroup_swapout, mem_cgroup_move_account) in memcontrol.c completely, because whether this page is compound or not doesn't depends on the callsite, but only depends on the page itself. I mean we can use the page to judge whether the page is compound or not. I didn't do that because I'm not sure whether it is worth. The other reason comfused me is compound page may not be thp. Of course it can only be thp in the current callsites. Maybe 'thp' is better than 'compound'. Thanks Yafang
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 2535e54..65c6f7c 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6306,7 +6306,6 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage) { struct mem_cgroup *memcg; unsigned int nr_pages; - bool compound; unsigned long flags; VM_BUG_ON_PAGE(!PageLocked(oldpage), oldpage); @@ -6328,8 +6327,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage) return; /* Force-charge the new page. The old one will be freed soon */ - compound = PageTransHuge(newpage); - nr_pages = compound ? hpage_nr_pages(newpage) : 1; + nr_pages = hpage_nr_pages(newpage); page_counter_charge(&memcg->memory, nr_pages); if (do_memsw_account()) @@ -6339,7 +6337,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage) commit_charge(newpage, memcg, false); local_irq_save(flags); - mem_cgroup_charge_statistics(memcg, newpage, compound, nr_pages); + mem_cgroup_charge_statistics(memcg, newpage, nr_pages > 1, nr_pages); memcg_check_events(memcg, newpage); local_irq_restore(flags); } @@ -6533,6 +6531,7 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry) struct mem_cgroup *memcg, *swap_memcg; unsigned int nr_entries; unsigned short oldid; + bool compound; VM_BUG_ON_PAGE(PageLRU(page), page); VM_BUG_ON_PAGE(page_count(page), page); @@ -6553,8 +6552,9 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry) */ swap_memcg = mem_cgroup_id_get_online(memcg); nr_entries = hpage_nr_pages(page); + compound = nr_entries > 1; /* Get references for the tail pages, too */ - if (nr_entries > 1) + if (compound) mem_cgroup_id_get_many(swap_memcg, nr_entries - 1); oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg), nr_entries); @@ -6579,8 +6579,7 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry) * only synchronisation we have for updating the per-CPU variables. */ VM_BUG_ON(!irqs_disabled()); - mem_cgroup_charge_statistics(memcg, page, PageTransHuge(page), - -nr_entries); + mem_cgroup_charge_statistics(memcg, page, compound, -nr_entries); memcg_check_events(memcg, page); if (!mem_cgroup_is_root(memcg))
If CONFIG_TRANSPARENT_HUGEPAGE is not set, hpage_nr_pages() is always 1; if CONFIG_TRANSPARENT_HUGEPAGE is set, hpage_nr_pages() will call PageTransHuge() to judge whether the page is compound page or not. So we can use the result of hpage_nr_pages() to avoid uneccessary PageTransHuge(). Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- mm/memcontrol.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)