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@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. > */ This one looks better and explain the reclaim model. Thanks > >With that, please feel free to add: > >Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx> -- Wei Yang Help you, Help me
