| Message ID | 20220225003437.12620-4-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memcg: some cleanup for mem_cgroup_iter() | expand |
On Fri, Feb 25, 2022 at 12:34:37AM +0000, Wei Yang wrote: > For each round-trip, we assign generation on first invocation and > compare it on subsequent invocations. > > Let's move them together to make it more self-explaining. Also this > reduce a check on prev. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> This makes sense. The function is structured into 1) load state, 2) advance, 3) save state. The load state is a better fit for initializing reclaim->generation. > @@ -996,7 +996,14 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root, > mz = root->nodeinfo[reclaim->pgdat->node_id]; > iter = &mz->iter; > > - if (prev && reclaim->generation != iter->generation) > + /* > + * On first invocation, assign iter->generation to > + * reclaim->generation. > + * On subsequent invocations, make sure no one else jump in. > + */ > + if (!prev) > + reclaim->generation = iter->generation; > + else if (reclaim->generation != iter->generation) > goto out_unlock; The comment duplicates the code, it doesn't explain why we're doing this. How about: /* * On start, join the current reclaim iteration cycle. * Exit when a concurrent walker completes it. */ With that, please feel free to add: Acked-by: Johannes Weiner <hannes@cmpxchg.org>
On Wed, Mar 30, 2022 at 11:57:07AM -0400, Johannes Weiner wrote: >On Fri, Feb 25, 2022 at 12:34:37AM +0000, Wei Yang wrote: >> For each round-trip, we assign generation on first invocation and >> compare it on subsequent invocations. >> >> Let's move them together to make it more self-explaining. Also this >> reduce a check on prev. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >This makes sense. The function is structured into 1) load state, 2) >advance, 3) save state. The load state is a better fit for >initializing reclaim->generation. > >> @@ -996,7 +996,14 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root, >> mz = root->nodeinfo[reclaim->pgdat->node_id]; >> iter = &mz->iter; >> >> - if (prev && reclaim->generation != iter->generation) >> + /* >> + * On first invocation, assign iter->generation to >> + * reclaim->generation. >> + * On subsequent invocations, make sure no one else jump in. >> + */ >> + if (!prev) >> + reclaim->generation = iter->generation; >> + else if (reclaim->generation != iter->generation) >> goto out_unlock; > >The comment duplicates the code, it doesn't explain why we're doing >this. How about: > > /* > * On start, join the current reclaim iteration cycle. > * Exit when a concurrent walker completes it. > */ This one looks better and explain the reclaim model. Thanks > >With that, please feel free to add: > >Acked-by: Johannes Weiner <hannes@cmpxchg.org>
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 03399146168f..17da93c2f94e 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -996,7 +996,14 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root, mz = root->nodeinfo[reclaim->pgdat->node_id]; iter = &mz->iter; - if (prev && reclaim->generation != iter->generation) + /* + * On first invocation, assign iter->generation to + * reclaim->generation. + * On subsequent invocations, make sure no one else jump in. + */ + if (!prev) + reclaim->generation = iter->generation; + else if (reclaim->generation != iter->generation) goto out_unlock; while (1) { @@ -1056,8 +1063,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root, if (!memcg) iter->generation++; - else if (!prev) - reclaim->generation = iter->generation; } out_unlock:
For each round-trip, we assign generation on first invocation and compare it on subsequent invocations. Let's move them together to make it more self-explaining. Also this reduce a check on prev. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- mm/memcontrol.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)