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.
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.
