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@xxxxxxxxx> 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@xxxxxxxxxxx>
