| Message ID | 20210622121551.3398730-13-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Folio-enabling the page cache | expand |
On Tue, Jun 22, 2021 at 01:15:17PM +0100, Matthew Wilcox (Oracle) wrote: > static struct mem_cgroup_per_node * > -mem_cgroup_page_nodeinfo(struct mem_cgroup *memcg, struct page *page) > +mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid) > { > - int nid = page_to_nid(page); > - > return memcg->nodeinfo[nid]; > } I'd just kill this function entirely and open code it into the only caller > - mctz = soft_limit_tree_from_page(page); > + mctz = soft_limit_tree_node(nid); And while were at it, soft_limit_tree_node seems like a completely pointless helper that does nothing but obsfucating the code. While you touch this area it might be worth to spin another patch to just remove it as well.
On Wed, Jun 23, 2021 at 10:12:40AM +0200, Christoph Hellwig wrote: > On Tue, Jun 22, 2021 at 01:15:17PM +0100, Matthew Wilcox (Oracle) wrote: > > static struct mem_cgroup_per_node * > > -mem_cgroup_page_nodeinfo(struct mem_cgroup *memcg, struct page *page) > > +mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid) > > { > > - int nid = page_to_nid(page); > > - > > return memcg->nodeinfo[nid]; > > } > > I'd just kill this function entirely and open code it into the only > caller Done. > > - mctz = soft_limit_tree_from_page(page); > > + mctz = soft_limit_tree_node(nid); > > And while were at it, soft_limit_tree_node seems like a completely > pointless helper that does nothing but obsfucating the code. While > you touch this area it might be worth to spin another patch to just > remove it as well. I'm scared that if I touch this file too much, people will start to think I know something about memcgs. Happy to add it on; cc'ing maintainers.
On Thu 24-06-21 17:19:53, Matthew Wilcox wrote: > On Wed, Jun 23, 2021 at 10:12:40AM +0200, Christoph Hellwig wrote: > > On Tue, Jun 22, 2021 at 01:15:17PM +0100, Matthew Wilcox (Oracle) wrote: > > > static struct mem_cgroup_per_node * > > > -mem_cgroup_page_nodeinfo(struct mem_cgroup *memcg, struct page *page) > > > +mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid) > > > { > > > - int nid = page_to_nid(page); > > > - > > > return memcg->nodeinfo[nid]; > > > } > > > > I'd just kill this function entirely and open code it into the only > > caller > > Done. This makes sense. > > > - mctz = soft_limit_tree_from_page(page); > > > + mctz = soft_limit_tree_node(nid); > > > > And while were at it, soft_limit_tree_node seems like a completely > > pointless helper that does nothing but obsfucating the code. While > > you touch this area it might be worth to spin another patch to just > > remove it as well. Yeah, the whole soft limit reclaim code is kinda pain to even look at. Opencoding those two will certainly not make it worse so fine with me. > I'm scared that if I touch this file too much, people will start to > think I know something about memcgs. Happy to add it on; cc'ing > maintainers. get_maintainers will surely notice ;)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 1204c6a0c671..7423cb11eb88 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -453,10 +453,8 @@ ino_t page_cgroup_ino(struct page *page) } static struct mem_cgroup_per_node * -mem_cgroup_page_nodeinfo(struct mem_cgroup *memcg, struct page *page) +mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid) { - int nid = page_to_nid(page); - return memcg->nodeinfo[nid]; } @@ -466,14 +464,6 @@ soft_limit_tree_node(int nid) return soft_limit_tree.rb_tree_per_node[nid]; } -static struct mem_cgroup_tree_per_node * -soft_limit_tree_from_page(struct page *page) -{ - int nid = page_to_nid(page); - - return soft_limit_tree.rb_tree_per_node[nid]; -} - static void __mem_cgroup_insert_exceeded(struct mem_cgroup_per_node *mz, struct mem_cgroup_tree_per_node *mctz, unsigned long new_usage_in_excess) @@ -549,8 +539,9 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, struct page *page) unsigned long excess; struct mem_cgroup_per_node *mz; struct mem_cgroup_tree_per_node *mctz; + int nid = page_to_nid(page); - mctz = soft_limit_tree_from_page(page); + mctz = soft_limit_tree_node(nid); if (!mctz) return; /* @@ -558,7 +549,7 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, struct page *page) * because their event counter is not touched. */ for (; memcg; memcg = parent_mem_cgroup(memcg)) { - mz = mem_cgroup_page_nodeinfo(memcg, page); + mz = mem_cgroup_nodeinfo(memcg, nid); excess = soft_limit_excess(memcg); /* * We have to update the tree if mz is on RB-tree or
Hoist the page_to_nid() call from mem_cgroup_page_nodeinfo() into mem_cgroup_update_tree(). That lets us call soft_limit_tree_node() and delete soft_limit_tree_from_page() altogether. Saves 42 bytes of kernel text on my config. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- mm/memcontrol.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)