Re: [PATCH] mm/vmscan: fix incorrect return type for cgroup_reclaim()

[chenqiwu <qiwuchen55@gmail.com>]

On Thu, Mar 19, 2020 at 04:20:25PM +0000, Chris Down wrote:
> qiwuchen55@gmail.com writes:
> >From: chenqiwu <chenqiwu@xiaomi.com>
> >
> >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@xiaomi.com>
> 
> After improving the patch title and summary, you can add:
> 
> Acked-by: Chris Down <chris@chrisdown.name>
>
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