[PATCH] mm: use in_atomic() in print_vma_addr()

[PATCH] mm: use in_atomic() in print_vma_addr()

[Yang Shi]

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

[PATCH] mm: use in_atomic() in print_vma_addr()

[Yang Shi]

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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

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

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Yang Shi]

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
> 

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Yang Shi]

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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

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

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Andrew Morton]

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?

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Andrew Morton]

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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Yang Shi]

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
> 

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Yang Shi]

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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Bart Van Assche]

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.

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Bart Van Assche]

[-- 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†·Ÿ

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

[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

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

[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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Peter Zijlstra]

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.

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Peter Zijlstra]

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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

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

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

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

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Peter Zijlstra]

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.

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Peter Zijlstra]

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>

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

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

Re: [PATCH] mm: use in_atomic() in print_vma_addr()

[Michal Hocko]

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>

[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]

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

[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]

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.
---

Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr

[Vlastimil Babka]

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;
>  
> 

Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr

[Vlastimil Babka]

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>

Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr

[Michal Hocko]

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

Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr

[Michal Hocko]

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>

Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr

[Yang Shi]

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;
>   
> 

Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr

[Yang Shi]

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>

Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr

[Michal Hocko]

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

Re: [PATCH] mm: do not rely on preempt_count in print_vma_addr

[Michal Hocko]

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>