| Message ID | 20240827230753.2073580-3-kinseyho@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improve mem_cgroup_iter() | expand |
On Tue, Aug 27, 2024 at 4:11 PM Kinsey Ho <kinseyho@google.com> wrote: > > To obtain the pointer to the next memcg position, mem_cgroup_iter() > currently holds css->refcnt during memcg traversal only to put > css->refcnt at the end of the routine. This isn't necessary as an > rcu_read_lock is already held throughout the function. The use of > the RCU read lock with css_next_descendant_pre() guarantees that > sibling linkage is safe without holding a ref on the passed-in @css. > > Remove css->refcnt usage during traversal by leveraging RCU. > > Signed-off-by: Kinsey Ho <kinseyho@google.com> Reviewed-by: T.J. Mercier <tjmercier@google.com> I found a different place where a more trivial css get/put pair than this could be removed, but I couldn't measure a perf difference. Like Yosry, I appreciate the simplicity gains here though. > --- > mm/memcontrol.c | 18 +----------------- > 1 file changed, 1 insertion(+), 17 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 35431035e782..67b1994377b7 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1013,20 +1013,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root, > else if (reclaim->generation != iter->generation) > goto out_unlock; > > - while (1) { > - pos = READ_ONCE(iter->position); > - if (!pos || css_tryget(&pos->css)) > - break; > - /* > - * css reference reached zero, so iter->position will > - * be cleared by ->css_released. However, we should not > - * rely on this happening soon, because ->css_released > - * is called from a work queue, and by busy-waiting we > - * might block it. So we clear iter->position right > - * away. > - */ > - (void)cmpxchg(&iter->position, pos, NULL); > - } > + pos = READ_ONCE(iter->position); > } else if (prev) { > pos = prev; > } > @@ -1067,9 +1054,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root, > */ > (void)cmpxchg(&iter->position, pos, memcg); > > - if (pos) > - css_put(&pos->css); > - > if (!memcg) > iter->generation++; > } > -- > 2.46.0.295.g3b9ea8a38a-goog > >
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 35431035e782..67b1994377b7 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -1013,20 +1013,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root, else if (reclaim->generation != iter->generation) goto out_unlock; - while (1) { - pos = READ_ONCE(iter->position); - if (!pos || css_tryget(&pos->css)) - break; - /* - * css reference reached zero, so iter->position will - * be cleared by ->css_released. However, we should not - * rely on this happening soon, because ->css_released - * is called from a work queue, and by busy-waiting we - * might block it. So we clear iter->position right - * away. - */ - (void)cmpxchg(&iter->position, pos, NULL); - } + pos = READ_ONCE(iter->position); } else if (prev) { pos = prev; } @@ -1067,9 +1054,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root, */ (void)cmpxchg(&iter->position, pos, memcg); - if (pos) - css_put(&pos->css); - if (!memcg) iter->generation++; }
To obtain the pointer to the next memcg position, mem_cgroup_iter() currently holds css->refcnt during memcg traversal only to put css->refcnt at the end of the routine. This isn't necessary as an rcu_read_lock is already held throughout the function. The use of the RCU read lock with css_next_descendant_pre() guarantees that sibling linkage is safe without holding a ref on the passed-in @css. Remove css->refcnt usage during traversal by leveraging RCU. Signed-off-by: Kinsey Ho <kinseyho@google.com> --- mm/memcontrol.c | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-)