All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-01 21:38 ` Yang Shi
  0 siblings, 0 replies; 36+ messages in thread
From: Yang Shi @ 2017-11-01 21:38 UTC (permalink / raw)
  To: mhocko, akpm; +Cc: Yang Shi, linux-mm, linux-kernel

commit 3e51f3c4004c9b01f66da03214a3e206f5ed627b
("sched/preempt: Remove PREEMPT_ACTIVE unmasking off in_atomic()") makes
in_atomic() just check the preempt count, so it is not necessary to use
preempt_count() in print_vma_addr() any more. Replace preempt_count() to
in_atomic() which is a generic API for checking atomic context.

Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
---
 mm/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory.c b/mm/memory.c
index a728bed..19b684e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4460,7 +4460,7 @@ void print_vma_addr(char *prefix, unsigned long ip)
 	 * Do not print if we are in atomic
 	 * contexts (in exception stacks, etc.):
 	 */
-	if (preempt_count())
+	if (in_atomic())
 		return;
 
 	down_read(&mm->mmap_sem);
-- 
1.8.3.1

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

* [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-01 21:38 ` Yang Shi
  0 siblings, 0 replies; 36+ messages in thread
From: Yang Shi @ 2017-11-01 21:38 UTC (permalink / raw)
  To: mhocko, akpm; +Cc: Yang Shi, linux-mm, linux-kernel

commit 3e51f3c4004c9b01f66da03214a3e206f5ed627b
("sched/preempt: Remove PREEMPT_ACTIVE unmasking off in_atomic()") makes
in_atomic() just check the preempt count, so it is not necessary to use
preempt_count() in print_vma_addr() any more. Replace preempt_count() to
in_atomic() which is a generic API for checking atomic context.

Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
---
 mm/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory.c b/mm/memory.c
index a728bed..19b684e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4460,7 +4460,7 @@ void print_vma_addr(char *prefix, unsigned long ip)
 	 * Do not print if we are in atomic
 	 * contexts (in exception stacks, etc.):
 	 */
-	if (preempt_count())
+	if (in_atomic())
 		return;
 
 	down_read(&mm->mmap_sem);
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-01 21:38 ` Yang Shi
@ 2017-11-02  7:57   ` Michal Hocko
  -1 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-02  7:57 UTC (permalink / raw)
  To: Yang Shi; +Cc: akpm, linux-mm, linux-kernel

On Thu 02-11-17 05:38:33, Yang Shi wrote:
> commit 3e51f3c4004c9b01f66da03214a3e206f5ed627b
> ("sched/preempt: Remove PREEMPT_ACTIVE unmasking off in_atomic()") makes
> in_atomic() just check the preempt count, so it is not necessary to use
> preempt_count() in print_vma_addr() any more. Replace preempt_count() to
> in_atomic() which is a generic API for checking atomic context.

But why? Is there some general work to get rid of the direct preempt_count
usage outside of the generic API?

> Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
> ---
>  mm/memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a728bed..19b684e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4460,7 +4460,7 @@ void print_vma_addr(char *prefix, unsigned long ip)
>  	 * Do not print if we are in atomic
>  	 * contexts (in exception stacks, etc.):
>  	 */
> -	if (preempt_count())
> +	if (in_atomic())
>  		return;
>  
>  	down_read(&mm->mmap_sem);
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-02  7:57   ` Michal Hocko
  0 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-02  7:57 UTC (permalink / raw)
  To: Yang Shi; +Cc: akpm, linux-mm, linux-kernel

On Thu 02-11-17 05:38:33, Yang Shi wrote:
> commit 3e51f3c4004c9b01f66da03214a3e206f5ed627b
> ("sched/preempt: Remove PREEMPT_ACTIVE unmasking off in_atomic()") makes
> in_atomic() just check the preempt count, so it is not necessary to use
> preempt_count() in print_vma_addr() any more. Replace preempt_count() to
> in_atomic() which is a generic API for checking atomic context.

But why? Is there some general work to get rid of the direct preempt_count
usage outside of the generic API?

> Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
> ---
>  mm/memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a728bed..19b684e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4460,7 +4460,7 @@ void print_vma_addr(char *prefix, unsigned long ip)
>  	 * Do not print if we are in atomic
>  	 * contexts (in exception stacks, etc.):
>  	 */
> -	if (preempt_count())
> +	if (in_atomic())
>  		return;
>  
>  	down_read(&mm->mmap_sem);
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-02  7:57   ` Michal Hocko
@ 2017-11-02 17:44     ` Yang Shi
  -1 siblings, 0 replies; 36+ messages in thread
From: Yang Shi @ 2017-11-02 17:44 UTC (permalink / raw)
  To: Michal Hocko, mingo; +Cc: akpm, linux-mm, linux-kernel



On 11/2/17 12:57 AM, Michal Hocko wrote:
> On Thu 02-11-17 05:38:33, Yang Shi wrote:
>> commit 3e51f3c4004c9b01f66da03214a3e206f5ed627b
>> ("sched/preempt: Remove PREEMPT_ACTIVE unmasking off in_atomic()") makes
>> in_atomic() just check the preempt count, so it is not necessary to use
>> preempt_count() in print_vma_addr() any more. Replace preempt_count() to
>> in_atomic() which is a generic API for checking atomic context.
> 
> But why? Is there some general work to get rid of the direct preempt_count
> usage outside of the generic API?

I may not articulate it in the commit log, I would say "in_atomic" is 
*preferred* API for checking atomic context instead of preempt_count() 
which should be used for retrieving the preemption count value.

I would say there is not such general elimination work undergoing right 
now, but if we go through the kernel code, almost everywhere "in_atomic" 
is used for such use case already, except two places:

- print_vma_addr()
- debug_smp_processor_id()

Both came from Ingo long time ago before commit 
3e51f3c4004c9b01f66da03214a3e206f5ed627b ("sched/preempt: Remove 
PREEMPT_ACTIVE unmasking off in_atomic()"). But, after this commit was 
merged, I don't see why *not* use in_atomic() to follow the convention.

Thanks,
Yang

> 
>> Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
>> ---
>>   mm/memory.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index a728bed..19b684e 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -4460,7 +4460,7 @@ void print_vma_addr(char *prefix, unsigned long ip)
>>   	 * Do not print if we are in atomic
>>   	 * contexts (in exception stacks, etc.):
>>   	 */
>> -	if (preempt_count())
>> +	if (in_atomic())
>>   		return;
>>   
>>   	down_read(&mm->mmap_sem);
>> -- 
>> 1.8.3.1
> 

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-02 17:44     ` Yang Shi
  0 siblings, 0 replies; 36+ messages in thread
From: Yang Shi @ 2017-11-02 17:44 UTC (permalink / raw)
  To: Michal Hocko, mingo; +Cc: akpm, linux-mm, linux-kernel



On 11/2/17 12:57 AM, Michal Hocko wrote:
> On Thu 02-11-17 05:38:33, Yang Shi wrote:
>> commit 3e51f3c4004c9b01f66da03214a3e206f5ed627b
>> ("sched/preempt: Remove PREEMPT_ACTIVE unmasking off in_atomic()") makes
>> in_atomic() just check the preempt count, so it is not necessary to use
>> preempt_count() in print_vma_addr() any more. Replace preempt_count() to
>> in_atomic() which is a generic API for checking atomic context.
> 
> But why? Is there some general work to get rid of the direct preempt_count
> usage outside of the generic API?

I may not articulate it in the commit log, I would say "in_atomic" is 
*preferred* API for checking atomic context instead of preempt_count() 
which should be used for retrieving the preemption count value.

I would say there is not such general elimination work undergoing right 
now, but if we go through the kernel code, almost everywhere "in_atomic" 
is used for such use case already, except two places:

- print_vma_addr()
- debug_smp_processor_id()

Both came from Ingo long time ago before commit 
3e51f3c4004c9b01f66da03214a3e206f5ed627b ("sched/preempt: Remove 
PREEMPT_ACTIVE unmasking off in_atomic()"). But, after this commit was 
merged, I don't see why *not* use in_atomic() to follow the convention.

Thanks,
Yang

> 
>> Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
>> ---
>>   mm/memory.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index a728bed..19b684e 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -4460,7 +4460,7 @@ void print_vma_addr(char *prefix, unsigned long ip)
>>   	 * Do not print if we are in atomic
>>   	 * contexts (in exception stacks, etc.):
>>   	 */
>> -	if (preempt_count())
>> +	if (in_atomic())
>>   		return;
>>   
>>   	down_read(&mm->mmap_sem);
>> -- 
>> 1.8.3.1
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-02 17:44     ` Yang Shi
@ 2017-11-03  8:29       ` Michal Hocko
  -1 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-03  8:29 UTC (permalink / raw)
  To: Yang Shi; +Cc: mingo, akpm, linux-mm, linux-kernel

On Fri 03-11-17 01:44:44, Yang Shi wrote:
> 
> 
> On 11/2/17 12:57 AM, Michal Hocko wrote:
> > On Thu 02-11-17 05:38:33, Yang Shi wrote:
> > > commit 3e51f3c4004c9b01f66da03214a3e206f5ed627b
> > > ("sched/preempt: Remove PREEMPT_ACTIVE unmasking off in_atomic()") makes
> > > in_atomic() just check the preempt count, so it is not necessary to use
> > > preempt_count() in print_vma_addr() any more. Replace preempt_count() to
> > > in_atomic() which is a generic API for checking atomic context.
> > 
> > But why? Is there some general work to get rid of the direct preempt_count
> > usage outside of the generic API?
> 
> I may not articulate it in the commit log, I would say "in_atomic" is
> *preferred* API for checking atomic context instead of preempt_count() which
> should be used for retrieving the preemption count value.
> 
> I would say there is not such general elimination work undergoing right now,
> but if we go through the kernel code, almost everywhere "in_atomic" is used
> for such use case already, except two places:
> 
> - print_vma_addr()
> - debug_smp_processor_id()
> 
> Both came from Ingo long time ago before commit
> 3e51f3c4004c9b01f66da03214a3e206f5ed627b ("sched/preempt: Remove
> PREEMPT_ACTIVE unmasking off in_atomic()"). But, after this commit was
> merged, I don't see why *not* use in_atomic() to follow the convention.

OK.

Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks,
> Yang
> 
> > 
> > > Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
> > > ---
> > >   mm/memory.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index a728bed..19b684e 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -4460,7 +4460,7 @@ void print_vma_addr(char *prefix, unsigned long ip)
> > >   	 * Do not print if we are in atomic
> > >   	 * contexts (in exception stacks, etc.):
> > >   	 */
> > > -	if (preempt_count())
> > > +	if (in_atomic())
> > >   		return;
> > >   	down_read(&mm->mmap_sem);
> > > -- 
> > > 1.8.3.1
> > 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-03  8:29       ` Michal Hocko
  0 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-03  8:29 UTC (permalink / raw)
  To: Yang Shi; +Cc: mingo, akpm, linux-mm, linux-kernel

On Fri 03-11-17 01:44:44, Yang Shi wrote:
> 
> 
> On 11/2/17 12:57 AM, Michal Hocko wrote:
> > On Thu 02-11-17 05:38:33, Yang Shi wrote:
> > > commit 3e51f3c4004c9b01f66da03214a3e206f5ed627b
> > > ("sched/preempt: Remove PREEMPT_ACTIVE unmasking off in_atomic()") makes
> > > in_atomic() just check the preempt count, so it is not necessary to use
> > > preempt_count() in print_vma_addr() any more. Replace preempt_count() to
> > > in_atomic() which is a generic API for checking atomic context.
> > 
> > But why? Is there some general work to get rid of the direct preempt_count
> > usage outside of the generic API?
> 
> I may not articulate it in the commit log, I would say "in_atomic" is
> *preferred* API for checking atomic context instead of preempt_count() which
> should be used for retrieving the preemption count value.
> 
> I would say there is not such general elimination work undergoing right now,
> but if we go through the kernel code, almost everywhere "in_atomic" is used
> for such use case already, except two places:
> 
> - print_vma_addr()
> - debug_smp_processor_id()
> 
> Both came from Ingo long time ago before commit
> 3e51f3c4004c9b01f66da03214a3e206f5ed627b ("sched/preempt: Remove
> PREEMPT_ACTIVE unmasking off in_atomic()"). But, after this commit was
> merged, I don't see why *not* use in_atomic() to follow the convention.

OK.

Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks,
> Yang
> 
> > 
> > > Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
> > > ---
> > >   mm/memory.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index a728bed..19b684e 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -4460,7 +4460,7 @@ void print_vma_addr(char *prefix, unsigned long ip)
> > >   	 * Do not print if we are in atomic
> > >   	 * contexts (in exception stacks, etc.):
> > >   	 */
> > > -	if (preempt_count())
> > > +	if (in_atomic())
> > >   		return;
> > >   	down_read(&mm->mmap_sem);
> > > -- 
> > > 1.8.3.1
> > 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-02 17:44     ` Yang Shi
@ 2017-11-03 18:02       ` Andrew Morton
  -1 siblings, 0 replies; 36+ messages in thread
From: Andrew Morton @ 2017-11-03 18:02 UTC (permalink / raw)
  To: Yang Shi; +Cc: Michal Hocko, mingo, linux-mm, linux-kernel, Joe Perches

On Fri, 03 Nov 2017 01:44:44 +0800 "Yang Shi" <yang.s@alibaba-inc.com> wrote:

> I may not articulate it in the commit log

You should have done so ;)

Here's the changelog I ended up with:

: From: "Yang Shi" <yang.s@alibaba-inc.com>
: Subject: mm: use in_atomic() in print_vma_addr()
: 
: 3e51f3c4004c9b ("sched/preempt: Remove PREEMPT_ACTIVE unmasking off
: in_atomic()") uses in_atomic() just check the preempt count, so it is not
: necessary to use preempt_count() in print_vma_addr() any more.  Replace
: preempt_count() to in_atomic() which is a generic API for checking atomic
: context.
: 
: in_atomic() is the preferred API for checking atomic context instead of
: preempt_count() which should be used for retrieving the preemption count
: value.
: 
: If we go through the kernel code, almost everywhere "in_atomic" is used
: for such use case already, except two places:
: 
: - print_vma_addr()
: - debug_smp_processor_id()
: 
: Both came from Ingo long time ago before 3e51f3c4004c9b01 ("sched/preempt:
: Remove PREEMPT_ACTIVE unmasking off in_atomic()").  But, after this commit
: was merged, use in_atomic() to follow the convention.
: 
: Link: http://lkml.kernel.org/r/1509572313-102989-1-git-send-email-yang.s@alibaba-inc.com
: Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
: Acked-by: Michal Hocko <mhocko@suse.com>
: Cc: Frederic Weisbecker <fweisbec@gmail.com>
: Cc: Ingo Molnar <mingo@elte.hu>



Also, checkpatch says

WARNING: use of in_atomic() is incorrect outside core kernel code
#43: FILE: mm/memory.c:4491:
+       if (in_atomic())

I don't recall why we did that, but perhaps this should be revisited?

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-03 18:02       ` Andrew Morton
  0 siblings, 0 replies; 36+ messages in thread
From: Andrew Morton @ 2017-11-03 18:02 UTC (permalink / raw)
  To: Yang Shi; +Cc: Michal Hocko, mingo, linux-mm, linux-kernel, Joe Perches

On Fri, 03 Nov 2017 01:44:44 +0800 "Yang Shi" <yang.s@alibaba-inc.com> wrote:

> I may not articulate it in the commit log

You should have done so ;)

Here's the changelog I ended up with:

: From: "Yang Shi" <yang.s@alibaba-inc.com>
: Subject: mm: use in_atomic() in print_vma_addr()
: 
: 3e51f3c4004c9b ("sched/preempt: Remove PREEMPT_ACTIVE unmasking off
: in_atomic()") uses in_atomic() just check the preempt count, so it is not
: necessary to use preempt_count() in print_vma_addr() any more.  Replace
: preempt_count() to in_atomic() which is a generic API for checking atomic
: context.
: 
: in_atomic() is the preferred API for checking atomic context instead of
: preempt_count() which should be used for retrieving the preemption count
: value.
: 
: If we go through the kernel code, almost everywhere "in_atomic" is used
: for such use case already, except two places:
: 
: - print_vma_addr()
: - debug_smp_processor_id()
: 
: Both came from Ingo long time ago before 3e51f3c4004c9b01 ("sched/preempt:
: Remove PREEMPT_ACTIVE unmasking off in_atomic()").  But, after this commit
: was merged, use in_atomic() to follow the convention.
: 
: Link: http://lkml.kernel.org/r/1509572313-102989-1-git-send-email-yang.s@alibaba-inc.com
: Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
: Acked-by: Michal Hocko <mhocko@suse.com>
: Cc: Frederic Weisbecker <fweisbec@gmail.com>
: Cc: Ingo Molnar <mingo@elte.hu>



Also, checkpatch says

WARNING: use of in_atomic() is incorrect outside core kernel code
#43: FILE: mm/memory.c:4491:
+       if (in_atomic())

I don't recall why we did that, but perhaps this should be revisited?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-03 18:02       ` Andrew Morton
@ 2017-11-03 18:16         ` Yang Shi
  -1 siblings, 0 replies; 36+ messages in thread
From: Yang Shi @ 2017-11-03 18:16 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Michal Hocko, mingo, linux-mm, linux-kernel, Joe Perches



On 11/3/17 11:02 AM, Andrew Morton wrote:
> On Fri, 03 Nov 2017 01:44:44 +0800 "Yang Shi" <yang.s@alibaba-inc.com> wrote:
> 
>> I may not articulate it in the commit log
> 
> You should have done so ;)

Yes, definitely. I could done it much better.

> 
> Here's the changelog I ended up with:
> 
> : From: "Yang Shi" <yang.s@alibaba-inc.com>
> : Subject: mm: use in_atomic() in print_vma_addr()
> :
> : 3e51f3c4004c9b ("sched/preempt: Remove PREEMPT_ACTIVE unmasking off
> : in_atomic()") uses in_atomic() just check the preempt count, so it is not
> : necessary to use preempt_count() in print_vma_addr() any more.  Replace
> : preempt_count() to in_atomic() which is a generic API for checking atomic
> : context.
> :
> : in_atomic() is the preferred API for checking atomic context instead of
> : preempt_count() which should be used for retrieving the preemption count
> : value.
> :
> : If we go through the kernel code, almost everywhere "in_atomic" is used
> : for such use case already, except two places:
> :
> : - print_vma_addr()
> : - debug_smp_processor_id()
> :
> : Both came from Ingo long time ago before 3e51f3c4004c9b01 ("sched/preempt:
> : Remove PREEMPT_ACTIVE unmasking off in_atomic()").  But, after this commit
> : was merged, use in_atomic() to follow the convention.
> :
> : Link: http://lkml.kernel.org/r/1509572313-102989-1-git-send-email-yang.s@alibaba-inc.com
> : Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
> : Acked-by: Michal Hocko <mhocko@suse.com>
> : Cc: Frederic Weisbecker <fweisbec@gmail.com>
> : Cc: Ingo Molnar <mingo@elte.hu>

Thanks a lot for reworking the commit log.

> 
> 
> 
> Also, checkpatch says
> 
> WARNING: use of in_atomic() is incorrect outside core kernel code
> #43: FILE: mm/memory.c:4491:
> +       if (in_atomic())
> 
> I don't recall why we did that, but perhaps this should be revisited?

I think the rule for in_atomic is obsolete in checkpatch.pl. A quick 
grep shows in_atomic() is used by arch, drivers, crypto, even though the 
comment in include/linux/preempt.h says in_atomic() should be not used 
by drivers.

However, the message could be ignored with --ignore=IN_ATOMIC. But, it 
sounds better to fix the wrong rule and maybe even the comment in 
include/linux/preempt.h since it sounds confusing.

Thanks,
Yang
> 

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-03 18:16         ` Yang Shi
  0 siblings, 0 replies; 36+ messages in thread
From: Yang Shi @ 2017-11-03 18:16 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Michal Hocko, mingo, linux-mm, linux-kernel, Joe Perches



On 11/3/17 11:02 AM, Andrew Morton wrote:
> On Fri, 03 Nov 2017 01:44:44 +0800 "Yang Shi" <yang.s@alibaba-inc.com> wrote:
> 
>> I may not articulate it in the commit log
> 
> You should have done so ;)

Yes, definitely. I could done it much better.

> 
> Here's the changelog I ended up with:
> 
> : From: "Yang Shi" <yang.s@alibaba-inc.com>
> : Subject: mm: use in_atomic() in print_vma_addr()
> :
> : 3e51f3c4004c9b ("sched/preempt: Remove PREEMPT_ACTIVE unmasking off
> : in_atomic()") uses in_atomic() just check the preempt count, so it is not
> : necessary to use preempt_count() in print_vma_addr() any more.  Replace
> : preempt_count() to in_atomic() which is a generic API for checking atomic
> : context.
> :
> : in_atomic() is the preferred API for checking atomic context instead of
> : preempt_count() which should be used for retrieving the preemption count
> : value.
> :
> : If we go through the kernel code, almost everywhere "in_atomic" is used
> : for such use case already, except two places:
> :
> : - print_vma_addr()
> : - debug_smp_processor_id()
> :
> : Both came from Ingo long time ago before 3e51f3c4004c9b01 ("sched/preempt:
> : Remove PREEMPT_ACTIVE unmasking off in_atomic()").  But, after this commit
> : was merged, use in_atomic() to follow the convention.
> :
> : Link: http://lkml.kernel.org/r/1509572313-102989-1-git-send-email-yang.s@alibaba-inc.com
> : Signed-off-by: Yang Shi <yang.s@alibaba-inc.com>
> : Acked-by: Michal Hocko <mhocko@suse.com>
> : Cc: Frederic Weisbecker <fweisbec@gmail.com>
> : Cc: Ingo Molnar <mingo@elte.hu>

Thanks a lot for reworking the commit log.

> 
> 
> 
> Also, checkpatch says
> 
> WARNING: use of in_atomic() is incorrect outside core kernel code
> #43: FILE: mm/memory.c:4491:
> +       if (in_atomic())
> 
> I don't recall why we did that, but perhaps this should be revisited?

I think the rule for in_atomic is obsolete in checkpatch.pl. A quick 
grep shows in_atomic() is used by arch, drivers, crypto, even though the 
comment in include/linux/preempt.h says in_atomic() should be not used 
by drivers.

However, the message could be ignored with --ignore=IN_ATOMIC. But, it 
sounds better to fix the wrong rule and maybe even the comment in 
include/linux/preempt.h since it sounds confusing.

Thanks,
Yang
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-03 18:02       ` Andrew Morton
@ 2017-11-03 20:09         ` Bart Van Assche
  -1 siblings, 0 replies; 36+ messages in thread
From: Bart Van Assche @ 2017-11-03 20:09 UTC (permalink / raw)
  To: yang.s, akpm; +Cc: joe, linux-kernel, linux-mm, mhocko, mingo

On Fri, 2017-11-03 at 11:02 -0700, Andrew Morton wrote:
> Also, checkpatch says
> 
> WARNING: use of in_atomic() is incorrect outside core kernel code
> #43: FILE: mm/memory.c:4491:
> +       if (in_atomic())
> 
> I don't recall why we did that, but perhaps this should be revisited?

Is the comment above in_atomic() still up-to-date? From <linux/preempt.h>:

/*
 * Are we running in atomic context?  WARNING: this macro cannot
 * always detect atomic context; in particular, it cannot know about
 * held spinlocks in non-preemptible kernels.  Thus it should not be
 * used in the general case to determine whether sleeping is possible.
 * Do not use in_atomic() in driver code.
 */
#define in_atomic()	(preempt_count() != 0)

Bart.

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-03 20:09         ` Bart Van Assche
  0 siblings, 0 replies; 36+ messages in thread
From: Bart Van Assche @ 2017-11-03 20:09 UTC (permalink / raw)
  To: yang.s, akpm; +Cc: joe, linux-kernel, linux-mm, mhocko, mingo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 852 bytes --]

On Fri, 2017-11-03 at 11:02 -0700, Andrew Morton wrote:
> Also, checkpatch says
> 
> WARNING: use of in_atomic() is incorrect outside core kernel code
> #43: FILE: mm/memory.c:4491:
> +       if (in_atomic())
> 
> I don't recall why we did that, but perhaps this should be revisited?

Is the comment above in_atomic() still up-to-date? From <linux/preempt.h>:

/*
 * Are we running in atomic context?  WARNING: this macro cannot
 * always detect atomic context; in particular, it cannot know about
 * held spinlocks in non-preemptible kernels.  Thus it should not be
 * used in the general case to determine whether sleeping is possible.
 * Do not use in_atomic() in driver code.
 */
#define in_atomic()	(preempt_count() != 0)

Bart.N‹§²æìr¸›zǧu©ž²Æ {\b­†éì¹»\x1c®&Þ–)îÆi¢žØ^n‡r¶‰šŽŠÝ¢j$½§$¢¸\x05¢¹¨­è§~Š'.)îÄÃ,yèm¶ŸÿÃ\f%Š{±šj+ƒðèž×¦j)Z†·Ÿ

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-03 20:09         ` Bart Van Assche
@ 2017-11-05  8:19           ` Michal Hocko
  -1 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-05  8:19 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: yang.s, akpm, joe, linux-kernel, linux-mm, mingo, Peter Zijlstra

[CC Peter]

On Fri 03-11-17 20:09:49, Bart Van Assche wrote:
> On Fri, 2017-11-03 at 11:02 -0700, Andrew Morton wrote:
> > Also, checkpatch says
> > 
> > WARNING: use of in_atomic() is incorrect outside core kernel code
> > #43: FILE: mm/memory.c:4491:
> > +       if (in_atomic())
> > 
> > I don't recall why we did that, but perhaps this should be revisited?
> 
> Is the comment above in_atomic() still up-to-date? From <linux/preempt.h>:
> 
> /*
>  * Are we running in atomic context?  WARNING: this macro cannot
>  * always detect atomic context; in particular, it cannot know about
>  * held spinlocks in non-preemptible kernels.  Thus it should not be
>  * used in the general case to determine whether sleeping is possible.
>  * Do not use in_atomic() in driver code.
>  */
> #define in_atomic()	(preempt_count() != 0)

I can still see preempt_disable NOOP for !CONFIG_PREEMPT_COUNT kernels
which makes me think this is still a valid comment.
-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-05  8:19           ` Michal Hocko
  0 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-05  8:19 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: yang.s, akpm, joe, linux-kernel, linux-mm, mingo, Peter Zijlstra

[CC Peter]

On Fri 03-11-17 20:09:49, Bart Van Assche wrote:
> On Fri, 2017-11-03 at 11:02 -0700, Andrew Morton wrote:
> > Also, checkpatch says
> > 
> > WARNING: use of in_atomic() is incorrect outside core kernel code
> > #43: FILE: mm/memory.c:4491:
> > +       if (in_atomic())
> > 
> > I don't recall why we did that, but perhaps this should be revisited?
> 
> Is the comment above in_atomic() still up-to-date? From <linux/preempt.h>:
> 
> /*
>  * Are we running in atomic context?  WARNING: this macro cannot
>  * always detect atomic context; in particular, it cannot know about
>  * held spinlocks in non-preemptible kernels.  Thus it should not be
>  * used in the general case to determine whether sleeping is possible.
>  * Do not use in_atomic() in driver code.
>  */
> #define in_atomic()	(preempt_count() != 0)

I can still see preempt_disable NOOP for !CONFIG_PREEMPT_COUNT kernels
which makes me think this is still a valid comment.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-05  8:19           ` Michal Hocko
@ 2017-11-06 10:05             ` Peter Zijlstra
  -1 siblings, 0 replies; 36+ messages in thread
From: Peter Zijlstra @ 2017-11-06 10:05 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Sun, Nov 05, 2017 at 09:19:46AM +0100, Michal Hocko wrote:
> [CC Peter]
> 
> On Fri 03-11-17 20:09:49, Bart Van Assche wrote:
> > On Fri, 2017-11-03 at 11:02 -0700, Andrew Morton wrote:
> > > Also, checkpatch says
> > > 
> > > WARNING: use of in_atomic() is incorrect outside core kernel code
> > > #43: FILE: mm/memory.c:4491:
> > > +       if (in_atomic())
> > > 
> > > I don't recall why we did that, but perhaps this should be revisited?
> > 
> > Is the comment above in_atomic() still up-to-date? From <linux/preempt.h>:
> > 
> > /*
> >  * Are we running in atomic context?  WARNING: this macro cannot
> >  * always detect atomic context; in particular, it cannot know about
> >  * held spinlocks in non-preemptible kernels.  Thus it should not be
> >  * used in the general case to determine whether sleeping is possible.
> >  * Do not use in_atomic() in driver code.
> >  */
> > #define in_atomic()	(preempt_count() != 0)
> 
> I can still see preempt_disable NOOP for !CONFIG_PREEMPT_COUNT kernels
> which makes me think this is still a valid comment.

Yes the comment is very much accurate.

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-06 10:05             ` Peter Zijlstra
  0 siblings, 0 replies; 36+ messages in thread
From: Peter Zijlstra @ 2017-11-06 10:05 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Sun, Nov 05, 2017 at 09:19:46AM +0100, Michal Hocko wrote:
> [CC Peter]
> 
> On Fri 03-11-17 20:09:49, Bart Van Assche wrote:
> > On Fri, 2017-11-03 at 11:02 -0700, Andrew Morton wrote:
> > > Also, checkpatch says
> > > 
> > > WARNING: use of in_atomic() is incorrect outside core kernel code
> > > #43: FILE: mm/memory.c:4491:
> > > +       if (in_atomic())
> > > 
> > > I don't recall why we did that, but perhaps this should be revisited?
> > 
> > Is the comment above in_atomic() still up-to-date? From <linux/preempt.h>:
> > 
> > /*
> >  * Are we running in atomic context?  WARNING: this macro cannot
> >  * always detect atomic context; in particular, it cannot know about
> >  * held spinlocks in non-preemptible kernels.  Thus it should not be
> >  * used in the general case to determine whether sleeping is possible.
> >  * Do not use in_atomic() in driver code.
> >  */
> > #define in_atomic()	(preempt_count() != 0)
> 
> I can still see preempt_disable NOOP for !CONFIG_PREEMPT_COUNT kernels
> which makes me think this is still a valid comment.

Yes the comment is very much accurate.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-06 10:05             ` Peter Zijlstra
@ 2017-11-06 10:43               ` Michal Hocko
  -1 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 10:43 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Mon 06-11-17 11:05:58, Peter Zijlstra wrote:
> On Sun, Nov 05, 2017 at 09:19:46AM +0100, Michal Hocko wrote:
> > [CC Peter]
> > 
> > On Fri 03-11-17 20:09:49, Bart Van Assche wrote:
> > > On Fri, 2017-11-03 at 11:02 -0700, Andrew Morton wrote:
> > > > Also, checkpatch says
> > > > 
> > > > WARNING: use of in_atomic() is incorrect outside core kernel code
> > > > #43: FILE: mm/memory.c:4491:
> > > > +       if (in_atomic())
> > > > 
> > > > I don't recall why we did that, but perhaps this should be revisited?
> > > 
> > > Is the comment above in_atomic() still up-to-date? From <linux/preempt.h>:
> > > 
> > > /*
> > >  * Are we running in atomic context?  WARNING: this macro cannot
> > >  * always detect atomic context; in particular, it cannot know about
> > >  * held spinlocks in non-preemptible kernels.  Thus it should not be
> > >  * used in the general case to determine whether sleeping is possible.
> > >  * Do not use in_atomic() in driver code.
> > >  */
> > > #define in_atomic()	(preempt_count() != 0)
> > 
> > I can still see preempt_disable NOOP for !CONFIG_PREEMPT_COUNT kernels
> > which makes me think this is still a valid comment.
> 
> Yes the comment is very much accurate.

Which suggests that print_vma_addr might be problematic, right?
Shouldn't we do trylock on mmap_sem instead?

-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-06 10:43               ` Michal Hocko
  0 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 10:43 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Mon 06-11-17 11:05:58, Peter Zijlstra wrote:
> On Sun, Nov 05, 2017 at 09:19:46AM +0100, Michal Hocko wrote:
> > [CC Peter]
> > 
> > On Fri 03-11-17 20:09:49, Bart Van Assche wrote:
> > > On Fri, 2017-11-03 at 11:02 -0700, Andrew Morton wrote:
> > > > Also, checkpatch says
> > > > 
> > > > WARNING: use of in_atomic() is incorrect outside core kernel code
> > > > #43: FILE: mm/memory.c:4491:
> > > > +       if (in_atomic())
> > > > 
> > > > I don't recall why we did that, but perhaps this should be revisited?
> > > 
> > > Is the comment above in_atomic() still up-to-date? From <linux/preempt.h>:
> > > 
> > > /*
> > >  * Are we running in atomic context?  WARNING: this macro cannot
> > >  * always detect atomic context; in particular, it cannot know about
> > >  * held spinlocks in non-preemptible kernels.  Thus it should not be
> > >  * used in the general case to determine whether sleeping is possible.
> > >  * Do not use in_atomic() in driver code.
> > >  */
> > > #define in_atomic()	(preempt_count() != 0)
> > 
> > I can still see preempt_disable NOOP for !CONFIG_PREEMPT_COUNT kernels
> > which makes me think this is still a valid comment.
> 
> Yes the comment is very much accurate.

Which suggests that print_vma_addr might be problematic, right?
Shouldn't we do trylock on mmap_sem instead?

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-06 10:43               ` Michal Hocko
@ 2017-11-06 10:56                 ` Michal Hocko
  -1 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 10:56 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Mon 06-11-17 11:43:54, Michal Hocko wrote:
> On Mon 06-11-17 11:05:58, Peter Zijlstra wrote:
> > On Sun, Nov 05, 2017 at 09:19:46AM +0100, Michal Hocko wrote:
> > > [CC Peter]
> > > 
> > > On Fri 03-11-17 20:09:49, Bart Van Assche wrote:
> > > > On Fri, 2017-11-03 at 11:02 -0700, Andrew Morton wrote:
> > > > > Also, checkpatch says
> > > > > 
> > > > > WARNING: use of in_atomic() is incorrect outside core kernel code
> > > > > #43: FILE: mm/memory.c:4491:
> > > > > +       if (in_atomic())
> > > > > 
> > > > > I don't recall why we did that, but perhaps this should be revisited?
> > > > 
> > > > Is the comment above in_atomic() still up-to-date? From <linux/preempt.h>:
> > > > 
> > > > /*
> > > >  * Are we running in atomic context?  WARNING: this macro cannot
> > > >  * always detect atomic context; in particular, it cannot know about
> > > >  * held spinlocks in non-preemptible kernels.  Thus it should not be
> > > >  * used in the general case to determine whether sleeping is possible.
> > > >  * Do not use in_atomic() in driver code.
> > > >  */
> > > > #define in_atomic()	(preempt_count() != 0)
> > > 
> > > I can still see preempt_disable NOOP for !CONFIG_PREEMPT_COUNT kernels
> > > which makes me think this is still a valid comment.
> > 
> > Yes the comment is very much accurate.
> 
> Which suggests that print_vma_addr might be problematic, right?
> Shouldn't we do trylock on mmap_sem instead?

I might be missing something but the check seems to be broken. The
original commit by Ingo e8bff74afbdb ("x86: fix "BUG: sleeping function
called from invalid context" in print_vma_addr()") relied on elevated
preempt count by preempt_conditional_sti which is gone for quite some
time. First replaced by explicit preempt_disable in d99e1bd175f4
("x86/entry/traps: Refactor preemption and interrupt flag handling").

So unless I am missing something this check doesn't work and we should
rather do the trylock thing.
-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-06 10:56                 ` Michal Hocko
  0 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 10:56 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Mon 06-11-17 11:43:54, Michal Hocko wrote:
> On Mon 06-11-17 11:05:58, Peter Zijlstra wrote:
> > On Sun, Nov 05, 2017 at 09:19:46AM +0100, Michal Hocko wrote:
> > > [CC Peter]
> > > 
> > > On Fri 03-11-17 20:09:49, Bart Van Assche wrote:
> > > > On Fri, 2017-11-03 at 11:02 -0700, Andrew Morton wrote:
> > > > > Also, checkpatch says
> > > > > 
> > > > > WARNING: use of in_atomic() is incorrect outside core kernel code
> > > > > #43: FILE: mm/memory.c:4491:
> > > > > +       if (in_atomic())
> > > > > 
> > > > > I don't recall why we did that, but perhaps this should be revisited?
> > > > 
> > > > Is the comment above in_atomic() still up-to-date? From <linux/preempt.h>:
> > > > 
> > > > /*
> > > >  * Are we running in atomic context?  WARNING: this macro cannot
> > > >  * always detect atomic context; in particular, it cannot know about
> > > >  * held spinlocks in non-preemptible kernels.  Thus it should not be
> > > >  * used in the general case to determine whether sleeping is possible.
> > > >  * Do not use in_atomic() in driver code.
> > > >  */
> > > > #define in_atomic()	(preempt_count() != 0)
> > > 
> > > I can still see preempt_disable NOOP for !CONFIG_PREEMPT_COUNT kernels
> > > which makes me think this is still a valid comment.
> > 
> > Yes the comment is very much accurate.
> 
> Which suggests that print_vma_addr might be problematic, right?
> Shouldn't we do trylock on mmap_sem instead?

I might be missing something but the check seems to be broken. The
original commit by Ingo e8bff74afbdb ("x86: fix "BUG: sleeping function
called from invalid context" in print_vma_addr()") relied on elevated
preempt count by preempt_conditional_sti which is gone for quite some
time. First replaced by explicit preempt_disable in d99e1bd175f4
("x86/entry/traps: Refactor preemption and interrupt flag handling").

So unless I am missing something this check doesn't work and we should
rather do the trylock thing.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-06 10:43               ` Michal Hocko
@ 2017-11-06 12:00                 ` Peter Zijlstra
  -1 siblings, 0 replies; 36+ messages in thread
From: Peter Zijlstra @ 2017-11-06 12:00 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
> > Yes the comment is very much accurate.
> 
> Which suggests that print_vma_addr might be problematic, right?
> Shouldn't we do trylock on mmap_sem instead?

Yes that's complete rubbish. trylock will get spurious failures to print
when the lock is contended.

The right solution is to not call this thing when you can't schedule,
trying to divine the state in the print function is doomed to failure.

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-06 12:00                 ` Peter Zijlstra
  0 siblings, 0 replies; 36+ messages in thread
From: Peter Zijlstra @ 2017-11-06 12:00 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
> > Yes the comment is very much accurate.
> 
> Which suggests that print_vma_addr might be problematic, right?
> Shouldn't we do trylock on mmap_sem instead?

Yes that's complete rubbish. trylock will get spurious failures to print
when the lock is contended.

The right solution is to not call this thing when you can't schedule,
trying to divine the state in the print function is doomed to failure.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
  2017-11-06 12:00                 ` Peter Zijlstra
@ 2017-11-06 12:12                   ` Michal Hocko
  -1 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 12:12 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
> On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
> > > Yes the comment is very much accurate.
> > 
> > Which suggests that print_vma_addr might be problematic, right?
> > Shouldn't we do trylock on mmap_sem instead?
> 
> Yes that's complete rubbish. trylock will get spurious failures to print
> when the lock is contended.

Yes, but I guess that it is acceptable to to not print the state under
that condition.
-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm: use in_atomic() in print_vma_addr()
@ 2017-11-06 12:12                   ` Michal Hocko
  0 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 12:12 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
> On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
> > > Yes the comment is very much accurate.
> > 
> > Which suggests that print_vma_addr might be problematic, right?
> > Shouldn't we do trylock on mmap_sem instead?
> 
> Yes that's complete rubbish. trylock will get spurious failures to print
> when the lock is contended.

Yes, but I guess that it is acceptable to to not print the state under
that condition.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* [PATCH] mm: do not rely on preempt_count in print_vma_addr (was: Re: [PATCH] mm: use in_atomic() in print_vma_addr())
  2017-11-06 12:12                   ` Michal Hocko
@ 2017-11-06 13:40                     ` Michal Hocko
  -1 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 13:40 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Mon 06-11-17 13:12:22, Michal Hocko wrote:
> On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
> > On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
> > > > Yes the comment is very much accurate.
> > > 
> > > Which suggests that print_vma_addr might be problematic, right?
> > > Shouldn't we do trylock on mmap_sem instead?
> > 
> > Yes that's complete rubbish. trylock will get spurious failures to print
> > when the lock is contended.
> 
> Yes, but I guess that it is acceptable to to not print the state under
> that condition.

So what do you think about this? I think this is more robust than
playing tricks with the explicit preempt count checks and less tedious
than checking to make it conditional on the context. This is on top of
Linus tree and if accepted it should replace the patch discussed here.
---
>From 0de6d57cbc54ee2686d1f1e4ffcc4ed490ded8aa Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 6 Nov 2017 14:31:20 +0100
Subject: [PATCH] mm: do not rely on preempt_count in print_vma_addr

The preempt count check on print_vma_addr has been added by e8bff74afbdb
("x86: fix "BUG: sleeping function called from invalid context" in
print_vma_addr()") and it relied on the elevated preempt count from
preempt_conditional_sti because preempt_count check doesn't work on
non preemptive kernels by default. The code has evolved though and
d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag
handling") has replaced preempt_conditional_sti by an explicit
preempt_disable which is noop on !PREEMPT so the check in print_vma_addr
is broken.

Fix the issue by using trylock on mmap_sem rather than chacking the
preempt count. The allocation we are relying on has to be GFP_NOWAIT
as well. There is a chance that we won't dump the vma state if the lock
is contended or the memory short but this is acceptable outcome and much
less fragile than the not working preemption check or tricks around it.

Fixes: d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag handling")
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memory.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index a728bed16c20..1e308ac8ca0a 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4457,17 +4457,15 @@ void print_vma_addr(char *prefix, unsigned long ip)
 	struct vm_area_struct *vma;
 
 	/*
-	 * Do not print if we are in atomic
-	 * contexts (in exception stacks, etc.):
+	 * we might be running from an atomic context so we cannot sleep
 	 */
-	if (preempt_count())
+	if (!down_read_trylock(&mm->mmap_sem))
 		return;
 
-	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, ip);
 	if (vma && vma->vm_file) {
 		struct file *f = vma->vm_file;
-		char *buf = (char *)__get_free_page(GFP_KERNEL);
+		char *buf = (char *)__get_free_page(GFP_NOWAIT);
 		if (buf) {
 			char *p;
 
-- 
2.14.2

-- 
Michal Hocko
SUSE Labs

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

* [PATCH] mm: do not rely on preempt_count in print_vma_addr (was: Re: [PATCH] mm: use in_atomic() in print_vma_addr())
@ 2017-11-06 13:40                     ` Michal Hocko
  0 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 13:40 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On Mon 06-11-17 13:12:22, Michal Hocko wrote:
> On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
> > On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
> > > > Yes the comment is very much accurate.
> > > 
> > > Which suggests that print_vma_addr might be problematic, right?
> > > Shouldn't we do trylock on mmap_sem instead?
> > 
> > Yes that's complete rubbish. trylock will get spurious failures to print
> > when the lock is contended.
> 
> Yes, but I guess that it is acceptable to to not print the state under
> that condition.

So what do you think about this? I think this is more robust than
playing tricks with the explicit preempt count checks and less tedious
than checking to make it conditional on the context. This is on top of
Linus tree and if accepted it should replace the patch discussed here.
---

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

* Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr
  2017-11-06 13:40                     ` Michal Hocko
@ 2017-11-06 14:19                       ` Vlastimil Babka
  -1 siblings, 0 replies; 36+ messages in thread
From: Vlastimil Babka @ 2017-11-06 14:19 UTC (permalink / raw)
  To: Michal Hocko, Peter Zijlstra
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On 11/06/2017 02:40 PM, Michal Hocko wrote:
> On Mon 06-11-17 13:12:22, Michal Hocko wrote:
>> On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
>>> On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
>>>>> Yes the comment is very much accurate.
>>>>
>>>> Which suggests that print_vma_addr might be problematic, right?
>>>> Shouldn't we do trylock on mmap_sem instead?
>>>
>>> Yes that's complete rubbish. trylock will get spurious failures to print
>>> when the lock is contended.
>>
>> Yes, but I guess that it is acceptable to to not print the state under
>> that condition.
> 
> So what do you think about this? I think this is more robust than
> playing tricks with the explicit preempt count checks and less tedious
> than checking to make it conditional on the context. This is on top of
> Linus tree and if accepted it should replace the patch discussed here.
> ---
> From 0de6d57cbc54ee2686d1f1e4ffcc4ed490ded8aa Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Mon, 6 Nov 2017 14:31:20 +0100
> Subject: [PATCH] mm: do not rely on preempt_count in print_vma_addr
> 
> The preempt count check on print_vma_addr has been added by e8bff74afbdb
> ("x86: fix "BUG: sleeping function called from invalid context" in
> print_vma_addr()") and it relied on the elevated preempt count from
> preempt_conditional_sti because preempt_count check doesn't work on
> non preemptive kernels by default. The code has evolved though and
> d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag
> handling") has replaced preempt_conditional_sti by an explicit
> preempt_disable which is noop on !PREEMPT so the check in print_vma_addr
> is broken.
> 
> Fix the issue by using trylock on mmap_sem rather than chacking the
> preempt count. The allocation we are relying on has to be GFP_NOWAIT
> as well. There is a chance that we won't dump the vma state if the lock
> is contended or the memory short but this is acceptable outcome and much
> less fragile than the not working preemption check or tricks around it.

If we fail to allocate the page, we could still print the addresses,
just miss the filename? But that's an improvement, not a fix.

> Fixes: d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag handling")
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/memory.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a728bed16c20..1e308ac8ca0a 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4457,17 +4457,15 @@ void print_vma_addr(char *prefix, unsigned long ip)
>  	struct vm_area_struct *vma;
>  
>  	/*
> -	 * Do not print if we are in atomic
> -	 * contexts (in exception stacks, etc.):
> +	 * we might be running from an atomic context so we cannot sleep
>  	 */
> -	if (preempt_count())
> +	if (!down_read_trylock(&mm->mmap_sem))
>  		return;
>  
> -	down_read(&mm->mmap_sem);
>  	vma = find_vma(mm, ip);
>  	if (vma && vma->vm_file) {
>  		struct file *f = vma->vm_file;
> -		char *buf = (char *)__get_free_page(GFP_KERNEL);
> +		char *buf = (char *)__get_free_page(GFP_NOWAIT);
>  		if (buf) {
>  			char *p;
>  
> 

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

* Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr
@ 2017-11-06 14:19                       ` Vlastimil Babka
  0 siblings, 0 replies; 36+ messages in thread
From: Vlastimil Babka @ 2017-11-06 14:19 UTC (permalink / raw)
  To: Michal Hocko, Peter Zijlstra
  Cc: Bart Van Assche, yang.s, akpm, joe, linux-kernel, linux-mm, mingo

On 11/06/2017 02:40 PM, Michal Hocko wrote:
> On Mon 06-11-17 13:12:22, Michal Hocko wrote:
>> On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
>>> On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
>>>>> Yes the comment is very much accurate.
>>>>
>>>> Which suggests that print_vma_addr might be problematic, right?
>>>> Shouldn't we do trylock on mmap_sem instead?
>>>
>>> Yes that's complete rubbish. trylock will get spurious failures to print
>>> when the lock is contended.
>>
>> Yes, but I guess that it is acceptable to to not print the state under
>> that condition.
> 
> So what do you think about this? I think this is more robust than
> playing tricks with the explicit preempt count checks and less tedious
> than checking to make it conditional on the context. This is on top of
> Linus tree and if accepted it should replace the patch discussed here.
> ---
> From 0de6d57cbc54ee2686d1f1e4ffcc4ed490ded8aa Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Mon, 6 Nov 2017 14:31:20 +0100
> Subject: [PATCH] mm: do not rely on preempt_count in print_vma_addr
> 
> The preempt count check on print_vma_addr has been added by e8bff74afbdb
> ("x86: fix "BUG: sleeping function called from invalid context" in
> print_vma_addr()") and it relied on the elevated preempt count from
> preempt_conditional_sti because preempt_count check doesn't work on
> non preemptive kernels by default. The code has evolved though and
> d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag
> handling") has replaced preempt_conditional_sti by an explicit
> preempt_disable which is noop on !PREEMPT so the check in print_vma_addr
> is broken.
> 
> Fix the issue by using trylock on mmap_sem rather than chacking the
> preempt count. The allocation we are relying on has to be GFP_NOWAIT
> as well. There is a chance that we won't dump the vma state if the lock
> is contended or the memory short but this is acceptable outcome and much
> less fragile than the not working preemption check or tricks around it.

If we fail to allocate the page, we could still print the addresses,
just miss the filename? But that's an improvement, not a fix.

> Fixes: d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag handling")
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/memory.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a728bed16c20..1e308ac8ca0a 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4457,17 +4457,15 @@ void print_vma_addr(char *prefix, unsigned long ip)
>  	struct vm_area_struct *vma;
>  
>  	/*
> -	 * Do not print if we are in atomic
> -	 * contexts (in exception stacks, etc.):
> +	 * we might be running from an atomic context so we cannot sleep
>  	 */
> -	if (preempt_count())
> +	if (!down_read_trylock(&mm->mmap_sem))
>  		return;
>  
> -	down_read(&mm->mmap_sem);
>  	vma = find_vma(mm, ip);
>  	if (vma && vma->vm_file) {
>  		struct file *f = vma->vm_file;
> -		char *buf = (char *)__get_free_page(GFP_KERNEL);
> +		char *buf = (char *)__get_free_page(GFP_NOWAIT);
>  		if (buf) {
>  			char *p;
>  
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr
  2017-11-06 14:19                       ` Vlastimil Babka
@ 2017-11-06 14:28                         ` Michal Hocko
  -1 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 14:28 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Peter Zijlstra, Bart Van Assche, yang.s, akpm, joe, linux-kernel,
	linux-mm, mingo

On Mon 06-11-17 15:19:46, Vlastimil Babka wrote:
> On 11/06/2017 02:40 PM, Michal Hocko wrote:
> > On Mon 06-11-17 13:12:22, Michal Hocko wrote:
> >> On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
> >>> On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
> >>>>> Yes the comment is very much accurate.
> >>>>
> >>>> Which suggests that print_vma_addr might be problematic, right?
> >>>> Shouldn't we do trylock on mmap_sem instead?
> >>>
> >>> Yes that's complete rubbish. trylock will get spurious failures to print
> >>> when the lock is contended.
> >>
> >> Yes, but I guess that it is acceptable to to not print the state under
> >> that condition.
> > 
> > So what do you think about this? I think this is more robust than
> > playing tricks with the explicit preempt count checks and less tedious
> > than checking to make it conditional on the context. This is on top of
> > Linus tree and if accepted it should replace the patch discussed here.
> > ---
> > From 0de6d57cbc54ee2686d1f1e4ffcc4ed490ded8aa Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Mon, 6 Nov 2017 14:31:20 +0100
> > Subject: [PATCH] mm: do not rely on preempt_count in print_vma_addr
> > 
> > The preempt count check on print_vma_addr has been added by e8bff74afbdb
> > ("x86: fix "BUG: sleeping function called from invalid context" in
> > print_vma_addr()") and it relied on the elevated preempt count from
> > preempt_conditional_sti because preempt_count check doesn't work on
> > non preemptive kernels by default. The code has evolved though and
> > d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag
> > handling") has replaced preempt_conditional_sti by an explicit
> > preempt_disable which is noop on !PREEMPT so the check in print_vma_addr
> > is broken.
> > 
> > Fix the issue by using trylock on mmap_sem rather than chacking the
> > preempt count. The allocation we are relying on has to be GFP_NOWAIT
> > as well. There is a chance that we won't dump the vma state if the lock
> > is contended or the memory short but this is acceptable outcome and much
> > less fragile than the not working preemption check or tricks around it.
> 
> If we fail to allocate the page, we could still print the addresses,
> just miss the filename? But that's an improvement, not a fix.

Agreed. Or we could have some preallocated buffer if this is more
widespread pattern

> > Fixes: d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag handling")
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks!

> 
> > ---
> >  mm/memory.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/memory.c b/mm/memory.c
> > index a728bed16c20..1e308ac8ca0a 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -4457,17 +4457,15 @@ void print_vma_addr(char *prefix, unsigned long ip)
> >  	struct vm_area_struct *vma;
> >  
> >  	/*
> > -	 * Do not print if we are in atomic
> > -	 * contexts (in exception stacks, etc.):
> > +	 * we might be running from an atomic context so we cannot sleep
> >  	 */
> > -	if (preempt_count())
> > +	if (!down_read_trylock(&mm->mmap_sem))
> >  		return;
> >  
> > -	down_read(&mm->mmap_sem);
> >  	vma = find_vma(mm, ip);
> >  	if (vma && vma->vm_file) {
> >  		struct file *f = vma->vm_file;
> > -		char *buf = (char *)__get_free_page(GFP_KERNEL);
> > +		char *buf = (char *)__get_free_page(GFP_NOWAIT);
> >  		if (buf) {
> >  			char *p;
> >  
> > 

-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr
@ 2017-11-06 14:28                         ` Michal Hocko
  0 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 14:28 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Peter Zijlstra, Bart Van Assche, yang.s, akpm, joe, linux-kernel,
	linux-mm, mingo

On Mon 06-11-17 15:19:46, Vlastimil Babka wrote:
> On 11/06/2017 02:40 PM, Michal Hocko wrote:
> > On Mon 06-11-17 13:12:22, Michal Hocko wrote:
> >> On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
> >>> On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
> >>>>> Yes the comment is very much accurate.
> >>>>
> >>>> Which suggests that print_vma_addr might be problematic, right?
> >>>> Shouldn't we do trylock on mmap_sem instead?
> >>>
> >>> Yes that's complete rubbish. trylock will get spurious failures to print
> >>> when the lock is contended.
> >>
> >> Yes, but I guess that it is acceptable to to not print the state under
> >> that condition.
> > 
> > So what do you think about this? I think this is more robust than
> > playing tricks with the explicit preempt count checks and less tedious
> > than checking to make it conditional on the context. This is on top of
> > Linus tree and if accepted it should replace the patch discussed here.
> > ---
> > From 0de6d57cbc54ee2686d1f1e4ffcc4ed490ded8aa Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Mon, 6 Nov 2017 14:31:20 +0100
> > Subject: [PATCH] mm: do not rely on preempt_count in print_vma_addr
> > 
> > The preempt count check on print_vma_addr has been added by e8bff74afbdb
> > ("x86: fix "BUG: sleeping function called from invalid context" in
> > print_vma_addr()") and it relied on the elevated preempt count from
> > preempt_conditional_sti because preempt_count check doesn't work on
> > non preemptive kernels by default. The code has evolved though and
> > d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag
> > handling") has replaced preempt_conditional_sti by an explicit
> > preempt_disable which is noop on !PREEMPT so the check in print_vma_addr
> > is broken.
> > 
> > Fix the issue by using trylock on mmap_sem rather than chacking the
> > preempt count. The allocation we are relying on has to be GFP_NOWAIT
> > as well. There is a chance that we won't dump the vma state if the lock
> > is contended or the memory short but this is acceptable outcome and much
> > less fragile than the not working preemption check or tricks around it.
> 
> If we fail to allocate the page, we could still print the addresses,
> just miss the filename? But that's an improvement, not a fix.

Agreed. Or we could have some preallocated buffer if this is more
widespread pattern

> > Fixes: d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag handling")
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks!

> 
> > ---
> >  mm/memory.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/memory.c b/mm/memory.c
> > index a728bed16c20..1e308ac8ca0a 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -4457,17 +4457,15 @@ void print_vma_addr(char *prefix, unsigned long ip)
> >  	struct vm_area_struct *vma;
> >  
> >  	/*
> > -	 * Do not print if we are in atomic
> > -	 * contexts (in exception stacks, etc.):
> > +	 * we might be running from an atomic context so we cannot sleep
> >  	 */
> > -	if (preempt_count())
> > +	if (!down_read_trylock(&mm->mmap_sem))
> >  		return;
> >  
> > -	down_read(&mm->mmap_sem);
> >  	vma = find_vma(mm, ip);
> >  	if (vma && vma->vm_file) {
> >  		struct file *f = vma->vm_file;
> > -		char *buf = (char *)__get_free_page(GFP_KERNEL);
> > +		char *buf = (char *)__get_free_page(GFP_NOWAIT);
> >  		if (buf) {
> >  			char *p;
> >  
> > 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr
  2017-11-06 13:40                     ` Michal Hocko
@ 2017-11-06 16:16                       ` Yang Shi
  -1 siblings, 0 replies; 36+ messages in thread
From: Yang Shi @ 2017-11-06 16:16 UTC (permalink / raw)
  To: Michal Hocko, Peter Zijlstra
  Cc: Bart Van Assche, akpm, joe, linux-kernel, linux-mm, mingo



On 11/6/17 5:40 AM, Michal Hocko wrote:
> On Mon 06-11-17 13:12:22, Michal Hocko wrote:
>> On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
>>> On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
>>>>> Yes the comment is very much accurate.
>>>>
>>>> Which suggests that print_vma_addr might be problematic, right?
>>>> Shouldn't we do trylock on mmap_sem instead?
>>>
>>> Yes that's complete rubbish. trylock will get spurious failures to print
>>> when the lock is contended.
>>
>> Yes, but I guess that it is acceptable to to not print the state under
>> that condition.
> 
> So what do you think about this? I think this is more robust than
> playing tricks with the explicit preempt count checks and less tedious
> than checking to make it conditional on the context. This is on top of
> Linus tree and if accepted it should replace the patch discussed here.
> ---
>  From 0de6d57cbc54ee2686d1f1e4ffcc4ed490ded8aa Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Mon, 6 Nov 2017 14:31:20 +0100
> Subject: [PATCH] mm: do not rely on preempt_count in print_vma_addr
> 
> The preempt count check on print_vma_addr has been added by e8bff74afbdb
> ("x86: fix "BUG: sleeping function called from invalid context" in
> print_vma_addr()") and it relied on the elevated preempt count from
> preempt_conditional_sti because preempt_count check doesn't work on
> non preemptive kernels by default. The code has evolved though and
> d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag
> handling") has replaced preempt_conditional_sti by an explicit
> preempt_disable which is noop on !PREEMPT so the check in print_vma_addr
> is broken.
> 
> Fix the issue by using trylock on mmap_sem rather than chacking the

s/chacking/checking

> preempt count. The allocation we are relying on has to be GFP_NOWAIT
> as well. There is a chance that we won't dump the vma state if the lock
> is contended or the memory short but this is acceptable outcome and much
> less fragile than the not working preemption check or tricks around it.
> 
> Fixes: d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag handling")
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Acked-by: Yang Shi <yang.s@alibaba-inc.com>

Regards,
Yang

> ---
>   mm/memory.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a728bed16c20..1e308ac8ca0a 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4457,17 +4457,15 @@ void print_vma_addr(char *prefix, unsigned long ip)
>   	struct vm_area_struct *vma;
>   
>   	/*
> -	 * Do not print if we are in atomic
> -	 * contexts (in exception stacks, etc.):
> +	 * we might be running from an atomic context so we cannot sleep
>   	 */
> -	if (preempt_count())
> +	if (!down_read_trylock(&mm->mmap_sem))
>   		return;
>   
> -	down_read(&mm->mmap_sem);
>   	vma = find_vma(mm, ip);
>   	if (vma && vma->vm_file) {
>   		struct file *f = vma->vm_file;
> -		char *buf = (char *)__get_free_page(GFP_KERNEL);
> +		char *buf = (char *)__get_free_page(GFP_NOWAIT);
>   		if (buf) {
>   			char *p;
>   
> 

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

* Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr
@ 2017-11-06 16:16                       ` Yang Shi
  0 siblings, 0 replies; 36+ messages in thread
From: Yang Shi @ 2017-11-06 16:16 UTC (permalink / raw)
  To: Michal Hocko, Peter Zijlstra
  Cc: Bart Van Assche, akpm, joe, linux-kernel, linux-mm, mingo



On 11/6/17 5:40 AM, Michal Hocko wrote:
> On Mon 06-11-17 13:12:22, Michal Hocko wrote:
>> On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
>>> On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
>>>>> Yes the comment is very much accurate.
>>>>
>>>> Which suggests that print_vma_addr might be problematic, right?
>>>> Shouldn't we do trylock on mmap_sem instead?
>>>
>>> Yes that's complete rubbish. trylock will get spurious failures to print
>>> when the lock is contended.
>>
>> Yes, but I guess that it is acceptable to to not print the state under
>> that condition.
> 
> So what do you think about this? I think this is more robust than
> playing tricks with the explicit preempt count checks and less tedious
> than checking to make it conditional on the context. This is on top of
> Linus tree and if accepted it should replace the patch discussed here.
> ---
>  From 0de6d57cbc54ee2686d1f1e4ffcc4ed490ded8aa Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Mon, 6 Nov 2017 14:31:20 +0100
> Subject: [PATCH] mm: do not rely on preempt_count in print_vma_addr
> 
> The preempt count check on print_vma_addr has been added by e8bff74afbdb
> ("x86: fix "BUG: sleeping function called from invalid context" in
> print_vma_addr()") and it relied on the elevated preempt count from
> preempt_conditional_sti because preempt_count check doesn't work on
> non preemptive kernels by default. The code has evolved though and
> d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag
> handling") has replaced preempt_conditional_sti by an explicit
> preempt_disable which is noop on !PREEMPT so the check in print_vma_addr
> is broken.
> 
> Fix the issue by using trylock on mmap_sem rather than chacking the

s/chacking/checking

> preempt count. The allocation we are relying on has to be GFP_NOWAIT
> as well. There is a chance that we won't dump the vma state if the lock
> is contended or the memory short but this is acceptable outcome and much
> less fragile than the not working preemption check or tricks around it.
> 
> Fixes: d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag handling")
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Acked-by: Yang Shi <yang.s@alibaba-inc.com>

Regards,
Yang

> ---
>   mm/memory.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a728bed16c20..1e308ac8ca0a 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4457,17 +4457,15 @@ void print_vma_addr(char *prefix, unsigned long ip)
>   	struct vm_area_struct *vma;
>   
>   	/*
> -	 * Do not print if we are in atomic
> -	 * contexts (in exception stacks, etc.):
> +	 * we might be running from an atomic context so we cannot sleep
>   	 */
> -	if (preempt_count())
> +	if (!down_read_trylock(&mm->mmap_sem))
>   		return;
>   
> -	down_read(&mm->mmap_sem);
>   	vma = find_vma(mm, ip);
>   	if (vma && vma->vm_file) {
>   		struct file *f = vma->vm_file;
> -		char *buf = (char *)__get_free_page(GFP_KERNEL);
> +		char *buf = (char *)__get_free_page(GFP_NOWAIT);
>   		if (buf) {
>   			char *p;
>   
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

* Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr
  2017-11-06 16:16                       ` Yang Shi
@ 2017-11-06 16:24                         ` Michal Hocko
  -1 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 16:24 UTC (permalink / raw)
  To: Yang Shi
  Cc: Peter Zijlstra, Bart Van Assche, akpm, joe, linux-kernel,
	linux-mm, mingo

On Tue 07-11-17 00:16:58, Yang Shi wrote:
> 
> 
> On 11/6/17 5:40 AM, Michal Hocko wrote:
> > On Mon 06-11-17 13:12:22, Michal Hocko wrote:
> > > On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
> > > > On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
> > > > > > Yes the comment is very much accurate.
> > > > > 
> > > > > Which suggests that print_vma_addr might be problematic, right?
> > > > > Shouldn't we do trylock on mmap_sem instead?
> > > > 
> > > > Yes that's complete rubbish. trylock will get spurious failures to print
> > > > when the lock is contended.
> > > 
> > > Yes, but I guess that it is acceptable to to not print the state under
> > > that condition.
> > 
> > So what do you think about this? I think this is more robust than
> > playing tricks with the explicit preempt count checks and less tedious
> > than checking to make it conditional on the context. This is on top of
> > Linus tree and if accepted it should replace the patch discussed here.
> > ---
> >  From 0de6d57cbc54ee2686d1f1e4ffcc4ed490ded8aa Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Mon, 6 Nov 2017 14:31:20 +0100
> > Subject: [PATCH] mm: do not rely on preempt_count in print_vma_addr
> > 
> > The preempt count check on print_vma_addr has been added by e8bff74afbdb
> > ("x86: fix "BUG: sleeping function called from invalid context" in
> > print_vma_addr()") and it relied on the elevated preempt count from
> > preempt_conditional_sti because preempt_count check doesn't work on
> > non preemptive kernels by default. The code has evolved though and
> > d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag
> > handling") has replaced preempt_conditional_sti by an explicit
> > preempt_disable which is noop on !PREEMPT so the check in print_vma_addr
> > is broken.
> > 
> > Fix the issue by using trylock on mmap_sem rather than chacking the
> 
> s/chacking/checking

ups, fixed

> > preempt count. The allocation we are relying on has to be GFP_NOWAIT
> > as well. There is a chance that we won't dump the vma state if the lock
> > is contended or the memory short but this is acceptable outcome and much
> > less fragile than the not working preemption check or tricks around it.
> > 
> > Fixes: d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag handling")
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Acked-by: Yang Shi <yang.s@alibaba-inc.com>

Thanks!
-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr
@ 2017-11-06 16:24                         ` Michal Hocko
  0 siblings, 0 replies; 36+ messages in thread
From: Michal Hocko @ 2017-11-06 16:24 UTC (permalink / raw)
  To: Yang Shi
  Cc: Peter Zijlstra, Bart Van Assche, akpm, joe, linux-kernel,
	linux-mm, mingo

On Tue 07-11-17 00:16:58, Yang Shi wrote:
> 
> 
> On 11/6/17 5:40 AM, Michal Hocko wrote:
> > On Mon 06-11-17 13:12:22, Michal Hocko wrote:
> > > On Mon 06-11-17 13:00:25, Peter Zijlstra wrote:
> > > > On Mon, Nov 06, 2017 at 11:43:54AM +0100, Michal Hocko wrote:
> > > > > > Yes the comment is very much accurate.
> > > > > 
> > > > > Which suggests that print_vma_addr might be problematic, right?
> > > > > Shouldn't we do trylock on mmap_sem instead?
> > > > 
> > > > Yes that's complete rubbish. trylock will get spurious failures to print
> > > > when the lock is contended.
> > > 
> > > Yes, but I guess that it is acceptable to to not print the state under
> > > that condition.
> > 
> > So what do you think about this? I think this is more robust than
> > playing tricks with the explicit preempt count checks and less tedious
> > than checking to make it conditional on the context. This is on top of
> > Linus tree and if accepted it should replace the patch discussed here.
> > ---
> >  From 0de6d57cbc54ee2686d1f1e4ffcc4ed490ded8aa Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Mon, 6 Nov 2017 14:31:20 +0100
> > Subject: [PATCH] mm: do not rely on preempt_count in print_vma_addr
> > 
> > The preempt count check on print_vma_addr has been added by e8bff74afbdb
> > ("x86: fix "BUG: sleeping function called from invalid context" in
> > print_vma_addr()") and it relied on the elevated preempt count from
> > preempt_conditional_sti because preempt_count check doesn't work on
> > non preemptive kernels by default. The code has evolved though and
> > d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag
> > handling") has replaced preempt_conditional_sti by an explicit
> > preempt_disable which is noop on !PREEMPT so the check in print_vma_addr
> > is broken.
> > 
> > Fix the issue by using trylock on mmap_sem rather than chacking the
> 
> s/chacking/checking

ups, fixed

> > preempt count. The allocation we are relying on has to be GFP_NOWAIT
> > as well. There is a chance that we won't dump the vma state if the lock
> > is contended or the memory short but this is acceptable outcome and much
> > less fragile than the not working preemption check or tricks around it.
> > 
> > Fixes: d99e1bd175f4 ("x86/entry/traps: Refactor preemption and interrupt flag handling")
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Acked-by: Yang Shi <yang.s@alibaba-inc.com>

Thanks!
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

end of thread, other threads:[~2017-11-06 16:24 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-01 21:38 [PATCH] mm: use in_atomic() in print_vma_addr() Yang Shi
2017-11-01 21:38 ` Yang Shi
2017-11-02  7:57 ` Michal Hocko
2017-11-02  7:57   ` Michal Hocko
2017-11-02 17:44   ` Yang Shi
2017-11-02 17:44     ` Yang Shi
2017-11-03  8:29     ` Michal Hocko
2017-11-03  8:29       ` Michal Hocko
2017-11-03 18:02     ` Andrew Morton
2017-11-03 18:02       ` Andrew Morton
2017-11-03 18:16       ` Yang Shi
2017-11-03 18:16         ` Yang Shi
2017-11-03 20:09       ` Bart Van Assche
2017-11-03 20:09         ` Bart Van Assche
2017-11-05  8:19         ` Michal Hocko
2017-11-05  8:19           ` Michal Hocko
2017-11-06 10:05           ` Peter Zijlstra
2017-11-06 10:05             ` Peter Zijlstra
2017-11-06 10:43             ` Michal Hocko
2017-11-06 10:43               ` Michal Hocko
2017-11-06 10:56               ` Michal Hocko
2017-11-06 10:56                 ` Michal Hocko
2017-11-06 12:00               ` Peter Zijlstra
2017-11-06 12:00                 ` Peter Zijlstra
2017-11-06 12:12                 ` Michal Hocko
2017-11-06 12:12                   ` Michal Hocko
2017-11-06 13:40                   ` [PATCH] mm: do not rely on preempt_count in print_vma_addr (was: Re: [PATCH] mm: use in_atomic() in print_vma_addr()) Michal Hocko
2017-11-06 13:40                     ` Michal Hocko
2017-11-06 14:19                     ` [PATCH] mm: do not rely on preempt_count in print_vma_addr Vlastimil Babka
2017-11-06 14:19                       ` Vlastimil Babka
2017-11-06 14:28                       ` Michal Hocko
2017-11-06 14:28                         ` Michal Hocko
2017-11-06 16:16                     ` Yang Shi
2017-11-06 16:16                       ` Yang Shi
2017-11-06 16:24                       ` Michal Hocko
2017-11-06 16:24                         ` Michal Hocko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.