Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1660211 > unrolled thread
| Started by | David Rientjes <rientjes@google.com> |
|---|---|
| First post | 2017-06-07 23:00 +0200 |
| Last post | 2017-06-08 17:30 +0200 |
| Articles | 16 — 5 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: Sleeping BUG in khugepaged for i586 David Rientjes <rientjes@google.com> - 2017-06-07 23:00 +0200
Re: Sleeping BUG in khugepaged for i586 Michal Hocko <mhocko@kernel.org> - 2017-06-08 16:50 +0200
Re: Sleeping BUG in khugepaged for i586 Matthew Wilcox <willy@infradead.org> - 2017-06-08 19:10 +0200
Re: Sleeping BUG in khugepaged for i586 Michal Hocko <mhocko@kernel.org> - 2017-06-08 22:20 +0200
Re: Sleeping BUG in khugepaged for i586 Michal Hocko <mhocko@kernel.org> - 2017-06-08 22:40 +0200
Re: Sleeping BUG in khugepaged for i586 Vlastimil Babka <vbabka@suse.cz> - 2017-06-09 08:50 +0200
Re: Sleeping BUG in khugepaged for i586 Michal Hocko <mhocko@kernel.org> - 2017-06-09 09:50 +0200
Re: Sleeping BUG in khugepaged for i586 Larry Finger <Larry.Finger@lwfinger.net> - 2017-06-09 16:30 +0200
Re: Sleeping BUG in khugepaged for i586 David Rientjes <rientjes@google.com> - 2017-06-10 00:40 +0200
Re: Sleeping BUG in khugepaged for i586 Michal Hocko <mhocko@kernel.org> - 2017-06-10 10:10 +0200
Re: Sleeping BUG in khugepaged for i586 David Rientjes <rientjes@google.com> - 2017-06-12 01:30 +0200
Re: Sleeping BUG in khugepaged for i586 Michal Hocko <mhocko@kernel.org> - 2017-06-12 08:30 +0200
Re: Sleeping BUG in khugepaged for i586 David Rientjes <rientjes@google.com> - 2017-06-15 02:30 +0200
Re: Sleeping BUG in khugepaged for i586 David Rientjes <rientjes@google.com> - 2017-06-15 03:20 +0200
Re: Sleeping BUG in khugepaged for i586 Michal Hocko <mhocko@kernel.org> - 2017-06-15 10:40 +0200
Re: Sleeping BUG in khugepaged for i586 Larry Finger <Larry.Finger@lwfinger.net> - 2017-06-08 17:30 +0200
| From | David Rientjes <rientjes@google.com> |
|---|---|
| Date | 2017-06-07 23:00 +0200 |
| Subject | Re: Sleeping BUG in khugepaged for i586 |
| Message-ID | <tPQdA-38j-17@gated-at.bofh.it> |
On Wed, 7 Jun 2017, Vlastimil Babka wrote: > >> Hmm I'd expect such spin lock to be reported together with mmap_sem in > >> the debugging "locks held" message? > > > > My bisection of the problem is about half done. My latest good version is commit > > 7b8cd33 and the latest bad one is 2ea659a. Only about 7 steps to go. > > Hmm, your bisection will most likely just find commit 338a16ba15495 > which added the cond_resched() at mm/khugepaged.c:655. CCing David who > added it. > I agree it's probably going to bisect to 338a16ba15495 since it's the cond_resched() at the line number reported, but I think there must be something else going on. I think the list of locks held by khugepaged is correct because it matches with the implementation. The preempt_count(), as suggested by Andrew, does not. If this is reproducible, I'd like to know what preempt_count() is.
[toc] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-08 16:50 +0200 |
| Message-ID | <tQ6V4-5vT-13@gated-at.bofh.it> |
| In reply to | #1660211 |
On Wed 07-06-17 13:56:01, David Rientjes wrote:
> On Wed, 7 Jun 2017, Vlastimil Babka wrote:
>
> > >> Hmm I'd expect such spin lock to be reported together with mmap_sem in
> > >> the debugging "locks held" message?
> > >
> > > My bisection of the problem is about half done. My latest good version is commit
> > > 7b8cd33 and the latest bad one is 2ea659a. Only about 7 steps to go.
> >
> > Hmm, your bisection will most likely just find commit 338a16ba15495
> > which added the cond_resched() at mm/khugepaged.c:655. CCing David who
> > added it.
> >
>
> I agree it's probably going to bisect to 338a16ba15495 since it's the
> cond_resched() at the line number reported, but I think there must be
> something else going on. I think the list of locks held by khugepaged is
> correct because it matches with the implementation. The preempt_count(),
> as suggested by Andrew, does not. If this is reproducible, I'd like to
> know what preempt_count() is.
collapse_huge_page
pte_offset_map
kmap_atomic
kmap_atomic_prot
preempt_disable
__collapse_huge_page_copy
pte_unmap
kunmap_atomic
__kunmap_atomic
preempt_enable
I suspect, so cond_resched seems indeed inappropriate on 32b systems.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Matthew Wilcox <willy@infradead.org> |
|---|---|
| Date | 2017-06-08 19:10 +0200 |
| Message-ID | <tQ96y-73e-17@gated-at.bofh.it> |
| In reply to | #1661348 |
On Thu, Jun 08, 2017 at 04:48:31PM +0200, Michal Hocko wrote:
> On Wed 07-06-17 13:56:01, David Rientjes wrote:
> > I agree it's probably going to bisect to 338a16ba15495 since it's the
> > cond_resched() at the line number reported, but I think there must be
> > something else going on. I think the list of locks held by khugepaged is
> > correct because it matches with the implementation. The preempt_count(),
> > as suggested by Andrew, does not. If this is reproducible, I'd like to
> > know what preempt_count() is.
>
> collapse_huge_page
> pte_offset_map
> kmap_atomic
> kmap_atomic_prot
> preempt_disable
> __collapse_huge_page_copy
> pte_unmap
> kunmap_atomic
> __kunmap_atomic
> preempt_enable
>
> I suspect, so cond_resched seems indeed inappropriate on 32b systems.
Then why doesn't it trigger on 64-bit systems too?
#ifndef ARCH_HAS_KMAP
...
static inline void *kmap_atomic(struct page *page)
{
preempt_disable();
pagefault_disable();
return page_address(page);
}
#define kmap_atomic_prot(page, prot) kmap_atomic(page)
... oh, wait, I see. Because pte_offset_map() doesn't call kmap_atomic()
on 64-bit. Indeed, it doesn't necessarily call kmap_atomic() on 32-bit
either; only with CONFIG_HIGHPTE enabled. How much of a performance
penalty would it be to call kmap_atomic() unconditionally on 64 bit to
make sure that this kind of problem doesn't show on 32-bit systems only?
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-08 22:20 +0200 |
| Message-ID | <tQc4q-sN-21@gated-at.bofh.it> |
| In reply to | #1661499 |
On Thu 08-06-17 10:05:57, Matthew Wilcox wrote:
> On Thu, Jun 08, 2017 at 04:48:31PM +0200, Michal Hocko wrote:
> > On Wed 07-06-17 13:56:01, David Rientjes wrote:
> > > I agree it's probably going to bisect to 338a16ba15495 since it's the
> > > cond_resched() at the line number reported, but I think there must be
> > > something else going on. I think the list of locks held by khugepaged is
> > > correct because it matches with the implementation. The preempt_count(),
> > > as suggested by Andrew, does not. If this is reproducible, I'd like to
> > > know what preempt_count() is.
> >
> > collapse_huge_page
> > pte_offset_map
> > kmap_atomic
> > kmap_atomic_prot
> > preempt_disable
> > __collapse_huge_page_copy
> > pte_unmap
> > kunmap_atomic
> > __kunmap_atomic
> > preempt_enable
> >
> > I suspect, so cond_resched seems indeed inappropriate on 32b systems.
>
> Then why doesn't it trigger on 64-bit systems too?
>
> #ifndef ARCH_HAS_KMAP
> ...
> static inline void *kmap_atomic(struct page *page)
> {
> preempt_disable();
> pagefault_disable();
> return page_address(page);
> }
> #define kmap_atomic_prot(page, prot) kmap_atomic(page)
>
>
> ... oh, wait, I see. Because pte_offset_map() doesn't call kmap_atomic()
> on 64-bit. Indeed, it doesn't necessarily call kmap_atomic() on 32-bit
> either; only with CONFIG_HIGHPTE enabled. How much of a performance
> penalty would it be to call kmap_atomic() unconditionally on 64 bit to
> make sure that this kind of problem doesn't show on 32-bit systems only?
I am not sure I understand why would we map those pages in 64b systems?
We can access them directly.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-08 22:40 +0200 |
| Message-ID | <tQcnM-z9-9@gated-at.bofh.it> |
| In reply to | #1661652 |
On Thu 08-06-17 22:18:22, Michal Hocko wrote:
> On Thu 08-06-17 10:05:57, Matthew Wilcox wrote:
> > On Thu, Jun 08, 2017 at 04:48:31PM +0200, Michal Hocko wrote:
> > > On Wed 07-06-17 13:56:01, David Rientjes wrote:
> > > > I agree it's probably going to bisect to 338a16ba15495 since it's the
> > > > cond_resched() at the line number reported, but I think there must be
> > > > something else going on. I think the list of locks held by khugepaged is
> > > > correct because it matches with the implementation. The preempt_count(),
> > > > as suggested by Andrew, does not. If this is reproducible, I'd like to
> > > > know what preempt_count() is.
> > >
> > > collapse_huge_page
> > > pte_offset_map
> > > kmap_atomic
> > > kmap_atomic_prot
> > > preempt_disable
> > > __collapse_huge_page_copy
> > > pte_unmap
> > > kunmap_atomic
> > > __kunmap_atomic
> > > preempt_enable
> > >
> > > I suspect, so cond_resched seems indeed inappropriate on 32b systems.
> >
> > Then why doesn't it trigger on 64-bit systems too?
> >
> > #ifndef ARCH_HAS_KMAP
> > ...
> > static inline void *kmap_atomic(struct page *page)
> > {
> > preempt_disable();
> > pagefault_disable();
> > return page_address(page);
> > }
> > #define kmap_atomic_prot(page, prot) kmap_atomic(page)
> >
> >
> > ... oh, wait, I see. Because pte_offset_map() doesn't call kmap_atomic()
> > on 64-bit. Indeed, it doesn't necessarily call kmap_atomic() on 32-bit
> > either; only with CONFIG_HIGHPTE enabled. How much of a performance
> > penalty would it be to call kmap_atomic() unconditionally on 64 bit to
> > make sure that this kind of problem doesn't show on 32-bit systems only?
>
> I am not sure I understand why would we map those pages in 64b systems?
> We can access them directly.
But I guess you are primary after syncing the preemptive mode for 64 and
32b systems, right? I agree that having a different model is more than
unfortunate because 32b gets much less testing coverage and so a risk of
introducing a new bug is just a matter of time. Maybe we should make
pte_offset_map disable preemption and currently noop pte_unmap to
preempt_enable. The overhead should be pretty marginal on x86_64 but not
all arches have per-cpu preempt count. So I am not sure we really want
to add this to just for the debugging purposes...
I would just pull the cond_resched out of __collapse_huge_page_copy
right after pte_unmap. But I am not really sure why this cond_resched is
really needed because the changelog of the patch which adds is is quite
terse on details.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2017-06-09 08:50 +0200 |
| Message-ID | <tQlU5-6xs-3@gated-at.bofh.it> |
| In reply to | #1661670 |
On 06/08/2017 10:30 PM, Michal Hocko wrote: > But I guess you are primary after syncing the preemptive mode for 64 and > 32b systems, right? I agree that having a different model is more than > unfortunate because 32b gets much less testing coverage and so a risk of > introducing a new bug is just a matter of time. Maybe we should make > pte_offset_map disable preemption and currently noop pte_unmap to > preempt_enable. The overhead should be pretty marginal on x86_64 but not > all arches have per-cpu preempt count. So I am not sure we really want > to add this to just for the debugging purposes... I think adding that overhead for everyone would be unfortunate. It would be acceptable, if it was done only for the config option that enables the might_sleep() checks (CONFIG_DEBUG_ATOMIC_SLEEP?) Vlastimil
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-09 09:50 +0200 |
| Message-ID | <tQmQa-78W-33@gated-at.bofh.it> |
| In reply to | #1661975 |
On Fri 09-06-17 08:48:58, Vlastimil Babka wrote: > On 06/08/2017 10:30 PM, Michal Hocko wrote: > > But I guess you are primary after syncing the preemptive mode for 64 and > > 32b systems, right? I agree that having a different model is more than > > unfortunate because 32b gets much less testing coverage and so a risk of > > introducing a new bug is just a matter of time. Maybe we should make > > pte_offset_map disable preemption and currently noop pte_unmap to > > preempt_enable. The overhead should be pretty marginal on x86_64 but not > > all arches have per-cpu preempt count. So I am not sure we really want > > to add this to just for the debugging purposes... > > I think adding that overhead for everyone would be unfortunate. It would > be acceptable, if it was done only for the config option that enables > the might_sleep() checks (CONFIG_DEBUG_ATOMIC_SLEEP?) That is certainly possible. But is it worth it? arch/alpha/include/asm/pgtable.h:#define pte_offset_map(dir,addr) pte_offset_kernel((dir),(addr)) arch/arc/include/asm/pgtable.h:#define pte_offset_map(dir, addr) pte_offset(dir, addr) arch/arm/include/asm/pgtable.h:#define pte_offset_map(pmd,addr) (__pte_map(pmd) + pte_index(addr)) arch/arm64/include/asm/pgtable.h:#define pte_offset_map(dir,addr) pte_offset_kernel((dir), (addr)) arch/arm64/include/asm/pgtable.h:#define pte_offset_map_nested(dir,addr) pte_offset_kernel((dir), (addr)) arch/cris/include/asm/pgtable.h:#define pte_offset_map(dir, address) \ arch/frv/include/asm/pgtable.h:#define pte_offset_map(dir, address) \ arch/frv/include/asm/pgtable.h:#define pte_offset_map(dir, address) \ arch/hexagon/include/asm/pgtable.h:#define pte_offset_map(dir, address) \ arch/hexagon/include/asm/pgtable.h:#define pte_offset_map_nested(pmd, addr) pte_offset_map(pmd, addr) arch/ia64/include/asm/pgtable.h:#define pte_offset_map(dir,addr) pte_offset_kernel(dir, addr) arch/m32r/include/asm/pgtable.h:#define pte_offset_map(dir, address) \ arch/m68k/include/asm/mcf_pgtable.h:#define pte_offset_map(pmdp, addr) ((pte_t *)__pmd_page(*pmdp) + \ arch/m68k/include/asm/motorola_pgtable.h:#define pte_offset_map(pmdp,address) ((pte_t *)__pmd_page(*pmdp) + (((address) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1))) arch/m68k/include/asm/sun3_pgtable.h:#define pte_offset_map(pmd, address) ((pte_t *)page_address(pmd_page(*pmd)) + pte_index(address)) arch/metag/include/asm/pgtable.h:#define pte_offset_map(dir, address) pte_offset_kernel(dir, address) arch/metag/include/asm/pgtable.h:#define pte_offset_map_nested(dir, address) pte_offset_kernel(dir, address) arch/microblaze/include/asm/pgtable.h:#define pte_offset_map(dir, addr) \ arch/mips/include/asm/pgtable-32.h:#define pte_offset_map(dir, address) \ arch/mips/include/asm/pgtable-64.h:#define pte_offset_map(dir, address) \ arch/mn10300/include/asm/pgtable.h:#define pte_offset_map(dir, address) \ arch/nios2/include/asm/pgtable.h:#define pte_offset_map(dir, addr) \ arch/openrisc/include/asm/pgtable.h:#define pte_offset_map(dir, address) \ arch/openrisc/include/asm/pgtable.h:#define pte_offset_map_nested(dir, address) \ arch/parisc/include/asm/pgtable.h:#define pte_offset_map(pmd, address) pte_offset_kernel(pmd, address) arch/powerpc/include/asm/book3s/32/pgtable.h:#define pte_offset_map(dir, addr) \ arch/powerpc/include/asm/book3s/64/pgtable.h:#define pte_offset_map(dir,addr) pte_offset_kernel((dir), (addr)) arch/powerpc/include/asm/nohash/32/pgtable.h:#define pte_offset_map(dir, addr) \ arch/powerpc/include/asm/nohash/64/pgtable.h:#define pte_offset_map(dir,addr) pte_offset_kernel((dir), (addr)) arch/s390/include/asm/pgtable.h:#define pte_offset_map(pmd, address) pte_offset_kernel(pmd, address) arch/score/include/asm/pgtable.h:#define pte_offset_map(dir, address) \ arch/sh/include/asm/pgtable_32.h:#define pte_offset_map(dir, address) pte_offset_kernel(dir, address) arch/sh/include/asm/pgtable_64.h:#define pte_offset_map(dir,addr) pte_offset_kernel(dir, addr) arch/sparc/include/asm/pgtable_32.h:#define pte_offset_map(d, a) pte_offset_kernel(d,a) arch/sparc/include/asm/pgtable_64.h:#define pte_offset_map pte_index arch/tile/include/asm/pgtable.h:#define pte_offset_map(dir, address) pte_offset_kernel(dir, address) arch/um/include/asm/pgtable.h:#define pte_offset_map(dir, address) \ arch/unicore32/include/asm/pgtable.h:#define pte_offset_map(dir, addr) (pmd_page_vaddr(*(dir)) \ arch/x86/include/asm/pgtable_32.h:#define pte_offset_map(dir, address) \ arch/x86/include/asm/pgtable_32.h:#define pte_offset_map(dir, address) \ arch/x86/include/asm/pgtable_64.h:#define pte_offset_map(dir, address) pte_offset_kernel((dir), (address)) arch/xtensa/include/asm/pgtable.h:#define pte_offset_map(dir,addr) pte_offset_kernel((dir),(addr)) include/linux/mm.h:#define pte_offset_map_lock(mm, pmd, address, ptlp) \ -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Larry Finger <Larry.Finger@lwfinger.net> |
|---|---|
| Date | 2017-06-09 16:30 +0200 |
| Message-ID | <tQt5h-2GI-55@gated-at.bofh.it> |
| In reply to | #1661975 |
On 06/09/2017 01:48 AM, Vlastimil Babka wrote: > On 06/08/2017 10:30 PM, Michal Hocko wrote: >> But I guess you are primary after syncing the preemptive mode for 64 and >> 32b systems, right? I agree that having a different model is more than >> unfortunate because 32b gets much less testing coverage and so a risk of >> introducing a new bug is just a matter of time. Maybe we should make >> pte_offset_map disable preemption and currently noop pte_unmap to >> preempt_enable. The overhead should be pretty marginal on x86_64 but not >> all arches have per-cpu preempt count. So I am not sure we really want >> to add this to just for the debugging purposes... > > I think adding that overhead for everyone would be unfortunate. It would > be acceptable, if it was done only for the config option that enables > the might_sleep() checks (CONFIG_DEBUG_ATOMIC_SLEEP?) As a "heads up", I will not be available for any testing from June 10 through June 17. Larry
[toc] | [prev] | [next] | [standalone]
| From | David Rientjes <rientjes@google.com> |
|---|---|
| Date | 2017-06-10 00:40 +0200 |
| Message-ID | <tQAJs-7ia-21@gated-at.bofh.it> |
| In reply to | #1661670 |
On Thu, 8 Jun 2017, Michal Hocko wrote: > I would just pull the cond_resched out of __collapse_huge_page_copy > right after pte_unmap. But I am not really sure why this cond_resched is > really needed because the changelog of the patch which adds is is quite > terse on details. I'm not sure what could possibly be added to the changelog. We have encountered need_resched warnings during the iteration. We fix these because need_resched warnings suppress future warnings of the same type for issues that are more important. I can fix the i386 issue but removing the cond_resched() entirely isn't really suitable.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-10 10:10 +0200 |
| Message-ID | <tQJD4-4zr-9@gated-at.bofh.it> |
| In reply to | #1662795 |
On Fri 09-06-17 15:38:44, David Rientjes wrote: > On Thu, 8 Jun 2017, Michal Hocko wrote: > > > I would just pull the cond_resched out of __collapse_huge_page_copy > > right after pte_unmap. But I am not really sure why this cond_resched is > > really needed because the changelog of the patch which adds is is quite > > terse on details. > > I'm not sure what could possibly be added to the changelog. We have > encountered need_resched warnings during the iteration. Well, the part the changelog is not really clear about is whether the HPAGE_PMD_NR loops itself is the source of the stall. This would be quite surprising because doing 512 iterations taking up to 20+s sounds way to much. So is it possible that we are missing a cond_resched somewhere up the __collapse_huge_page_copy call path? Or do we really do something stupidly expensive here? > We fix these > because need_resched warnings suppress future warnings of the same type > for issues that are more important. Sure thing. I do care about soft lockups as well. > I can fix the i386 issue but removing the cond_resched() entirely isn't > really suitable. I am not calling for a complete removal. I just do not yet see what is the source of the long processing of the the loop. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | David Rientjes <rientjes@google.com> |
|---|---|
| Date | 2017-06-12 01:30 +0200 |
| Message-ID | <tRksV-2Q5-3@gated-at.bofh.it> |
| In reply to | #1662900 |
On Sat, 10 Jun 2017, Michal Hocko wrote:
> > > I would just pull the cond_resched out of __collapse_huge_page_copy
> > > right after pte_unmap. But I am not really sure why this cond_resched is
> > > really needed because the changelog of the patch which adds is is quite
> > > terse on details.
> >
> > I'm not sure what could possibly be added to the changelog. We have
> > encountered need_resched warnings during the iteration.
>
> Well, the part the changelog is not really clear about is whether the
> HPAGE_PMD_NR loops itself is the source of the stall. This would be
> quite surprising because doing 512 iterations taking up to 20+s sounds
> way to much.
I have no idea where you come up with 20+ seconds.
These are not soft lockups, these are need_resched warnings. We monitor
how long need_resched has been set and when a thread takes an excessive
amount of time to reschedule after it has been set. A loop of 512 pages
with ptl contention and doing {clear,copy}_user_highpage() shows that
need_resched can sit without scheduling for an excessive amount of time.
> So is it possible that we are missing a cond_resched
> somewhere up the __collapse_huge_page_copy call path?
No.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-12 08:30 +0200 |
| Message-ID | <tRr1n-6ZK-5@gated-at.bofh.it> |
| In reply to | #1663165 |
On Sun 11-06-17 16:28:11, David Rientjes wrote:
> On Sat, 10 Jun 2017, Michal Hocko wrote:
>
> > > > I would just pull the cond_resched out of __collapse_huge_page_copy
> > > > right after pte_unmap. But I am not really sure why this cond_resched is
> > > > really needed because the changelog of the patch which adds is is quite
> > > > terse on details.
> > >
> > > I'm not sure what could possibly be added to the changelog. We have
> > > encountered need_resched warnings during the iteration.
> >
> > Well, the part the changelog is not really clear about is whether the
> > HPAGE_PMD_NR loops itself is the source of the stall. This would be
> > quite surprising because doing 512 iterations taking up to 20+s sounds
> > way to much.
>
> I have no idea where you come up with 20+ seconds.
OK, I misread your report as a soft lockup.
> These are not soft lockups, these are need_resched warnings. We monitor
> how long need_resched has been set and when a thread takes an excessive
> amount of time to reschedule after it has been set. A loop of 512 pages
> with ptl contention and doing {clear,copy}_user_highpage() shows that
> need_resched can sit without scheduling for an excessive amount of time.
How much is excessive here?
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | David Rientjes <rientjes@google.com> |
|---|---|
| Date | 2017-06-15 02:30 +0200 |
| Message-ID | <tSqPE-3IL-7@gated-at.bofh.it> |
| In reply to | #1663263 |
On Mon, 12 Jun 2017, Michal Hocko wrote:
> > These are not soft lockups, these are need_resched warnings. We monitor
> > how long need_resched has been set and when a thread takes an excessive
> > amount of time to reschedule after it has been set. A loop of 512 pages
> > with ptl contention and doing {clear,copy}_user_highpage() shows that
> > need_resched can sit without scheduling for an excessive amount of time.
>
> How much is excessive here?
We monitor anything that holds the cpu for more than 1/20th of a second,
but this specific occurrence has been observed for ~1/8th. The majority
of mm/ is quite good in this regard.
[toc] | [prev] | [next] | [standalone]
| From | David Rientjes <rientjes@google.com> |
|---|---|
| Date | 2017-06-15 03:20 +0200 |
| Message-ID | <tSrC1-4gc-7@gated-at.bofh.it> |
| In reply to | #1661348 |
On Thu, 8 Jun 2017, Michal Hocko wrote: > collapse_huge_page > pte_offset_map > kmap_atomic > kmap_atomic_prot > preempt_disable > __collapse_huge_page_copy > pte_unmap > kunmap_atomic > __kunmap_atomic > preempt_enable > > I suspect, so cond_resched seems indeed inappropriate on 32b systems. > Seems to be an issue for i386 and arm with ARM_LPAE. I'm slightly surprised we can get away with __collapse_huge_page_swapin() for VM_FAULT_RETRY, unless that hasn't been encountered yet. I think the cond_resched() in __collapse_huge_page_copy() could be done only for !in_atomic() if we choose.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-15 10:40 +0200 |
| Message-ID | <tSytQ-8X-11@gated-at.bofh.it> |
| In reply to | #1666350 |
On Wed 14-06-17 18:12:06, David Rientjes wrote: > On Thu, 8 Jun 2017, Michal Hocko wrote: > > > collapse_huge_page > > pte_offset_map > > kmap_atomic > > kmap_atomic_prot > > preempt_disable > > __collapse_huge_page_copy > > pte_unmap > > kunmap_atomic > > __kunmap_atomic > > preempt_enable > > > > I suspect, so cond_resched seems indeed inappropriate on 32b systems. > > > > Seems to be an issue for i386 and arm with ARM_LPAE. I'm slightly > surprised we can get away with __collapse_huge_page_swapin() for > VM_FAULT_RETRY, unless that hasn't been encountered yet. I do not see what you mean here or how is it related. __collapse_huge_page_swapin is called outside of pte_offset_map/pte_unmap section > I think the cond_resched() in __collapse_huge_page_copy() could be > done only for !in_atomic() if we choose. in_atomic() depends on having PREEMPT_COUNT enabled to work properly AFAIR. I haven't double checked and something might have changed since I've looked the last time. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Larry Finger <Larry.Finger@lwfinger.net> |
|---|---|
| Date | 2017-06-08 17:30 +0200 |
| Message-ID | <tQ7xN-60p-45@gated-at.bofh.it> |
| In reply to | #1660211 |
On 06/07/2017 03:56 PM, David Rientjes wrote: > On Wed, 7 Jun 2017, Vlastimil Babka wrote: > >>>> Hmm I'd expect such spin lock to be reported together with mmap_sem in >>>> the debugging "locks held" message? >>> >>> My bisection of the problem is about half done. My latest good version is commit >>> 7b8cd33 and the latest bad one is 2ea659a. Only about 7 steps to go. >> >> Hmm, your bisection will most likely just find commit 338a16ba15495 >> which added the cond_resched() at mm/khugepaged.c:655. CCing David who >> added it. >> > > I agree it's probably going to bisect to 338a16ba15495 since it's the > cond_resched() at the line number reported, but I think there must be > something else going on. I think the list of locks held by khugepaged is > correct because it matches with the implementation. The preempt_count(), > as suggested by Andrew, does not. If this is reproducible, I'd like to > know what preempt_count() is. > The BUG output is reproducible. By the time the box finishes booting, there are at least 2 of them logged. My bisection shows that commit 338a16ba15495 is the bad one. I added a pr_info() to output the value of preempt_count() just before the cond_resched() statement. The count was always 1 whether the BUG was triggered or not. If there are other things you would like logged at that point, or any other diagnostics, please let me know. Larry
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web