diff mbox series

[v7,09/12] mm: vmscan: use per memcg nr_deferred of shrinker

Message ID 20210209174646.1310591-10-shy828301@gmail.com (mailing list archive)
State New
Headers show
Series [v7,01/12] mm: vmscan: use nid from shrink_control for tracepoint | expand

Commit Message

Yang Shi Feb. 9, 2021, 5:46 p.m. UTC
Use per memcg's nr_deferred for memcg aware shrinkers.  The shrinker's nr_deferred
will be used in the following cases:
    1. Non memcg aware shrinkers
    2. !CONFIG_MEMCG
    3. memcg is disabled by boot parameter

Signed-off-by: Yang Shi <shy828301@gmail.com>
---
 mm/vmscan.c | 78 ++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 66 insertions(+), 12 deletions(-)

Comments

Roman Gushchin Feb. 10, 2021, 1:27 a.m. UTC | #1
On Tue, Feb 09, 2021 at 09:46:43AM -0800, Yang Shi wrote:
> Use per memcg's nr_deferred for memcg aware shrinkers.  The shrinker's nr_deferred
> will be used in the following cases:
>     1. Non memcg aware shrinkers
>     2. !CONFIG_MEMCG
>     3. memcg is disabled by boot parameter
> 
> Signed-off-by: Yang Shi <shy828301@gmail.com>
> ---
>  mm/vmscan.c | 78 ++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 66 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index d4b030a0b2a9..748aa6e90f83 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -368,6 +368,24 @@ static void unregister_memcg_shrinker(struct shrinker *shrinker)
>  	up_write(&shrinker_rwsem);
>  }
>  
> +static long count_nr_deferred_memcg(int nid, struct shrinker *shrinker,
> +				    struct mem_cgroup *memcg)

"Count" is not associated with xchg() semantics in my head, but I don't know
what's the better version. Maybe steal or pop?

Otherwise the patch looks good to me.

Thanks!
Yang Shi Feb. 10, 2021, 1:52 a.m. UTC | #2
On Tue, Feb 9, 2021 at 5:27 PM Roman Gushchin <guro@fb.com> wrote:
>
> On Tue, Feb 09, 2021 at 09:46:43AM -0800, Yang Shi wrote:
> > Use per memcg's nr_deferred for memcg aware shrinkers.  The shrinker's nr_deferred
> > will be used in the following cases:
> >     1. Non memcg aware shrinkers
> >     2. !CONFIG_MEMCG
> >     3. memcg is disabled by boot parameter
> >
> > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > ---
> >  mm/vmscan.c | 78 ++++++++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 66 insertions(+), 12 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index d4b030a0b2a9..748aa6e90f83 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -368,6 +368,24 @@ static void unregister_memcg_shrinker(struct shrinker *shrinker)
> >       up_write(&shrinker_rwsem);
> >  }
> >
> > +static long count_nr_deferred_memcg(int nid, struct shrinker *shrinker,
> > +                                 struct mem_cgroup *memcg)
>
> "Count" is not associated with xchg() semantics in my head, but I don't know
> what's the better version. Maybe steal or pop?

It is used to retrieve the nr_deferred value. I don't think "steal" or
"pop" helps understand. Actually "count" is borrowed from
count_objects() method of shrinker.

>
> Otherwise the patch looks good to me.
>
> Thanks!
Kirill Tkhai Feb. 10, 2021, 2:36 p.m. UTC | #3
On 10.02.2021 04:52, Yang Shi wrote:
> On Tue, Feb 9, 2021 at 5:27 PM Roman Gushchin <guro@fb.com> wrote:
>>
>> On Tue, Feb 09, 2021 at 09:46:43AM -0800, Yang Shi wrote:
>>> Use per memcg's nr_deferred for memcg aware shrinkers.  The shrinker's nr_deferred
>>> will be used in the following cases:
>>>     1. Non memcg aware shrinkers
>>>     2. !CONFIG_MEMCG
>>>     3. memcg is disabled by boot parameter
>>>
>>> Signed-off-by: Yang Shi <shy828301@gmail.com>
>>> ---
>>>  mm/vmscan.c | 78 ++++++++++++++++++++++++++++++++++++++++++++---------
>>>  1 file changed, 66 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>> index d4b030a0b2a9..748aa6e90f83 100644
>>> --- a/mm/vmscan.c
>>> +++ b/mm/vmscan.c
>>> @@ -368,6 +368,24 @@ static void unregister_memcg_shrinker(struct shrinker *shrinker)
>>>       up_write(&shrinker_rwsem);
>>>  }
>>>
>>> +static long count_nr_deferred_memcg(int nid, struct shrinker *shrinker,
>>> +                                 struct mem_cgroup *memcg)
>>
>> "Count" is not associated with xchg() semantics in my head, but I don't know
>> what's the better version. Maybe steal or pop?
> 
> It is used to retrieve the nr_deferred value. I don't think "steal" or
> "pop" helps understand. Actually "count" is borrowed from
> count_objects() method of shrinker.

I'd also voted for another name.

xchg_nr_deferred() or steal/pop sound better for me.
Yang Shi Feb. 10, 2021, 4:41 p.m. UTC | #4
On Wed, Feb 10, 2021 at 6:37 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> On 10.02.2021 04:52, Yang Shi wrote:
> > On Tue, Feb 9, 2021 at 5:27 PM Roman Gushchin <guro@fb.com> wrote:
> >>
> >> On Tue, Feb 09, 2021 at 09:46:43AM -0800, Yang Shi wrote:
> >>> Use per memcg's nr_deferred for memcg aware shrinkers.  The shrinker's nr_deferred
> >>> will be used in the following cases:
> >>>     1. Non memcg aware shrinkers
> >>>     2. !CONFIG_MEMCG
> >>>     3. memcg is disabled by boot parameter
> >>>
> >>> Signed-off-by: Yang Shi <shy828301@gmail.com>
> >>> ---
> >>>  mm/vmscan.c | 78 ++++++++++++++++++++++++++++++++++++++++++++---------
> >>>  1 file changed, 66 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/mm/vmscan.c b/mm/vmscan.c
> >>> index d4b030a0b2a9..748aa6e90f83 100644
> >>> --- a/mm/vmscan.c
> >>> +++ b/mm/vmscan.c
> >>> @@ -368,6 +368,24 @@ static void unregister_memcg_shrinker(struct shrinker *shrinker)
> >>>       up_write(&shrinker_rwsem);
> >>>  }
> >>>
> >>> +static long count_nr_deferred_memcg(int nid, struct shrinker *shrinker,
> >>> +                                 struct mem_cgroup *memcg)
> >>
> >> "Count" is not associated with xchg() semantics in my head, but I don't know
> >> what's the better version. Maybe steal or pop?
> >
> > It is used to retrieve the nr_deferred value. I don't think "steal" or
> > "pop" helps understand. Actually "count" is borrowed from
> > count_objects() method of shrinker.
>
> I'd also voted for another name.
>
> xchg_nr_deferred() or steal/pop sound better for me.

OK, I do have a hard time to understand steal/pop, but xchg sounds
more self-explained to me.

>
diff mbox series

Patch

diff --git a/mm/vmscan.c b/mm/vmscan.c
index d4b030a0b2a9..748aa6e90f83 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -368,6 +368,24 @@  static void unregister_memcg_shrinker(struct shrinker *shrinker)
 	up_write(&shrinker_rwsem);
 }
 
+static long count_nr_deferred_memcg(int nid, struct shrinker *shrinker,
+				    struct mem_cgroup *memcg)
+{
+	struct shrinker_info *info;
+
+	info = shrinker_info_protected(memcg, nid);
+	return atomic_long_xchg(&info->nr_deferred[shrinker->id], 0);
+}
+
+static long add_nr_deferred_memcg(long nr, int nid, struct shrinker *shrinker,
+				  struct mem_cgroup *memcg)
+{
+	struct shrinker_info *info;
+
+	info = shrinker_info_protected(memcg, nid);
+	return atomic_long_add_return(nr, &info->nr_deferred[shrinker->id]);
+}
+
 static bool cgroup_reclaim(struct scan_control *sc)
 {
 	return sc->target_mem_cgroup;
@@ -406,6 +424,18 @@  static void unregister_memcg_shrinker(struct shrinker *shrinker)
 {
 }
 
+static long count_nr_deferred_memcg(int nid, struct shrinker *shrinker,
+				    struct mem_cgroup *memcg)
+{
+	return 0;
+}
+
+static long add_nr_deferred_memcg(long nr, int nid, struct shrinker *shrinker,
+				  struct mem_cgroup *memcg)
+{
+	return 0;
+}
+
 static bool cgroup_reclaim(struct scan_control *sc)
 {
 	return false;
@@ -417,6 +447,39 @@  static bool writeback_throttling_sane(struct scan_control *sc)
 }
 #endif
 
+static long count_nr_deferred(struct shrinker *shrinker,
+			      struct shrink_control *sc)
+{
+	int nid = sc->nid;
+
+	if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
+		nid = 0;
+
+	if (sc->memcg &&
+	    (shrinker->flags & SHRINKER_MEMCG_AWARE))
+		return count_nr_deferred_memcg(nid, shrinker,
+					       sc->memcg);
+
+	return atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
+}
+
+
+static long add_nr_deferred(long nr, struct shrinker *shrinker,
+			    struct shrink_control *sc)
+{
+	int nid = sc->nid;
+
+	if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
+		nid = 0;
+
+	if (sc->memcg &&
+	    (shrinker->flags & SHRINKER_MEMCG_AWARE))
+		return add_nr_deferred_memcg(nr, nid, shrinker,
+					     sc->memcg);
+
+	return atomic_long_add_return(nr, &shrinker->nr_deferred[nid]);
+}
+
 /*
  * This misses isolated pages which are not accounted for to save counters.
  * As the data only determines if reclaim or compaction continues, it is
@@ -549,14 +612,10 @@  static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	long freeable;
 	long nr;
 	long new_nr;
-	int nid = shrinkctl->nid;
 	long batch_size = shrinker->batch ? shrinker->batch
 					  : SHRINK_BATCH;
 	long scanned = 0, next_deferred;
 
-	if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
-		nid = 0;
-
 	freeable = shrinker->count_objects(shrinker, shrinkctl);
 	if (freeable == 0 || freeable == SHRINK_EMPTY)
 		return freeable;
@@ -566,7 +625,7 @@  static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	 * and zero it so that other concurrent shrinker invocations
 	 * don't also do this scanning work.
 	 */
-	nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
+	nr = count_nr_deferred(shrinker, shrinkctl);
 
 	total_scan = nr;
 	if (shrinker->seeks) {
@@ -657,14 +716,9 @@  static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 		next_deferred = 0;
 	/*
 	 * move the unused scan count back into the shrinker in a
-	 * manner that handles concurrent updates. If we exhausted the
-	 * scan, there is no need to do an update.
+	 * manner that handles concurrent updates.
 	 */
-	if (next_deferred > 0)
-		new_nr = atomic_long_add_return(next_deferred,
-						&shrinker->nr_deferred[nid]);
-	else
-		new_nr = atomic_long_read(&shrinker->nr_deferred[nid]);
+	new_nr = add_nr_deferred(next_deferred, shrinker, shrinkctl);
 
 	trace_mm_shrink_slab_end(shrinker, shrinkctl->nid, freed, nr, new_nr, total_scan);
 	return freed;