re: [PATCH] mm: avoid the unnecessary waiting when force empty a cgroup

re: [PATCH] mm: avoid the unnecessary waiting when force empty a cgroup

[Li,Rongqing]

> -----邮件原件-----
> 发件人: Michal Hocko [mailto:mhocko@kernel.org]
> 发送时间: 2018年4月3日 16:05
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: hannes@cmpxchg.org; vdavydov.dev@gmail.com;
> cgroups@vger.kernel.org; linux-mm@kvack.org;
> linux-kernel@vger.kernel.org
> 主题: Re: [PATCH] mm: avoid the unnecessary waiting when force empty a
> cgroup
> 
> On Tue 03-04-18 15:12:09, Li RongQing wrote:
> > The number of writeback and dirty page can be read out from memcg, the
> > unnecessary waiting can be avoided by these counts
> 
> This changelog doesn't explain the problem and how the patch fixes it.

If a process in a memory cgroup takes some RSS, when force empty this memory cgroup, congestion_wait will be called unconditionally, there is 0.5 seconds delay

If use this patch, nearly no delay.


> Why do wee another throttling when we do already throttle in the reclaim
> path?

Do you mean we should remove congestion_wait(BLK_RW_ASYNC, HZ/10) from mem_cgroup_force_empty, since try_to_free_mem_cgroup_pages [shrink_inactive_list] has called congestion_wait


-RongQing

> 
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> >  mm/memcontrol.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c index
> > 9ec024b862ac..5258651bd4ec 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2613,9 +2613,13 @@ static int mem_cgroup_force_empty(struct
> mem_cgroup *memcg)
> >  		progress = try_to_free_mem_cgroup_pages(memcg, 1,
> >  							GFP_KERNEL, true);
> >  		if (!progress) {
> > +			unsigned long num;
> > +
> > +			num = memcg_page_state(memcg, NR_WRITEBACK) +
> > +					memcg_page_state(memcg, NR_FILE_DIRTY);
> >  			nr_retries--;
> > -			/* maybe some writeback is necessary */
> > -			congestion_wait(BLK_RW_ASYNC, HZ/10);
> > +			if (num)
> > +				congestion_wait(BLK_RW_ASYNC, HZ/10);
> >  		}
> >
> >  	}
> > --
> > 2.11.0
> 
> --
> Michal Hocko
> SUSE Labs

Re: [PATCH] mm: avoid the unnecessary waiting when force empty a cgroup

[Michal Hocko]

On Tue 03-04-18 08:29:39, Li,Rongqing wrote:
> 
> 
> > -----e?(R)a>>?a??a>>?-----
> > a??a>>?aoo: Michal Hocko [mailto:mhocko@kernel.org]
> > a??e??ae??e?': 2018a1'4ae??3ae?JPY 16:05
> > ae??a>>?aoo: Li,Rongqing <lirongqing@baidu.com>
> > ae??e??: hannes@cmpxchg.org; vdavydov.dev@gmail.com;
> > cgroups@vger.kernel.org; linux-mm@kvack.org;
> > linux-kernel@vger.kernel.org
> > a,>>ec?: Re: [PATCH] mm: avoid the unnecessary waiting when force empty a
> > cgroup
> > 
> > On Tue 03-04-18 15:12:09, Li RongQing wrote:
> > > The number of writeback and dirty page can be read out from memcg, the
> > > unnecessary waiting can be avoided by these counts
> > 
> > This changelog doesn't explain the problem and how the patch fixes it.
> 
> If a process in a memory cgroup takes some RSS, when force empty this
> memory cgroup, congestion_wait will be called unconditionally, there
> is 0.5 seconds delay

OK, so the problem is that force_empty hits congestion_wait too much?
Why do we have no progress from try_to_free_mem_cgroup_pages?
 
> If use this patch, nearly no delay.
> 
> 
> > Why do wee another throttling when we do already throttle in the reclaim
> > path?
> 
> Do you mean we should remove congestion_wait(BLK_RW_ASYNC, HZ/10)
> from mem_cgroup_force_empty, since try_to_free_mem_cgroup_pages
> [shrink_inactive_list] has called congestion_wait

If it turns unnecessary, which is quite possible then yes. As I've said
we already throttle when seeing pages under writeback. If that is not
sufficient then we should investigate why.

Please also note that force_empty is considered deprecated. Do you have
any usecase which led you to fixing it?
-- 
Michal Hocko
SUSE Labs

[PATCH] mm: avoid the unnecessary waiting when force empty a cgroup

[Li RongQing]

The number of writeback and dirty page can be read out from memcg,
the unnecessary waiting can be avoided by these counts

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 mm/memcontrol.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9ec024b862ac..5258651bd4ec 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2613,9 +2613,13 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
 		progress = try_to_free_mem_cgroup_pages(memcg, 1,
 							GFP_KERNEL, true);
 		if (!progress) {
+			unsigned long num;
+
+			num = memcg_page_state(memcg, NR_WRITEBACK) +
+					memcg_page_state(memcg, NR_FILE_DIRTY);
 			nr_retries--;
-			/* maybe some writeback is necessary */
-			congestion_wait(BLK_RW_ASYNC, HZ/10);
+			if (num)
+				congestion_wait(BLK_RW_ASYNC, HZ/10);
 		}
 
 	}
-- 
2.11.0

Re: [PATCH] mm: avoid the unnecessary waiting when force empty a cgroup

[Michal Hocko]

On Tue 03-04-18 15:12:09, Li RongQing wrote:
> The number of writeback and dirty page can be read out from memcg,
> the unnecessary waiting can be avoided by these counts

This changelog doesn't explain the problem and how the patch fixes it.
Why do wee another throttling when we do already throttle in the reclaim
path?

> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  mm/memcontrol.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 9ec024b862ac..5258651bd4ec 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2613,9 +2613,13 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg)
>  		progress = try_to_free_mem_cgroup_pages(memcg, 1,
>  							GFP_KERNEL, true);
>  		if (!progress) {
> +			unsigned long num;
> +
> +			num = memcg_page_state(memcg, NR_WRITEBACK) +
> +					memcg_page_state(memcg, NR_FILE_DIRTY);
>  			nr_retries--;
> -			/* maybe some writeback is necessary */
> -			congestion_wait(BLK_RW_ASYNC, HZ/10);
> +			if (num)
> +				congestion_wait(BLK_RW_ASYNC, HZ/10);
>  		}
>  
>  	}
> -- 
> 2.11.0

-- 
Michal Hocko
SUSE Labs