On Mon, Jun 24, 2019 at 8:33 PM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote: > > On 24.06.2019 15:30, Yafang Shao wrote: > > On Mon, Jun 24, 2019 at 4:53 PM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote: > >> > >> On 21.06.2019 13:14, Yafang Shao wrote: > >>> There're six different reclaim paths by now, > >>> - kswapd reclaim path > >>> - node reclaim path > >>> - hibernate preallocate memory reclaim path > >>> - direct reclaim path > >>> - memcg reclaim path > >>> - memcg softlimit reclaim path > >>> > >>> The slab caches reclaimed in these paths are only calculated in the above > >>> three paths. > >>> > >>> There're some drawbacks if we don't calculate the reclaimed slab caches. > >>> - The sc->nr_reclaimed isn't correct if there're some slab caches > >>> relcaimed in this path. > >>> - The slab caches may be reclaimed thoroughly if there're lots of > >>> reclaimable slab caches and few page caches. > >>> Let's take an easy example for this case. > >>> If one memcg is full of slab caches and the limit of it is 512M, in > >>> other words there're approximately 512M slab caches in this memcg. > >>> Then the limit of the memcg is reached and the memcg reclaim begins, > >>> and then in this memcg reclaim path it will continuesly reclaim the > >>> slab caches until the sc->priority drops to 0. > >>> After this reclaim stops, you will find there're few slab caches left, > >>> which is less than 20M in my test case. > >>> While after this patch applied the number is greater than 300M and > >>> the sc->priority only drops to 3. > >>> > >>> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx> > >>> --- > >>> mm/vmscan.c | 7 +++++++ > >>> 1 file changed, 7 insertions(+) > >>> > >>> diff --git a/mm/vmscan.c b/mm/vmscan.c > >>> index 18a66e5..d6c3fc8 100644 > >>> --- a/mm/vmscan.c > >>> +++ b/mm/vmscan.c > >>> @@ -3164,11 +3164,13 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order, > >>> if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask)) > >>> return 1; > >>> > >>> + current->reclaim_state = &sc.reclaim_state; > >>> trace_mm_vmscan_direct_reclaim_begin(order, sc.gfp_mask); > >>> > >>> nr_reclaimed = do_try_to_free_pages(zonelist, &sc); > >>> > >>> trace_mm_vmscan_direct_reclaim_end(nr_reclaimed); > >>> + current->reclaim_state = NULL; > >> > >> Shouldn't we remove reclaim_state assignment from __perform_reclaim() after this? > >> > > > > Oh yes. We should remove it. Thanks for pointing out. > > I will post a fix soon. > > With the change above, feel free to add my Reviewed-by: to all of the series. > Sure, thanks for your review. Thanks Yafang
