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

[Matthew Wilcox <willy@infradead.org>]

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.

Thank you for this better changelog; I have a better idea about what
this patch is doing now.

> > @@ -276,9 +276,9 @@ static void unregister_memcg_shrinker(struct shrinker *shrinker)
> > {
> > }
> > 
> > -static bool cgroup_reclaim(struct scan_control *sc)
> > +static struct mem_cgroup *cgroup_reclaim(struct scan_control *sc)
> > {
> > -	return false;
> > +	return NULL;
> > }

I think this is actually the important bit.  For those who build
their kernels with cgroups disabled, it will save a small number of
instructions since cgroup_reclaim() will be NULL rather than dereferencing
sc->target_mem_group.  It'd be nice to have that saving quantified as
part of the changelog.

> > @@ -2625,7 +2626,7 @@ static inline bool should_continue_reclaim(struct pglist_data *pgdat,
> > 
> > static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc)
> > {
> > -	struct mem_cgroup *target_memcg = sc->target_mem_cgroup;
> > +	struct mem_cgroup *target_memcg = cgroup_reclaim(sc);

It feels like the name is wrong, doesn't it?  cgroup_reclaim() doesn't
really scream to me "I return a mem_cgroup pointer".  I can't think of
a good name, but maybe someone else can.