On Tue, Oct 22, 2019 at 07:40:52PM +0000, Roman Gushchin wrote:
> On Tue, Oct 22, 2019 at 10:47:59AM -0400, Johannes Weiner wrote:
> > Seven years after introducing the global_reclaim() function, I still
> > have to double take when reading a callsite. I don't know how others
> > do it, this is a terrible name.
> >
> > Invert the meaning and rename it to cgroup_reclaim().
> >
> > [ After all, "global reclaim" is just regular reclaim invoked from the
> > page allocator. It's reclaim on behalf of a cgroup limit that is a
> > special case of reclaim, and should be explicit - not the reverse. ]
> >
> > sane_reclaim() isn't very descriptive either: it tests whether we can
> > use the regular writeback throttling - available during regular page
> > reclaim or cgroup2 limit reclaim - or need to use the broken
> > wait_on_page_writeback() method. Use "writeback_throttling_sane()".
> >
> > Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> > ---
> > mm/vmscan.c | 38 ++++++++++++++++++--------------------
> > 1 file changed, 18 insertions(+), 20 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 622b77488144..302dad112f75 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -239,13 +239,13 @@ static void unregister_memcg_shrinker(struct shrinker *shrinker)
> > up_write(&shrinker_rwsem);
> > }
> >
> > -static bool global_reclaim(struct scan_control *sc)
> > +static bool cgroup_reclaim(struct scan_control *sc)
> > {
> > - return !sc->target_mem_cgroup;
> > + return sc->target_mem_cgroup;
> > }
>
> Isn't targeted_reclaim() better?
>
> cgroup_reclaim() is also ok to me, but it sounds a bit like we reclaim
> from this specific cgroup. Also targeted/global is IMO a better opposition
> than cgroup/global (the latter reminds me days when there were global
> and cgroup LRUs).
I think "targeted" is quite a bit less descriptive when you come at
the page replacement algorithm without cgroups in mind.
> The rest of the patch looks good!
>
> Reviewed-by: Roman Gushchin <guro@xxxxxx>
Thanks!
