On Thu, Mar 19, 2020 at 04:20:25PM +0000, Chris Down wrote: > qiwuchen55@xxxxxxxxx writes: > >From: chenqiwu <chenqiwu@xxxxxxxxxx> > > > >The return type of cgroup_reclaim() is bool, but the correct type > >should be struct mem_cgroup pointer. As a result, cgroup_reclaim() > >can be used to wrap sc->target_mem_cgroup in vmscan code. > > The code changes looks okay to me, but the changelog and patch > subject could do with some improvement. For example, the type isn't > "incorrect" before and "correct" now, per se, it's just coercing > from *struct mem_cgroup to bool. > > Could you make it more clear what your intent is in the patch? If > it's that you found the code confusing, you can just write something > like: > > mm/vmscan: return target_mem_cgroup from cgroup_reclaim > > Previously the code splits apart the action of checking whether > we are in cgroup reclaim from getting the target memory cgroup > for that reclaim. This split is confusing and seems unnecessary, > since cgroup_reclaim itself only checks if sc->target_mem_cgroup > is NULL or not, so merge the two use cases into one by returning > the target_mem_cgroup itself from cgroup_reclaim. > > >Signed-off-by: chenqiwu <chenqiwu@xxxxxxxxxx> > > After improving the patch title and summary, you can add: > > Acked-by: Chris Down <chris@xxxxxxxxxxxxxx> > Hi Chris, Thanks for your review. I will resend this as patch v2. BTW, sc->target_mem_cgroup is just used when CONFIG_MEMCG=y, so wrap it with config CONFIG_MEMCG will save some space for those who build their kernels with cgroups disabled. Qiwu
