Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1595671 > unrolled thread

"mm: fix lazyfree BUG_ON check in try_to_unmap_one()" build error

Back to article view | Back to linux.kernel

Contents

  "mm: fix lazyfree BUG_ON check in try_to_unmap_one()" build error Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-09 05:30 +0100
    Re: "mm: fix lazyfree BUG_ON check in try_to_unmap_one()" build error Minchan Kim <minchan@kernel.org> - 2017-03-09 07:10 +0100
      Re: "mm: fix lazyfree BUG_ON check in try_to_unmap_one()" build error Michal Hocko <mhocko@kernel.org> - 2017-03-09 10:00 +0100
      Re: "mm: fix lazyfree BUG_ON check in try_to_unmap_one()" build  error Andrew Morton <akpm@linux-foundation.org> - 2017-03-09 22:30 +0100
        Re: "mm: fix lazyfree BUG_ON check in try_to_unmap_one()" build error Minchan Kim <minchan@kernel.org> - 2017-03-10 01:50 +0100
          Re: "mm: fix lazyfree BUG_ON check in try_to_unmap_one()" build error Vlastimil Babka <vbabka@suse.cz> - 2017-03-10 11:10 +0100

#1595671 — "mm: fix lazyfree BUG_ON check in try_to_unmap_one()" build error

Hello Minchan,

/* I can't https://marc.info/?l=linux-kernel&m=148886631303107 thread
   in my mail box for some reason so the Reply-To message-id may be wrong. */



commit "mm: fix lazyfree BUG_ON check in try_to_unmap_one()"
(mmotm fd07630cbf59bead90046dd3e5cfd891e58e6987)


	if (VM_WARN_ON_ONCE(PageSwapBacked(page) !=
			PageSwapCache(page))) {
	...
	}


does not compile on !CONFIG_DEBUG_VM configs, because VM_WARN_ONCE() is

	#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))



In file included from ./include/linux/mmdebug.h:4:0,
                 from ./include/linux/mm.h:8,
                 from mm/rmap.c:48:
mm/rmap.c: In function ‘try_to_unmap_one’:
./include/linux/bug.h:45:33: error: void value not ignored as it ought to be
 #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
                                 ^
./include/linux/mmdebug.h:49:31: note: in expansion of macro ‘BUILD_BUG_ON_INVALID’
 #define VM_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
                               ^~~~~~~~~~~~~~~~~~~~
mm/rmap.c:1416:8: note: in expansion of macro ‘VM_WARN_ON_ONCE’
    if (VM_WARN_ON_ONCE(PageSwapBacked(page) !=
        ^~~~~~~~~~~~~~~

	-ss

[toc] | [next] | [standalone]

#1595685

Hi Sergey,

On Thu, Mar 09, 2017 at 01:29:08PM +0900, Sergey Senozhatsky wrote:
> Hello Minchan,
> 
> /* I can't https://marc.info/?l=linux-kernel&m=148886631303107 thread
>    in my mail box for some reason so the Reply-To message-id may be wrong. */
> 
> 
> 
> commit "mm: fix lazyfree BUG_ON check in try_to_unmap_one()"
> (mmotm fd07630cbf59bead90046dd3e5cfd891e58e6987)
> 
> 
> 	if (VM_WARN_ON_ONCE(PageSwapBacked(page) !=
> 			PageSwapCache(page))) {
> 	...
> 	}
> 
> 
> does not compile on !CONFIG_DEBUG_VM configs, because VM_WARN_ONCE() is
> 
> 	#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
> 
> 
> 
> In file included from ./include/linux/mmdebug.h:4:0,
>                  from ./include/linux/mm.h:8,
>                  from mm/rmap.c:48:
> mm/rmap.c: In function ‘try_to_unmap_one’:
> ./include/linux/bug.h:45:33: error: void value not ignored as it ought to be
>  #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
>                                  ^
> ./include/linux/mmdebug.h:49:31: note: in expansion of macro ‘BUILD_BUG_ON_INVALID’
>  #define VM_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
>                                ^~~~~~~~~~~~~~~~~~~~
> mm/rmap.c:1416:8: note: in expansion of macro ‘VM_WARN_ON_ONCE’
>     if (VM_WARN_ON_ONCE(PageSwapBacked(page) !=
>         ^~~~~~~~~~~~~~~
> 
> 	-ss
> 

Thanks for the report, Sergey!
If others are not against, I want to go this.

From 38b10e560d066c2cef8f9d028e14008cefdaa3e0 Mon Sep 17 00:00:00 2001
From: Minchan Kim <minchan@kernel.org>
Date: Thu, 9 Mar 2017 14:58:23 +0900
Subject: [PATCH] mm: do not use VM_WARN_ON_ONCE as if condition

Sergey reported VM_WARN_ON_ONCE returns void with !CONFIG_DEBUG_VM
so we cannot use it as if's condition unlike WARN_ON.

This patch fixes it.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/rmap.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 1d82057144ba..7d24bb93445b 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1413,12 +1413,11 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 			 * Store the swap location in the pte.
 			 * See handle_pte_fault() ...
 			 */
-			if (VM_WARN_ON_ONCE(PageSwapBacked(page) !=
-						PageSwapCache(page))) {
+			if (unlikely(PageSwapBacked(page) != PageSwapCache(page))) {
+				WARN_ON_ONCE(1);
 				ret = SWAP_FAIL;
 				page_vma_mapped_walk_done(&pvmw);
 				break;
-
 			}
 
 			/* MADV_FREE page check */
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1595807

On Thu 09-03-17 15:02:26, Minchan Kim wrote:
[...]
> >From 38b10e560d066c2cef8f9d028e14008cefdaa3e0 Mon Sep 17 00:00:00 2001
> From: Minchan Kim <minchan@kernel.org>
> Date: Thu, 9 Mar 2017 14:58:23 +0900
> Subject: [PATCH] mm: do not use VM_WARN_ON_ONCE as if condition
> 
> Sergey reported VM_WARN_ON_ONCE returns void with !CONFIG_DEBUG_VM
> so we cannot use it as if's condition unlike WARN_ON.

I would swear I've seen WARN_ON_ONCE there when looking at the previous
patch! Btw. could have simply s@VM_@@ 

> This patch fixes it.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>

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

> ---
>  mm/rmap.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 1d82057144ba..7d24bb93445b 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1413,12 +1413,11 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>  			 * Store the swap location in the pte.
>  			 * See handle_pte_fault() ...
>  			 */
> -			if (VM_WARN_ON_ONCE(PageSwapBacked(page) !=
> -						PageSwapCache(page))) {
> +			if (unlikely(PageSwapBacked(page) != PageSwapCache(page))) {
> +				WARN_ON_ONCE(1);
>  				ret = SWAP_FAIL;
>  				page_vma_mapped_walk_done(&pvmw);
>  				break;
> -
>  			}
>  
>  			/* MADV_FREE page check */
> -- 
> 2.7.4

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1596394 — Re: "mm: fix lazyfree BUG_ON check in try_to_unmap_one()" build error

On Thu, 9 Mar 2017 15:02:26 +0900 Minchan Kim <minchan@kernel.org> wrote:

> Sergey reported VM_WARN_ON_ONCE returns void with !CONFIG_DEBUG_VM
> so we cannot use it as if's condition unlike WARN_ON.

Can we instead fix VM_WARN_ON_ONCE()?

[toc] | [prev] | [next] | [standalone]

#1596513

Hi Andrew,

On Thu, Mar 09, 2017 at 01:27:06PM -0800, Andrew Morton wrote:
> On Thu, 9 Mar 2017 15:02:26 +0900 Minchan Kim <minchan@kernel.org> wrote:
> 
> > Sergey reported VM_WARN_ON_ONCE returns void with !CONFIG_DEBUG_VM
> > so we cannot use it as if's condition unlike WARN_ON.
> 
> Can we instead fix VM_WARN_ON_ONCE()?

I thought the direction but the reason to decide WARN_ON_ONCE in this case
is losing of benefit with using CONFIG_DEBU_VM if we go that way.

I think the benefit with VM_WARN_ON friends is that it should be completely
out from the binary in !CONFIG_DEBUG_VM. However, if we fix VM_WARN_ON
like WARN_ON to !!condition, at least, compiler should generate condition
check and return so it's not what CONFIG_DEBUG_VM want, IMHO.
However, if guys believe it's okay to add some instructions to debug VM
although we disable CONFIG_DEBUG_VM, we can go that way.
It's a just policy matter. ;-)

Anyway, Even though we fix VM_WARN_ON_ONCE, in my case, WARN_ON_ONCE is
better because we should do !!condition regardless of CONFIG_DEBUG_VM
and if so, WARN_ON is more wide coverage than VM_WARN_ON which only works
with CONFIG_DEBUG_VM.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1596959

On 03/10/2017 01:45 AM, Minchan Kim wrote:
> Hi Andrew,
> 
> On Thu, Mar 09, 2017 at 01:27:06PM -0800, Andrew Morton wrote:
>> On Thu, 9 Mar 2017 15:02:26 +0900 Minchan Kim <minchan@kernel.org> wrote:
>>
>>> Sergey reported VM_WARN_ON_ONCE returns void with !CONFIG_DEBUG_VM
>>> so we cannot use it as if's condition unlike WARN_ON.
>>
>> Can we instead fix VM_WARN_ON_ONCE()?
> 
> I thought the direction but the reason to decide WARN_ON_ONCE in this case
> is losing of benefit with using CONFIG_DEBU_VM if we go that way.
> 
> I think the benefit with VM_WARN_ON friends is that it should be completely
> out from the binary in !CONFIG_DEBUG_VM. However, if we fix VM_WARN_ON
> like WARN_ON to !!condition, at least, compiler should generate condition
> check and return so it's not what CONFIG_DEBUG_VM want, IMHO.
> However, if guys believe it's okay to add some instructions to debug VM
> although we disable CONFIG_DEBUG_VM, we can go that way.
> It's a just policy matter. ;-)
> 
> Anyway, Even though we fix VM_WARN_ON_ONCE, in my case, WARN_ON_ONCE is
> better because we should do !!condition regardless of CONFIG_DEBUG_VM
> and if so, WARN_ON is more wide coverage than VM_WARN_ON which only works
> with CONFIG_DEBUG_VM.

Agreed. WARN_ON...() can work that way as one can't disable them
(AFAIK), but VM_* variants are optional for overhead reasons.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web