Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] mm/memcg: remove useless check on page->mem_cgroup
@ 2020-07-31  3:34 Alex Shi
  2020-07-31 15:16 ` Johannes Weiner
  0 siblings, 1 reply; 14+ messages in thread
From: Alex Shi @ 2020-07-31  3:34 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Michal Hocko, Vladimir Davydov, Andrew Morton, cgroups, linux-mm,
	linux-kernel

Since readahead page will be charged on memcg too. We don't need to
check this exception now. Rmove them is safe as all user pages are
charged before use.

Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: cgroups@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/memcontrol.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e84c2b5596f2..9e44ae22d591 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1322,12 +1322,7 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgd
 	}
 
 	memcg = page->mem_cgroup;
-	/*
-	 * Swapcache readahead pages are added to the LRU - and
-	 * possibly migrated - before they are charged.
-	 */
-	if (!memcg)
-		memcg = root_mem_cgroup;
+	VM_BUG_ON_PAGE(!memcg, page);
 
 	mz = mem_cgroup_page_nodeinfo(memcg, page);
 	lruvec = &mz->lruvec;
@@ -6897,10 +6892,8 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
 	if (newpage->mem_cgroup)
 		return;
 
-	/* Swapcache readahead pages can get replaced before being charged */
 	memcg = oldpage->mem_cgroup;
-	if (!memcg)
-		return;
+	VM_BUG_ON_PAGE(!memcg, oldpage);
 
 	/* Force-charge the new page. The old one will be freed soon */
 	nr_pages = thp_nr_pages(newpage);
@@ -7094,10 +7087,7 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 		return;
 
 	memcg = page->mem_cgroup;
-
-	/* Readahead page, never charged */
-	if (!memcg)
-		return;
+	VM_BUG_ON_PAGE(!memcg, page);
 
 	/*
 	 * In case the memcg owning these pages has been offlined and doesn't
@@ -7158,10 +7148,7 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
 		return 0;
 
 	memcg = page->mem_cgroup;
-
-	/* Readahead page, never charged */
-	if (!memcg)
-		return 0;
+	VM_BUG_ON_PAGE(!memcg, page);
 
 	if (!entry.val) {
 		memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-07-31  3:34 [PATCH] mm/memcg: remove useless check on page->mem_cgroup Alex Shi
@ 2020-07-31 15:16 ` Johannes Weiner
  2020-08-01  3:58   ` Alex Shi
  0 siblings, 1 reply; 14+ messages in thread
From: Johannes Weiner @ 2020-07-31 15:16 UTC (permalink / raw)
  To: Alex Shi
  Cc: Michal Hocko, Vladimir Davydov, Andrew Morton, cgroups, linux-mm,
	linux-kernel

On Fri, Jul 31, 2020 at 11:34:40AM +0800, Alex Shi wrote:
> Since readahead page will be charged on memcg too. We don't need to
> check this exception now. Rmove them is safe as all user pages are
> charged before use.
> 
> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: cgroups@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/memcontrol.c | 21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e84c2b5596f2..9e44ae22d591 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1322,12 +1322,7 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgd
>  	}
>  
>  	memcg = page->mem_cgroup;
> -	/*
> -	 * Swapcache readahead pages are added to the LRU - and
> -	 * possibly migrated - before they are charged.
> -	 */
> -	if (!memcg)
> -		memcg = root_mem_cgroup;
> +	VM_BUG_ON_PAGE(!memcg, page);
>  
>  	mz = mem_cgroup_page_nodeinfo(memcg, page);
>  	lruvec = &mz->lruvec;
> @@ -6897,10 +6892,8 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
>  	if (newpage->mem_cgroup)
>  		return;
>  
> -	/* Swapcache readahead pages can get replaced before being charged */
>  	memcg = oldpage->mem_cgroup;
> -	if (!memcg)
> -		return;
> +	VM_BUG_ON_PAGE(!memcg, oldpage);
>  
>  	/* Force-charge the new page. The old one will be freed soon */
>  	nr_pages = thp_nr_pages(newpage);
> @@ -7094,10 +7087,7 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  		return;
>  
>  	memcg = page->mem_cgroup;
> -
> -	/* Readahead page, never charged */
> -	if (!memcg)
> -		return;
> +	VM_BUG_ON_PAGE(!memcg, page);
>  
>  	/*
>  	 * In case the memcg owning these pages has been offlined and doesn't
> @@ -7158,10 +7148,7 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
>  		return 0;
>  
>  	memcg = page->mem_cgroup;
> -
> -	/* Readahead page, never charged */
> -	if (!memcg)
> -		return 0;
> +	VM_BUG_ON_PAGE(!memcg, page);
>  
>  	if (!entry.val) {
>  		memcg_memory_event(memcg, MEMCG_SWAP_FAIL);

Uncharged readahead pages are gone, but I'm not 100% sure uncharged
pages in general are gone. ISTR that the !page->mem_cgroup check in
mem_cgroup_uncharge() prevented a crash - although that is of course a
much broader interface, whereas the ones you change should only apply
to LRU pages (which are hopefully all charged).

Nevertheless, to avoid unnecessary crashes if we discover that we've
been wrong, how about leaving the branches for now, but adding a (new)
VM_WARN_ON_ONCE_PAGE() to them?


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-07-31 15:16 ` Johannes Weiner
@ 2020-08-01  3:58   ` Alex Shi
  2020-08-03  8:18     ` Michal Hocko
  0 siblings, 1 reply; 14+ messages in thread
From: Alex Shi @ 2020-08-01  3:58 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Michal Hocko, Vladimir Davydov, Andrew Morton, cgroups, linux-mm,
	linux-kernel



在 2020/7/31 下午11:16, Johannes Weiner 写道:
>>  	if (!entry.val) {
>>  		memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
> Uncharged readahead pages are gone, but I'm not 100% sure uncharged
> pages in general are gone. ISTR that the !page->mem_cgroup check in
> mem_cgroup_uncharge() prevented a crash - although that is of course a
> much broader interface, whereas the ones you change should only apply
> to LRU pages (which are hopefully all charged).
> 
> Nevertheless, to avoid unnecessary crashes if we discover that we've
> been wrong, how about leaving the branches for now, but adding a (new)
> VM_WARN_ON_ONCE_PAGE() to them?


Right, let's see if other unexcepted things happens, and then do actions.
So it's the patch:

From 28893cf8e55b98665cce58c0ba6d54aeafb63a62 Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@linux.alibaba.com>
Date: Sat, 1 Aug 2020 10:43:55 +0800
Subject: [PATCH] mm/memcg: warning on !memcg after readahead page charged

Since readahead page is charged on memcg too, in theory we don't have to
check this exception now. Before safely remove them all, add a warning
for the unexpected !memcg.

Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: cgroups@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/mmdebug.h |  8 ++++++++
 mm/memcontrol.c         | 15 ++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 2ad72d2c8cc5..639e98a3384e 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -37,6 +37,13 @@
 			BUG();						\
 		}							\
 	} while (0)
+#define VM_WARN_ON_ONCE_PAGE(cond, page)				\
+	do {								\
+		if (unlikely(cond)) {					\
+			dump_page(page, "VM_WARN_ON_ONCE_PAGE(" __stringify(cond)")");\
+			WARN_ON_ONCE(cond);				\
+		}							\
+	} while (0)
 #define VM_WARN_ON(cond) (void)WARN_ON(cond)
 #define VM_WARN_ON_ONCE(cond) (void)WARN_ON_ONCE(cond)
 #define VM_WARN_ONCE(cond, format...) (void)WARN_ONCE(cond, format)
@@ -48,6 +55,7 @@
 #define VM_BUG_ON_MM(cond, mm) VM_BUG_ON(cond)
 #define VM_WARN_ON(cond) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
+#define VM_WARN_ON_ONCE_PAGE(cond, page)  BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN(cond, format...) BUILD_BUG_ON_INVALID(cond)
 #endif
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e84c2b5596f2..0174c31f6491 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1322,10 +1322,8 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgd
 	}
 
 	memcg = page->mem_cgroup;
-	/*
-	 * Swapcache readahead pages are added to the LRU - and
-	 * possibly migrated - before they are charged.
-	 */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		memcg = root_mem_cgroup;
 
@@ -6897,8 +6895,9 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
 	if (newpage->mem_cgroup)
 		return;
 
-	/* Swapcache readahead pages can get replaced before being charged */
 	memcg = oldpage->mem_cgroup;
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, oldpage);
 	if (!memcg)
 		return;
 
@@ -7095,7 +7094,8 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 
 	memcg = page->mem_cgroup;
 
-	/* Readahead page, never charged */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		return;
 
@@ -7159,7 +7159,8 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
 
 	memcg = page->mem_cgroup;
 
-	/* Readahead page, never charged */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		return 0;
 
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-01  3:58   ` Alex Shi
@ 2020-08-03  8:18     ` Michal Hocko
  2020-08-04  7:35       ` Alex Shi
  0 siblings, 1 reply; 14+ messages in thread
From: Michal Hocko @ 2020-08-03  8:18 UTC (permalink / raw)
  To: Alex Shi
  Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
	linux-mm, linux-kernel

On Sat 01-08-20 11:58:41, Alex Shi wrote:
> 
> 
> 在 2020/7/31 下午11:16, Johannes Weiner 写道:
> >>  	if (!entry.val) {
> >>  		memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
> > Uncharged readahead pages are gone, but I'm not 100% sure uncharged
> > pages in general are gone. ISTR that the !page->mem_cgroup check in
> > mem_cgroup_uncharge() prevented a crash - although that is of course a
> > much broader interface, whereas the ones you change should only apply
> > to LRU pages (which are hopefully all charged).
> > 
> > Nevertheless, to avoid unnecessary crashes if we discover that we've
> > been wrong, how about leaving the branches for now, but adding a (new)
> > VM_WARN_ON_ONCE_PAGE() to them?

Agreed!

> Right, let's see if other unexcepted things happens, and then do actions.
> So it's the patch:
> 
> >From 28893cf8e55b98665cce58c0ba6d54aeafb63a62 Mon Sep 17 00:00:00 2001
> From: Alex Shi <alex.shi@linux.alibaba.com>
> Date: Sat, 1 Aug 2020 10:43:55 +0800
> Subject: [PATCH] mm/memcg: warning on !memcg after readahead page charged
> 
> Since readahead page is charged on memcg too, in theory we don't have to
> check this exception now. Before safely remove them all, add a warning
> for the unexpected !memcg.

I would find it useful to mention since when this assumption holds.

> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: cgroups@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  include/linux/mmdebug.h |  8 ++++++++
>  mm/memcontrol.c         | 15 ++++++++-------
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index 2ad72d2c8cc5..639e98a3384e 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -37,6 +37,13 @@
>  			BUG();						\
>  		}							\
>  	} while (0)
> +#define VM_WARN_ON_ONCE_PAGE(cond, page)				\
> +	do {								\
> +		if (unlikely(cond)) {					\
> +			dump_page(page, "VM_WARN_ON_ONCE_PAGE(" __stringify(cond)")");\
> +			WARN_ON_ONCE(cond);				\
> +		}							\

This is a bit strange behavior. You dump page for each occasion but warn
only once. I would expect either "once" semantic for any output or just
dump on each occasion because if the whole point is to reduce to amount
of output then the above doesn't serve the purpose.

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-03  8:18     ` Michal Hocko
@ 2020-08-04  7:35       ` Alex Shi
  2020-08-05 12:28         ` Alex Shi
  0 siblings, 1 reply; 14+ messages in thread
From: Alex Shi @ 2020-08-04  7:35 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
	linux-mm, linux-kernel



在 2020/8/3 下午4:18, Michal Hocko 写道:
> On Sat 01-08-20 11:58:41, Alex Shi wrote:
>>
>>
>> 在 2020/7/31 下午11:16, Johannes Weiner 写道:
>>>>  	if (!entry.val) {
>>>>  		memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
>>> Uncharged readahead pages are gone, but I'm not 100% sure uncharged
>>> pages in general are gone. ISTR that the !page->mem_cgroup check in
>>> mem_cgroup_uncharge() prevented a crash - although that is of course a
>>> much broader interface, whereas the ones you change should only apply
>>> to LRU pages (which are hopefully all charged).
>>>
>>> Nevertheless, to avoid unnecessary crashes if we discover that we've
>>> been wrong, how about leaving the branches for now, but adding a (new)
>>> VM_WARN_ON_ONCE_PAGE() to them?
> 
> Agreed!
> 
>> Right, let's see if other unexcepted things happens, and then do actions.
>> So it's the patch:
>>
>> >From 28893cf8e55b98665cce58c0ba6d54aeafb63a62 Mon Sep 17 00:00:00 2001
>> From: Alex Shi <alex.shi@linux.alibaba.com>
>> Date: Sat, 1 Aug 2020 10:43:55 +0800
>> Subject: [PATCH] mm/memcg: warning on !memcg after readahead page charged
>>
>> Since readahead page is charged on memcg too, in theory we don't have to
>> check this exception now. Before safely remove them all, add a warning
>> for the unexpected !memcg.
> 
> I would find it useful to mention since when this assumption holds.> 
>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: cgroups@vger.kernel.org
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  include/linux/mmdebug.h |  8 ++++++++
>>  mm/memcontrol.c         | 15 ++++++++-------
>>  2 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
>> index 2ad72d2c8cc5..639e98a3384e 100644
>> --- a/include/linux/mmdebug.h
>> +++ b/include/linux/mmdebug.h
>> @@ -37,6 +37,13 @@
>>  			BUG();						\
>>  		}							\
>>  	} while (0)
>> +#define VM_WARN_ON_ONCE_PAGE(cond, page)				\
>> +	do {								\
>> +		if (unlikely(cond)) {					\
>> +			dump_page(page, "VM_WARN_ON_ONCE_PAGE(" __stringify(cond)")");\
>> +			WARN_ON_ONCE(cond);				\
>> +		}							\
> 
> This is a bit strange behavior. You dump page for each occasion but warn
> only once. I would expect either "once" semantic for any output or just
> dump on each occasion because if the whole point is to reduce to amount
> of output then the above doesn't serve the purpose.
> 

Yes, left more dump_page may ommited by users. for reduce dmesg purpose, warn once
is better.

Thanks for comment!
Alex 
--
From 3cee031d50625733a64b58240d0e6f8151e5299c Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@linux.alibaba.com>
Date: Sat, 1 Aug 2020 10:43:55 +0800
Subject: [PATCH v2] mm/memcg: warning on !memcg after readahead page charged

Since readahead page is charged on memcg too, in theory we don't have to
check this exception now. Before safely remove them all, add a warning
for the unexpected !memcg.

Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: cgroups@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/mmdebug.h | 13 +++++++++++++
 mm/memcontrol.c         | 15 ++++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 2ad72d2c8cc5..698eaf56f89f 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -37,6 +37,18 @@
 			BUG();						\
 		}							\
 	} while (0)
+#define VM_WARN_ON_ONCE_PAGE(condition, page)	({			\
+	static bool __section(.data.once) __warned;			\
+	int __ret_warn_once = !!(condition);				\
+									\
+	if (unlikely(__ret_warn_once && !__warned)) {			\
+		dump_page(page, "VM_WARN_ON_ONCE_PAGE(" __stringify(cond)")");\
+		__warned = true;					\
+		WARN_ON(1);						\
+	}								\
+	unlikely(__ret_warn_once);					\
+})
+
 #define VM_WARN_ON(cond) (void)WARN_ON(cond)
 #define VM_WARN_ON_ONCE(cond) (void)WARN_ON_ONCE(cond)
 #define VM_WARN_ONCE(cond, format...) (void)WARN_ONCE(cond, format)
@@ -48,6 +60,7 @@
 #define VM_BUG_ON_MM(cond, mm) VM_BUG_ON(cond)
 #define VM_WARN_ON(cond) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
+#define VM_WARN_ON_ONCE_PAGE(cond, page)  BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN(cond, format...) BUILD_BUG_ON_INVALID(cond)
 #endif
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 130093bdf74b..299382fc55a9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1322,10 +1322,8 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgd
 	}
 
 	memcg = page->mem_cgroup;
-	/*
-	 * Swapcache readahead pages are added to the LRU - and
-	 * possibly migrated - before they are charged.
-	 */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		memcg = root_mem_cgroup;
 
@@ -6906,8 +6904,9 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
 	if (newpage->mem_cgroup)
 		return;
 
-	/* Swapcache readahead pages can get replaced before being charged */
 	memcg = oldpage->mem_cgroup;
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, oldpage);
 	if (!memcg)
 		return;
 
@@ -7104,7 +7103,8 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 
 	memcg = page->mem_cgroup;
 
-	/* Readahead page, never charged */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		return;
 
@@ -7168,7 +7168,8 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
 
 	memcg = page->mem_cgroup;
 
-	/* Readahead page, never charged */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		return 0;
 
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-04  7:35       ` Alex Shi
@ 2020-08-05 12:28         ` Alex Shi
  2020-08-05 13:02           ` Alex Shi
  2020-08-10  9:55           ` Michal Hocko
  0 siblings, 2 replies; 14+ messages in thread
From: Alex Shi @ 2020-08-05 12:28 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
	linux-mm, linux-kernel

The last patch has a problem on define. this version could fix it.

BTW, I see some !memcg happens when MEMCG compilered but disabled by cgroup_disable


[   94.657666] ---[ end trace f1f34bfc3b32ed2f ]---
[   95.138995] anon flags: 0x5005b48008000d(locked|uptodate|dirty|swapbacked)
[   95.146220] raw: 005005b48008000d dead000000000100 dead000000000122 ffff8897c7c76ad1
[   95.154549] raw: 0000000000000022 0000000000000000 0000000200000000 0000000000000000
[   95.162876] page dumped because: VM_WARN_ON_ONCE_PAGE(!memcg)



From 2ca3e87fd3878ab729551682ad083a70f15bb3fc Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@linux.alibaba.com>
Date: Sat, 1 Aug 2020 10:43:55 +0800
Subject: [PATCH v3] mm/memcg: warning on !memcg after readahead page charged

Since readahead page is charged on memcg too, in theory we don't have to
check this exception now. Before safely remove them all, add a warning
for the unexpected !memcg.

Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: cgroups@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/mmdebug.h | 13 +++++++++++++
 mm/memcontrol.c         | 15 ++++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 2ad72d2c8cc5..4ed52879ce55 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -37,6 +37,18 @@
 			BUG();						\
 		}							\
 	} while (0)
+#define VM_WARN_ON_ONCE_PAGE(cond, page)	({			\
+	static bool __section(.data.once) __warned;			\
+	int __ret_warn_once = !!(cond);					\
+									\
+	if (unlikely(__ret_warn_once && !__warned)) {			\
+		dump_page(page, "VM_WARN_ON_ONCE_PAGE(" __stringify(cond)")");\
+		__warned = true;					\
+		WARN_ON(1);						\
+	}								\
+	unlikely(__ret_warn_once);					\
+})
+
 #define VM_WARN_ON(cond) (void)WARN_ON(cond)
 #define VM_WARN_ON_ONCE(cond) (void)WARN_ON_ONCE(cond)
 #define VM_WARN_ONCE(cond, format...) (void)WARN_ONCE(cond, format)
@@ -48,6 +60,7 @@
 #define VM_BUG_ON_MM(cond, mm) VM_BUG_ON(cond)
 #define VM_WARN_ON(cond) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
+#define VM_WARN_ON_ONCE_PAGE(cond, page)  BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN(cond, format...) BUILD_BUG_ON_INVALID(cond)
 #endif
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 130093bdf74b..299382fc55a9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1322,10 +1322,8 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgd
 	}
 
 	memcg = page->mem_cgroup;
-	/*
-	 * Swapcache readahead pages are added to the LRU - and
-	 * possibly migrated - before they are charged.
-	 */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		memcg = root_mem_cgroup;
 
@@ -6906,8 +6904,9 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
 	if (newpage->mem_cgroup)
 		return;
 
-	/* Swapcache readahead pages can get replaced before being charged */
 	memcg = oldpage->mem_cgroup;
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, oldpage);
 	if (!memcg)
 		return;
 
@@ -7104,7 +7103,8 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 
 	memcg = page->mem_cgroup;
 
-	/* Readahead page, never charged */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		return;
 
@@ -7168,7 +7168,8 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
 
 	memcg = page->mem_cgroup;
 
-	/* Readahead page, never charged */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		return 0;
 
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-05 12:28         ` Alex Shi
@ 2020-08-05 13:02           ` Alex Shi
  2020-08-08 13:22             ` Alex Shi
  2020-08-10  9:52             ` Michal Hocko
  2020-08-10  9:55           ` Michal Hocko
  1 sibling, 2 replies; 14+ messages in thread
From: Alex Shi @ 2020-08-05 13:02 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
	linux-mm, linux-kernel



在 2020/8/5 下午8:28, Alex Shi 写道:
> The last patch has a problem on define. this version could fix it.
> 
> BTW, I see some !memcg happens when MEMCG compilered but disabled by cgroup_disable
> 
> 
> [   94.657666] ---[ end trace f1f34bfc3b32ed2f ]---
> [   95.138995] anon flags: 0x5005b48008000d(locked|uptodate|dirty|swapbacked)
> [   95.146220] raw: 005005b48008000d dead000000000100 dead000000000122 ffff8897c7c76ad1
> [   95.154549] raw: 0000000000000022 0000000000000000 0000000200000000 0000000000000000
> [   95.162876] page dumped because: VM_WARN_ON_ONCE_PAGE(!memcg)
> 
> 

The following patch may helpful.

From 8bfb26a2e37e08dc61d20212bcfa5812a367ba94 Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@linux.alibaba.com>
Date: Wed, 5 Aug 2020 20:32:12 +0800
Subject: [PATCH] mm/memcg: don't try charge swap if memcg disabled

If we disabled memcg by cgroup_disable=memory, the swap charges are
still called. Let's return from the funcs earlier and keep WARN_ON
monitor.

Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: cgroups@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/memcontrol.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index cb07a48d53aa..65f2b42d25af 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -7163,6 +7163,9 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 	VM_BUG_ON_PAGE(PageLRU(page), page);
 	VM_BUG_ON_PAGE(page_count(page), page);
 
+	if (mem_cgroup_disabled())
+		return;
+
 	if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
 		return;
 
@@ -7228,6 +7231,9 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
 	struct mem_cgroup *memcg;
 	unsigned short oldid;
 
+	if (mem_cgroup_disabled())
+		return 0;
+
 	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
 		return 0;
 
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-05 13:02           ` Alex Shi
@ 2020-08-08 13:22             ` Alex Shi
  2020-08-08 21:43               ` Roman Gushchin
  2020-08-10  9:52             ` Michal Hocko
  1 sibling, 1 reply; 14+ messages in thread
From: Alex Shi @ 2020-08-08 13:22 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
	linux-mm, linux-kernel



在 2020/8/5 下午9:02, Alex Shi 写道:
> 
> 
> 在 2020/8/5 下午8:28, Alex Shi 写道:
>> The last patch has a problem on define. this version could fix it.
>>
>> BTW, I see some !memcg happens when MEMCG compilered but disabled by cgroup_disable
>>
>>
>> [   94.657666] ---[ end trace f1f34bfc3b32ed2f ]---
>> [   95.138995] anon flags: 0x5005b48008000d(locked|uptodate|dirty|swapbacked)
>> [   95.146220] raw: 005005b48008000d dead000000000100 dead000000000122 ffff8897c7c76ad1
>> [   95.154549] raw: 0000000000000022 0000000000000000 0000000200000000 0000000000000000
>> [   95.162876] page dumped because: VM_WARN_ON_ONCE_PAGE(!memcg)
>>
>>
> 
> The following patch may helpful.

Any comments for the 2 patches?

Thanks
Alex

> 
> From 8bfb26a2e37e08dc61d20212bcfa5812a367ba94 Mon Sep 17 00:00:00 2001
> From: Alex Shi <alex.shi@linux.alibaba.com>
> Date: Wed, 5 Aug 2020 20:32:12 +0800
> Subject: [PATCH] mm/memcg: don't try charge swap if memcg disabled
> 
> If we disabled memcg by cgroup_disable=memory, the swap charges are
> still called. Let's return from the funcs earlier and keep WARN_ON
> monitor.
> 
> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: cgroups@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/memcontrol.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index cb07a48d53aa..65f2b42d25af 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7163,6 +7163,9 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  	VM_BUG_ON_PAGE(PageLRU(page), page);
>  	VM_BUG_ON_PAGE(page_count(page), page);
>  
> +	if (mem_cgroup_disabled())
> +		return;
> +
>  	if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
>  		return;
>  
> @@ -7228,6 +7231,9 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
>  	struct mem_cgroup *memcg;
>  	unsigned short oldid;
>  
> +	if (mem_cgroup_disabled())
> +		return 0;
> +
>  	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
>  		return 0;
>  
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-08 13:22             ` Alex Shi
@ 2020-08-08 21:43               ` Roman Gushchin
  2020-08-10  7:44                 ` Alex Shi
  0 siblings, 1 reply; 14+ messages in thread
From: Roman Gushchin @ 2020-08-08 21:43 UTC (permalink / raw)
  To: Alex Shi
  Cc: Michal Hocko, Johannes Weiner, Vladimir Davydov, Andrew Morton,
	cgroups, linux-mm, linux-kernel

On Sat, Aug 08, 2020 at 09:22:29PM +0800, Alex Shi wrote:
> 
> 
> 在 2020/8/5 下午9:02, Alex Shi 写道:
> > 
> > 
> > 在 2020/8/5 下午8:28, Alex Shi 写道:
> >> The last patch has a problem on define. this version could fix it.
> >>
> >> BTW, I see some !memcg happens when MEMCG compilered but disabled by cgroup_disable
> >>
> >>
> >> [   94.657666] ---[ end trace f1f34bfc3b32ed2f ]---
> >> [   95.138995] anon flags: 0x5005b48008000d(locked|uptodate|dirty|swapbacked)
> >> [   95.146220] raw: 005005b48008000d dead000000000100 dead000000000122 ffff8897c7c76ad1
> >> [   95.154549] raw: 0000000000000022 0000000000000000 0000000200000000 0000000000000000
> >> [   95.162876] page dumped because: VM_WARN_ON_ONCE_PAGE(!memcg)
> >>
> >>
> > 
> > The following patch may helpful.
> 
> Any comments for the 2 patches?
> 
> Thanks
> Alex
> 
> > 
> > From 8bfb26a2e37e08dc61d20212bcfa5812a367ba94 Mon Sep 17 00:00:00 2001
> > From: Alex Shi <alex.shi@linux.alibaba.com>
> > Date: Wed, 5 Aug 2020 20:32:12 +0800
> > Subject: [PATCH] mm/memcg: don't try charge swap if memcg disabled
> > 
> > If we disabled memcg by cgroup_disable=memory, the swap charges are
> > still called. Let's return from the funcs earlier and keep WARN_ON
> > monitor.
> > 
> > Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Michal Hocko <mhocko@kernel.org>
> > Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: cgroups@vger.kernel.org
> > Cc: linux-mm@kvack.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  mm/memcontrol.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index cb07a48d53aa..65f2b42d25af 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -7163,6 +7163,9 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
> >  	VM_BUG_ON_PAGE(PageLRU(page), page);
> >  	VM_BUG_ON_PAGE(page_count(page), page);
> >  
> > +	if (mem_cgroup_disabled())
> > +		return;
> > +
> >  	if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
> >  		return;
> >  
> > @@ -7228,6 +7231,9 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
> >  	struct mem_cgroup *memcg;
> >  	unsigned short oldid;
> >  
> > +	if (mem_cgroup_disabled())
> > +		return 0;
> > +
> >  	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
> >  		return 0;
> >  
> > 


Hi Alex,

this patch looks good to me. Please, feel free to add
Reviewed-by: Roman Gushchin <guro@fb.com>

What's the second patch?

Thanks!



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-08 21:43               ` Roman Gushchin
@ 2020-08-10  7:44                 ` Alex Shi
  0 siblings, 0 replies; 14+ messages in thread
From: Alex Shi @ 2020-08-10  7:44 UTC (permalink / raw)
  To: Roman Gushchin
  Cc: Michal Hocko, Johannes Weiner, Vladimir Davydov, Andrew Morton,
	cgroups, linux-mm, linux-kernel



在 2020/8/9 上午5:43, Roman Gushchin 写道:
>>>  mm/memcontrol.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> index cb07a48d53aa..65f2b42d25af 100644
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -7163,6 +7163,9 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>>>  	VM_BUG_ON_PAGE(PageLRU(page), page);
>>>  	VM_BUG_ON_PAGE(page_count(page), page);
>>>  
>>> +	if (mem_cgroup_disabled())
>>> +		return;
>>> +
>>>  	if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
>>>  		return;
>>>  
>>> @@ -7228,6 +7231,9 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
>>>  	struct mem_cgroup *memcg;
>>>  	unsigned short oldid;
>>>  
>>> +	if (mem_cgroup_disabled())
>>> +		return 0;
>>> +
>>>  	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
>>>  		return 0;
>>>  
>>>
> 
> Hi Alex,
> 
> this patch looks good to me. Please, feel free to add
> Reviewed-by: Roman Gushchin <guro@fb.com>

Thanks a lot!
> 
> What's the second patch?
> 

It's the patch, 
https://www.spinics.net/lists/linux-mm/msg222228.html
or

From 2ca3e87fd3878ab729551682ad083a70f15bb3fc Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@linux.alibaba.com>
Date: Sat, 1 Aug 2020 10:43:55 +0800
Subject: [PATCH v3] mm/memcg: warning on !memcg after readahead page charged

Since readahead page is charged on memcg too, in theory we don't have to
check this exception now. Before safely remove them all, add a warning
for the unexpected !memcg.

Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: cgroups@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/mmdebug.h | 13 +++++++++++++
 mm/memcontrol.c         | 15 ++++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 2ad72d2c8cc5..4ed52879ce55 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -37,6 +37,18 @@
 			BUG();						\
 		}							\
 	} while (0)
+#define VM_WARN_ON_ONCE_PAGE(cond, page)	({			\
+	static bool __section(.data.once) __warned;			\
+	int __ret_warn_once = !!(cond);					\
+									\
+	if (unlikely(__ret_warn_once && !__warned)) {			\
+		dump_page(page, "VM_WARN_ON_ONCE_PAGE(" __stringify(cond)")");\
+		__warned = true;					\
+		WARN_ON(1);						\
+	}								\
+	unlikely(__ret_warn_once);					\
+})
+
 #define VM_WARN_ON(cond) (void)WARN_ON(cond)
 #define VM_WARN_ON_ONCE(cond) (void)WARN_ON_ONCE(cond)
 #define VM_WARN_ONCE(cond, format...) (void)WARN_ONCE(cond, format)
@@ -48,6 +60,7 @@
 #define VM_BUG_ON_MM(cond, mm) VM_BUG_ON(cond)
 #define VM_WARN_ON(cond) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
+#define VM_WARN_ON_ONCE_PAGE(cond, page)  BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN(cond, format...) BUILD_BUG_ON_INVALID(cond)
 #endif
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 130093bdf74b..299382fc55a9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1322,10 +1322,8 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgd
 	}
 
 	memcg = page->mem_cgroup;
-	/*
-	 * Swapcache readahead pages are added to the LRU - and
-	 * possibly migrated - before they are charged.
-	 */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		memcg = root_mem_cgroup;
 
@@ -6906,8 +6904,9 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
 	if (newpage->mem_cgroup)
 		return;
 
-	/* Swapcache readahead pages can get replaced before being charged */
 	memcg = oldpage->mem_cgroup;
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, oldpage);
 	if (!memcg)
 		return;
 
@@ -7104,7 +7103,8 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 
 	memcg = page->mem_cgroup;
 
-	/* Readahead page, never charged */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		return;
 
@@ -7168,7 +7168,8 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
 
 	memcg = page->mem_cgroup;
 
-	/* Readahead page, never charged */
+	/* Readahead page is charged too, to see if other page uncharged */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		return 0;
 
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-05 13:02           ` Alex Shi
  2020-08-08 13:22             ` Alex Shi
@ 2020-08-10  9:52             ` Michal Hocko
  2020-08-10  9:53               ` Michal Hocko
  1 sibling, 1 reply; 14+ messages in thread
From: Michal Hocko @ 2020-08-10  9:52 UTC (permalink / raw)
  To: Alex Shi
  Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
	linux-mm, linux-kernel

On Wed 05-08-20 21:02:30, Alex Shi wrote:
> 
> 
> 在 2020/8/5 下午8:28, Alex Shi 写道:
> > The last patch has a problem on define. this version could fix it.
> > 
> > BTW, I see some !memcg happens when MEMCG compilered but disabled by cgroup_disable
> > 
> > 
> > [   94.657666] ---[ end trace f1f34bfc3b32ed2f ]---
> > [   95.138995] anon flags: 0x5005b48008000d(locked|uptodate|dirty|swapbacked)
> > [   95.146220] raw: 005005b48008000d dead000000000100 dead000000000122 ffff8897c7c76ad1
> > [   95.154549] raw: 0000000000000022 0000000000000000 0000000200000000 0000000000000000
> > [   95.162876] page dumped because: VM_WARN_ON_ONCE_PAGE(!memcg)
> > 
> > 
> 
> The following patch may helpful.
> 
> >From 8bfb26a2e37e08dc61d20212bcfa5812a367ba94 Mon Sep 17 00:00:00 2001
> From: Alex Shi <alex.shi@linux.alibaba.com>
> Date: Wed, 5 Aug 2020 20:32:12 +0800
> Subject: [PATCH] mm/memcg: don't try charge swap if memcg disabled
> 
> If we disabled memcg by cgroup_disable=memory, the swap charges are
> still called. Let's return from the funcs earlier and keep WARN_ON
> monitor.

Do I get it right that this is on top of your patch to remove the memcg
check or a preparatory work? Both are good but it would be better to
call that out specifically for clarity (along with the warning if that
is a follow up fix).

> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: cgroups@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/memcontrol.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index cb07a48d53aa..65f2b42d25af 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7163,6 +7163,9 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  	VM_BUG_ON_PAGE(PageLRU(page), page);
>  	VM_BUG_ON_PAGE(page_count(page), page);
>  
> +	if (mem_cgroup_disabled())
> +		return;
> +
>  	if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
>  		return;
>  
> @@ -7228,6 +7231,9 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
>  	struct mem_cgroup *memcg;
>  	unsigned short oldid;
>  
> +	if (mem_cgroup_disabled())
> +		return 0;
> +
>  	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
>  		return 0;
>  
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-10  9:52             ` Michal Hocko
@ 2020-08-10  9:53               ` Michal Hocko
  0 siblings, 0 replies; 14+ messages in thread
From: Michal Hocko @ 2020-08-10  9:53 UTC (permalink / raw)
  To: Alex Shi
  Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
	linux-mm, linux-kernel

On Mon 10-08-20 11:52:02, Michal Hocko wrote:
> On Wed 05-08-20 21:02:30, Alex Shi wrote:
> > 
> > 
> > 在 2020/8/5 下午8:28, Alex Shi 写道:
> > > The last patch has a problem on define. this version could fix it.
> > > 
> > > BTW, I see some !memcg happens when MEMCG compilered but disabled by cgroup_disable
> > > 
> > > 
> > > [   94.657666] ---[ end trace f1f34bfc3b32ed2f ]---
> > > [   95.138995] anon flags: 0x5005b48008000d(locked|uptodate|dirty|swapbacked)
> > > [   95.146220] raw: 005005b48008000d dead000000000100 dead000000000122 ffff8897c7c76ad1
> > > [   95.154549] raw: 0000000000000022 0000000000000000 0000000200000000 0000000000000000
> > > [   95.162876] page dumped because: VM_WARN_ON_ONCE_PAGE(!memcg)
> > > 
> > > 
> > 
> > The following patch may helpful.
> > 
> > >From 8bfb26a2e37e08dc61d20212bcfa5812a367ba94 Mon Sep 17 00:00:00 2001
> > From: Alex Shi <alex.shi@linux.alibaba.com>
> > Date: Wed, 5 Aug 2020 20:32:12 +0800
> > Subject: [PATCH] mm/memcg: don't try charge swap if memcg disabled
> > 
> > If we disabled memcg by cgroup_disable=memory, the swap charges are
> > still called. Let's return from the funcs earlier and keep WARN_ON
> > monitor.
> 
> Do I get it right that this is on top of your patch to remove the memcg
> check or a preparatory work?

Sorry meant to say - add the warning rather than drop the check.

> Both are good but it would be better to
> call that out specifically for clarity (along with the warning if that
> is a follow up fix).
-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-05 12:28         ` Alex Shi
  2020-08-05 13:02           ` Alex Shi
@ 2020-08-10  9:55           ` Michal Hocko
  2020-08-10 12:29             ` Alex Shi
  1 sibling, 1 reply; 14+ messages in thread
From: Michal Hocko @ 2020-08-10  9:55 UTC (permalink / raw)
  To: Alex Shi
  Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
	linux-mm, linux-kernel

On Wed 05-08-20 20:28:33, Alex Shi wrote:
[...]
> >From 2ca3e87fd3878ab729551682ad083a70f15bb3fc Mon Sep 17 00:00:00 2001
> From: Alex Shi <alex.shi@linux.alibaba.com>
> Date: Sat, 1 Aug 2020 10:43:55 +0800
> Subject: [PATCH v3] mm/memcg: warning on !memcg after readahead page charged
> 
> Since readahead page is charged on memcg too, in theory we don't have to
> check this exception now. Before safely remove them all, add a warning
> for the unexpected !memcg.
> 
> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: cgroups@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org

Looks good to me. I am not familiar with the section tweaks but that
should be ok.

Acked-by: Michal Hocko <mhocko@suse.com>

Once you collect more feedback, please send both patches so that they do
not get lost in this thread.

Thanks!

> ---
>  include/linux/mmdebug.h | 13 +++++++++++++
>  mm/memcontrol.c         | 15 ++++++++-------
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index 2ad72d2c8cc5..4ed52879ce55 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -37,6 +37,18 @@
>  			BUG();						\
>  		}							\
>  	} while (0)
> +#define VM_WARN_ON_ONCE_PAGE(cond, page)	({			\
> +	static bool __section(.data.once) __warned;			\
> +	int __ret_warn_once = !!(cond);					\
> +									\
> +	if (unlikely(__ret_warn_once && !__warned)) {			\
> +		dump_page(page, "VM_WARN_ON_ONCE_PAGE(" __stringify(cond)")");\
> +		__warned = true;					\
> +		WARN_ON(1);						\
> +	}								\
> +	unlikely(__ret_warn_once);					\
> +})
> +
>  #define VM_WARN_ON(cond) (void)WARN_ON(cond)
>  #define VM_WARN_ON_ONCE(cond) (void)WARN_ON_ONCE(cond)
>  #define VM_WARN_ONCE(cond, format...) (void)WARN_ONCE(cond, format)
> @@ -48,6 +60,7 @@
>  #define VM_BUG_ON_MM(cond, mm) VM_BUG_ON(cond)
>  #define VM_WARN_ON(cond) BUILD_BUG_ON_INVALID(cond)
>  #define VM_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
> +#define VM_WARN_ON_ONCE_PAGE(cond, page)  BUILD_BUG_ON_INVALID(cond)
>  #define VM_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
>  #define VM_WARN(cond, format...) BUILD_BUG_ON_INVALID(cond)
>  #endif
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 130093bdf74b..299382fc55a9 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1322,10 +1322,8 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgd
>  	}
>  
>  	memcg = page->mem_cgroup;
> -	/*
> -	 * Swapcache readahead pages are added to the LRU - and
> -	 * possibly migrated - before they are charged.
> -	 */
> +	/* Readahead page is charged too, to see if other page uncharged */
> +	VM_WARN_ON_ONCE_PAGE(!memcg, page);
>  	if (!memcg)
>  		memcg = root_mem_cgroup;
>  
> @@ -6906,8 +6904,9 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
>  	if (newpage->mem_cgroup)
>  		return;
>  
> -	/* Swapcache readahead pages can get replaced before being charged */
>  	memcg = oldpage->mem_cgroup;
> +	/* Readahead page is charged too, to see if other page uncharged */
> +	VM_WARN_ON_ONCE_PAGE(!memcg, oldpage);
>  	if (!memcg)
>  		return;
>  
> @@ -7104,7 +7103,8 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  
>  	memcg = page->mem_cgroup;
>  
> -	/* Readahead page, never charged */
> +	/* Readahead page is charged too, to see if other page uncharged */
> +	VM_WARN_ON_ONCE_PAGE(!memcg, page);
>  	if (!memcg)
>  		return;
>  
> @@ -7168,7 +7168,8 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry)
>  
>  	memcg = page->mem_cgroup;
>  
> -	/* Readahead page, never charged */
> +	/* Readahead page is charged too, to see if other page uncharged */
> +	VM_WARN_ON_ONCE_PAGE(!memcg, page);
>  	if (!memcg)
>  		return 0;
>  
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] mm/memcg: remove useless check on page->mem_cgroup
  2020-08-10  9:55           ` Michal Hocko
@ 2020-08-10 12:29             ` Alex Shi
  0 siblings, 0 replies; 14+ messages in thread
From: Alex Shi @ 2020-08-10 12:29 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
	linux-mm, linux-kernel



在 2020/8/10 下午5:55, Michal Hocko 写道:
>>
>> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: cgroups@vger.kernel.org
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
> Looks good to me. I am not familiar with the section tweaks but that
> should be ok.
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 

Thanks a lot, Michal!

> Once you collect more feedback, please send both patches so that they do
> not get lost in this thread.

I will resend with your ack.

Thanks!
Alex


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-31  3:34 [PATCH] mm/memcg: remove useless check on page->mem_cgroup Alex Shi
2020-07-31 15:16 ` Johannes Weiner
2020-08-01  3:58   ` Alex Shi
2020-08-03  8:18     ` Michal Hocko
2020-08-04  7:35       ` Alex Shi
2020-08-05 12:28         ` Alex Shi
2020-08-05 13:02           ` Alex Shi
2020-08-08 13:22             ` Alex Shi
2020-08-08 21:43               ` Roman Gushchin
2020-08-10  7:44                 ` Alex Shi
2020-08-10  9:52             ` Michal Hocko
2020-08-10  9:53               ` Michal Hocko
2020-08-10  9:55           ` Michal Hocko
2020-08-10 12:29             ` Alex Shi

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git