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

Groups > linux.kernel > #1720049 > unrolled thread

Re: kvm splat in mmu_spte_clear_track_bits

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.

Contents

  Re: kvm splat in mmu_spte_clear_track_bits Adam Borowski <kilobyte@angband.pl> - 2017-08-25 15:20 +0200
    Re: kvm splat in mmu_spte_clear_track_bits Paolo Bonzini <pbonzini@redhat.com> - 2017-08-25 15:50 +0200
      Re: kvm splat in mmu_spte_clear_track_bits Adam Borowski <kilobyte@angband.pl> - 2017-08-27 14:40 +0200
        Re: kvm splat in mmu_spte_clear_track_bits Bernhard Held <berny156@gmx.de> - 2017-08-28 17:30 +0200
          Re: kvm splat in mmu_spte_clear_track_bits Bernhard Held <berny156@gmx.de> - 2017-08-28 18:10 +0200
            Re: kvm splat in mmu_spte_clear_track_bits Takashi Iwai <tiwai@suse.de> - 2017-08-28 18:20 +0200
          Re: kvm splat in mmu_spte_clear_track_bits Takashi Iwai <tiwai@suse.de> - 2017-08-28 18:10 +0200
          Re: kvm splat in mmu_spte_clear_track_bits Nadav Amit <nadav.amit@gmail.com> - 2017-08-28 19:00 +0200
            Re: kvm splat in mmu_spte_clear_track_bits Bernhard Held <berny156@gmx.de> - 2017-08-29 11:20 +0200
              Re: kvm splat in mmu_spte_clear_track_bits Adam Borowski <kilobyte@angband.pl> - 2017-08-29 15:10 +0200
                Re: kvm splat in mmu_spte_clear_track_bits Andrea Arcangeli <aarcange@redhat.com> - 2017-08-29 16:10 +0200
                  Re: kvm splat in mmu_spte_clear_track_bits Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-29 18:20 +0200
                    Re: kvm splat in mmu_spte_clear_track_bits Jerome Glisse <jglisse@redhat.com> - 2017-08-29 20:30 +0200
                  Re: kvm splat in mmu_spte_clear_track_bits Jerome Glisse <jglisse@redhat.com> - 2017-08-29 20:40 +0200
                    Re: kvm splat in mmu_spte_clear_track_bits Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-29 21:10 +0200
                      Re: kvm splat in mmu_spte_clear_track_bits Jerome Glisse <jglisse@redhat.com> - 2017-08-29 21:20 +0200
                        Re: kvm splat in mmu_spte_clear_track_bits Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-29 21:40 +0200
                          Re: kvm splat in mmu_spte_clear_track_bits Andrea Arcangeli <aarcange@redhat.com> - 2017-08-29 22:50 +0200
                            Re: kvm splat in mmu_spte_clear_track_bits Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-29 23:00 +0200
                  Re: kvm splat in mmu_spte_clear_track_bits Michal Hocko <mhocko@kernel.org> - 2017-08-30 10:20 +0200
              Re: kvm splat in mmu_spte_clear_track_bits Nadav Amit <nadav.amit@gmail.com> - 2017-08-29 18:00 +0200
            Re: kvm splat in mmu_spte_clear_track_bits Mike Galbraith <efault@gmx.de> - 2017-08-29 15:00 +0200

Page 1 of 2  [1] 2  Next page →

#1720049 — Re: kvm splat in mmu_spte_clear_track_bits

On Thu, Aug 24, 2017 at 03:43:55PM +0800, Wanpeng Li wrote:
> 2017-08-23 20:22 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> > On 22/08/2017 00:32, Adam Borowski wrote:
> >> On Mon, Aug 21, 2017 at 09:58:34PM +0200, Radim Krčmář wrote:
> >>> 2017-08-21 21:12+0200, Adam Borowski:
> >>>> Also, it doesn't reproduce for me on 4.12.
> >>>
> >>> Great info ... the most suspicious between v4.12 and v4.13-rc5 is the
> >>> series with dcdca5fed5f6 ("x86: kvm: mmu: make spte mmio mask more
> >>> explicit"), does reverting it help?
> >>>
> >>> `git revert ce00053b1cfca312c22e2a6465451f1862561eab~1..995f00a619584e65e53eff372d9b73b121a7bad5`
> >>
> >> Alas, doesn't seem to help.
> >>
> >> I've first installed a Debian stretch guest, the host survived both the
> >> installation and subsequent fooling around.  But then I started a win10
> >> guest which splatted as soon as the initial screen.
> >
> > Can you check if disabling THP on the host also fixes it for you?

As in: ?
echo never >/sys/kernel/mm/transparent_hugepage/enabled
echo never >/sys/kernel/mm/transparent_hugepage/defrag
Still reproduces, with or without reverting
ce00053b1cfca312c22e2a6465451f1862561eab~1..995f00a619584e65e53eff372d9b73b121a7bad5

> > I would also try commit 1372324b328cd5dabaef5e345e37ad48c63df2a9 to
> > identify whether it was caused by a KVM change in 4.13 or something
> > else.

I've ran different guests for a couple of hours, no explosions.  Thus it
looks like updating Cornelia's email address isn't the cause.

Too bad, there's 15k commits between 1372324b and 7f680d7ec315.

> For the OOM testcase, the splat will disappear if disabling THP.


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢰⠒⠀⣿⡁ Vat kind uf sufficiently advanced technology iz dis!?
⢿⡄⠘⠷⠚⠋⠀                                 -- Genghis Ht'rok'din
⠈⠳⣄⠀⠀⠀⠀ 

[toc] | [next] | [standalone]

#1720076

On 25/08/2017 15:14, Adam Borowski wrote:
>>> I would also try commit 1372324b328cd5dabaef5e345e37ad48c63df2a9 to
>>> identify whether it was caused by a KVM change in 4.13 or something
>>> else.
> I've ran different guests for a couple of hours, no explosions.  Thus it
> looks like updating Cornelia's email address isn't the cause.
> 
> Too bad, there's 15k commits between 1372324b and 7f680d7ec315.

So:

- 4.12 works

- 1372324b328cd5dabaef5e345e37ad48c63df2a9 works

- 4.13-rc6 fails

The next ones to test are going to be
c136b84393d4e340e1b53fc7f737dd5827b19ee5 and 4.13-rc1.

This can be a starting point for bisection or pointing the finger at
someone else.

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

#1720812

On Fri, Aug 25, 2017 at 03:40:50PM +0200, Paolo Bonzini wrote:
> On 25/08/2017 15:14, Adam Borowski wrote:
> >>> I would also try commit 1372324b328cd5dabaef5e345e37ad48c63df2a9 to
> >>> identify whether it was caused by a KVM change in 4.13 or something
> >>> else.
> > I've ran different guests for a couple of hours, no explosions.  Thus it
> > looks like updating Cornelia's email address isn't the cause.
> > 
> > Too bad, there's 15k commits between 1372324b and 7f680d7ec315.
> 
> So:
> 
> - 4.12 works
> 
> - 1372324b328cd5dabaef5e345e37ad48c63df2a9 works
> 
> - 4.13-rc6 fails
> 
> The next ones to test are going to be
> c136b84393d4e340e1b53fc7f737dd5827b19ee5 and 4.13-rc1.

c136b84393d4e340e1b53fc7f737dd5827b19ee5 works

4.13-rc1 works
Which actually isn't so surprising, as I pull from linus/master every Sunday
night/Monday morning, so I'd would have noticed KVM breakage sooner.

4.13-rc4 works

4.13-rc5 works (but ↓)

v4.13-rc5-173-g58d4e450a490 works... uh oh.
It shouldn't -- it's merge-base between mainline and what I was running on
the initial crash, and I'm sure anything non-mainline I had isn't the
culprit.  After a couple of hours of running various loads inside and
outside KVM, I finally got it to crash.

4.13-rc5 retested fails
Crashed only after two hours or so of testing.

4.13-rc4 apparently works
It survived several hours of varied tests (like 5 debian-installer runs, a
win10 point release upgrade, some hurd package building, openbsd, etc),
all while the host was likewise busy.


Thus: to the best of my knowledge, the problem is between 4.13-rc4 and 4.13-rc5
but I wouldn't bet my life on it.


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢰⠒⠀⣿⡁ Vat kind uf sufficiently advanced technology iz dis!?
⢿⡄⠘⠷⠚⠋⠀                                 -- Genghis Ht'rok'din
⠈⠳⣄⠀⠀⠀⠀ 

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

#1721744

On 08/27/2017 at 02:35 PM, Adam Borowski wrote:
> 4.13-rc5 retested fails
> Crashed only after two hours or so of testing.
> 
> 4.13-rc4 apparently works
> It survived several hours of varied tests (like 5 debian-installer runs, a
> win10 point release upgrade, some hurd package building, openbsd, etc),
> all while the host was likewise busy.
> 
> Thus: to the best of my knowledge, the problem is between 4.13-rc4 and 4.13-rc5
> but I wouldn't bet my life on it.

I get crashes with Win10 in kvm with 4.13-rc5. 4.13-rc4 works for me. THP seems to accelerate the crash, but that's not 100% sure.

There's still no crash after reverting merge 27df70 on 4.13-rc7. There are 21 commits in this merge, 10 are mm-related:

$ git log 4e082e9ba7cd..e86b298bebf7 --pretty=oneline --abbrev-commit
e86b298bebf7 userfaultfd: replace ENOSPC with ESRCH in case mm has gone during copy/zeropage
f357e345eef7 zram: rework copy of compressor name in comp_algorithm_store()
aac2fea94f7a rmap: do not call mmu_notifier_invalidate_page() under ptl
d041353dc98a mm: fix list corruptions on shmem shrinklist
af54aed94bf3 mm/balloon_compaction.c: don't zero ballooned pages
c0a6a5ae6b5d MAINTAINERS: copy virtio on balloon_compaction.c
b3a81d0841a9 mm: fix KSM data corruption
99baac21e458 mm: fix MADV_[FREE|DONTNEED] TLB flush miss problem
0a2dd266dd6b mm: make tlb_flush_pending global
56236a59556c mm: refactor TLB gathering API
a9b802500ebb Revert "mm: numa: defer TLB flush for THP migration as long as possible"
0a2c40487f3e mm: migrate: fix barriers around tlb_flush_pending
16af97dc5a89 mm: migrate: prevent racy access to tlb_flush_pending
9eeb52ae712e fault-inject: fix wrong should_fail() decision in task context
4e98ebe5f435 test_kmod: fix small memory leak on filesystem tests
9c56771316ef test_kmod: fix the lock in register_test_dev_kmod()
434b06ae23ba test_kmod: fix bug which allows negative values on two config options
a4afe8cdec16 test_kmod: fix spelling mistake: "EMTPY" -> "EMPTY"
5af10dfd0afc userfaultfd: hugetlbfs: remove superfluous page unlock in VM_SHARED case
75dddef32514 mm: ratelimit PFNs busy info message
d507e2ebd2c7 mm: fix global NR_SLAB_.*CLAIMABLE counter reads

Any hint on what to test first is welcome!

Bernhard

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

#1721774

On 08/28/2017 at 06:01 PM, Takashi Iwai wrote:
> On Mon, 28 Aug 2017 17:26:05 +0200,
> Bernhard Held wrote:
>> I get crashes with Win10 in kvm
>>
> Did you get the crash reliably?
> I've been struggling how to trigger it efficiently, but currently in
> vain.  The memory pressure isn't a single key to trigger it, as it
> seems...

Yes, I get the crash pretty reliable with Win10 in kvm after 10 to 30 minutes. Some workload in Windows seems to be necessary to trigger the bug.

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

#1721782

On Mon, 28 Aug 2017 18:07:37 +0200,
Bernhard Held wrote:
> 
> On 08/28/2017 at 06:01 PM, Takashi Iwai wrote:
> > On Mon, 28 Aug 2017 17:26:05 +0200,
> > Bernhard Held wrote:
> >> I get crashes with Win10 in kvm
> >>
> > Did you get the crash reliably?
> > I've been struggling how to trigger it efficiently, but currently in
> > vain.  The memory pressure isn't a single key to trigger it, as it
> > seems...
> 
> Yes, I get the crash pretty reliable with Win10 in kvm after 10 to 30 minutes. Some workload in Windows seems to be necessary to trigger the bug.

OK, thanks, it's good to know.
Unfortunately I have no Windows on my machine, so it doesn't work for
me ;)  But it implies that rather the workload in VM is likely more
essential than the workload on host.


Takashi

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

#1721781

On Mon, 28 Aug 2017 17:26:05 +0200,
Bernhard Held wrote:
> 
> On 08/27/2017 at 02:35 PM, Adam Borowski wrote:
> > 4.13-rc5 retested fails
> > Crashed only after two hours or so of testing.
> >
> > 4.13-rc4 apparently works
> > It survived several hours of varied tests (like 5 debian-installer runs, a
> > win10 point release upgrade, some hurd package building, openbsd, etc),
> > all while the host was likewise busy.
> >
> > Thus: to the best of my knowledge, the problem is between 4.13-rc4 and 4.13-rc5
> > but I wouldn't bet my life on it.
> 
> I get crashes with Win10 in kvm with 4.13-rc5. 4.13-rc4 works for me. THP seems to accelerate the crash, but that's not 100% sure.
> 
> There's still no crash after reverting merge 27df70 on 4.13-rc7. There are 21 commits in this merge, 10 are mm-related:
> 
> $ git log 4e082e9ba7cd..e86b298bebf7 --pretty=oneline --abbrev-commit
> e86b298bebf7 userfaultfd: replace ENOSPC with ESRCH in case mm has gone during copy/zeropage
> f357e345eef7 zram: rework copy of compressor name in comp_algorithm_store()
> aac2fea94f7a rmap: do not call mmu_notifier_invalidate_page() under ptl
> d041353dc98a mm: fix list corruptions on shmem shrinklist
> af54aed94bf3 mm/balloon_compaction.c: don't zero ballooned pages
> c0a6a5ae6b5d MAINTAINERS: copy virtio on balloon_compaction.c
> b3a81d0841a9 mm: fix KSM data corruption
> 99baac21e458 mm: fix MADV_[FREE|DONTNEED] TLB flush miss problem
> 0a2dd266dd6b mm: make tlb_flush_pending global
> 56236a59556c mm: refactor TLB gathering API
> a9b802500ebb Revert "mm: numa: defer TLB flush for THP migration as long as possible"
> 0a2c40487f3e mm: migrate: fix barriers around tlb_flush_pending
> 16af97dc5a89 mm: migrate: prevent racy access to tlb_flush_pending
> 9eeb52ae712e fault-inject: fix wrong should_fail() decision in task context
> 4e98ebe5f435 test_kmod: fix small memory leak on filesystem tests
> 9c56771316ef test_kmod: fix the lock in register_test_dev_kmod()
> 434b06ae23ba test_kmod: fix bug which allows negative values on two config options
> a4afe8cdec16 test_kmod: fix spelling mistake: "EMTPY" -> "EMPTY"
> 5af10dfd0afc userfaultfd: hugetlbfs: remove superfluous page unlock in VM_SHARED case
> 75dddef32514 mm: ratelimit PFNs busy info message
> d507e2ebd2c7 mm: fix global NR_SLAB_.*CLAIMABLE counter reads
> 
> Any hint on what to test first is welcome!

Did you get the crash reliably?
I've been struggling how to trigger it efficiently, but currently in
vain.  The memory pressure isn't a single key to trigger it, as it
seems...


thanks,

Takashi

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

#1721807

Bernhard Held <berny156@gmx.de> wrote:

> On 08/27/2017 at 02:35 PM, Adam Borowski wrote:
>> 4.13-rc5 retested fails
>> Crashed only after two hours or so of testing.
>> 4.13-rc4 apparently works
>> It survived several hours of varied tests (like 5 debian-installer runs, a
>> win10 point release upgrade, some hurd package building, openbsd, etc),
>> all while the host was likewise busy.
>> Thus: to the best of my knowledge, the problem is between 4.13-rc4 and 4.13-rc5
>> but I wouldn't bet my life on it.
> 
> I get crashes with Win10 in kvm with 4.13-rc5. 4.13-rc4 works for me. THP seems to accelerate the crash, but that's not 100% sure.
> 
> There's still no crash after reverting merge 27df70 on 4.13-rc7. There are 21 commits in this merge, 10 are mm-related:
> 
> $ git log 4e082e9ba7cd..e86b298bebf7 --pretty=oneline --abbrev-commit
> e86b298bebf7 userfaultfd: replace ENOSPC with ESRCH in case mm has gone during copy/zeropage
> f357e345eef7 zram: rework copy of compressor name in comp_algorithm_store()
> aac2fea94f7a rmap: do not call mmu_notifier_invalidate_page() under ptl
> d041353dc98a mm: fix list corruptions on shmem shrinklist
> af54aed94bf3 mm/balloon_compaction.c: don't zero ballooned pages
> c0a6a5ae6b5d MAINTAINERS: copy virtio on balloon_compaction.c
> b3a81d0841a9 mm: fix KSM data corruption
> 99baac21e458 mm: fix MADV_[FREE|DONTNEED] TLB flush miss problem
> 0a2dd266dd6b mm: make tlb_flush_pending global
> 56236a59556c mm: refactor TLB gathering API
> a9b802500ebb Revert "mm: numa: defer TLB flush for THP migration as long as possible"
> 0a2c40487f3e mm: migrate: fix barriers around tlb_flush_pending
> 16af97dc5a89 mm: migrate: prevent racy access to tlb_flush_pending
> 9eeb52ae712e fault-inject: fix wrong should_fail() decision in task context
> 4e98ebe5f435 test_kmod: fix small memory leak on filesystem tests
> 9c56771316ef test_kmod: fix the lock in register_test_dev_kmod()
> 434b06ae23ba test_kmod: fix bug which allows negative values on two config options
> a4afe8cdec16 test_kmod: fix spelling mistake: "EMTPY" -> "EMPTY"
> 5af10dfd0afc userfaultfd: hugetlbfs: remove superfluous page unlock in VM_SHARED case
> 75dddef32514 mm: ratelimit PFNs busy info message
> d507e2ebd2c7 mm: fix global NR_SLAB_.*CLAIMABLE counter reads

Don’t blame me for the TLB stuff... My money is on aac2fea94f7a .

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

#1722259

On 08/28/2017 at 06:56 PM, Nadav Amit wrote:
> Bernhard Held <berny156@gmx.de> wrote:
> 
>> On 08/27/2017 at 02:35 PM, Adam Borowski wrote:
>>> 4.13-rc5 retested fails
>>> Crashed only after two hours or so of testing.
>>> 4.13-rc4 apparently works
>>> It survived several hours of varied tests (like 5 debian-installer runs, a
>>> win10 point release upgrade, some hurd package building, openbsd, etc),
>>> all while the host was likewise busy.
>>> Thus: to the best of my knowledge, the problem is between 4.13-rc4 and 4.13-rc5
>>> but I wouldn't bet my life on it.
>>
>> I get crashes with Win10 in kvm with 4.13-rc5. 4.13-rc4 works for me. THP seems to accelerate the crash, but that's not 100% sure.
>>
>> There's still no crash after reverting merge 27df70 on 4.13-rc7. There are 21 commits in this merge, 10 are mm-related:
>>
>> $ git log 4e082e9ba7cd..e86b298bebf7 --pretty=oneline --abbrev-commit
>> e86b298bebf7 userfaultfd: replace ENOSPC with ESRCH in case mm has gone during copy/zeropage
>> f357e345eef7 zram: rework copy of compressor name in comp_algorithm_store()
>> aac2fea94f7a rmap: do not call mmu_notifier_invalidate_page() under ptl
>> d041353dc98a mm: fix list corruptions on shmem shrinklist
>> af54aed94bf3 mm/balloon_compaction.c: don't zero ballooned pages
>> c0a6a5ae6b5d MAINTAINERS: copy virtio on balloon_compaction.c
>> b3a81d0841a9 mm: fix KSM data corruption
>> 99baac21e458 mm: fix MADV_[FREE|DONTNEED] TLB flush miss problem
>> 0a2dd266dd6b mm: make tlb_flush_pending global
>> 56236a59556c mm: refactor TLB gathering API
>> a9b802500ebb Revert "mm: numa: defer TLB flush for THP migration as long as possible"
>> 0a2c40487f3e mm: migrate: fix barriers around tlb_flush_pending
>> 16af97dc5a89 mm: migrate: prevent racy access to tlb_flush_pending
>> 9eeb52ae712e fault-inject: fix wrong should_fail() decision in task context
>> 4e98ebe5f435 test_kmod: fix small memory leak on filesystem tests
>> 9c56771316ef test_kmod: fix the lock in register_test_dev_kmod()
>> 434b06ae23ba test_kmod: fix bug which allows negative values on two config options
>> a4afe8cdec16 test_kmod: fix spelling mistake: "EMTPY" -> "EMPTY"
>> 5af10dfd0afc userfaultfd: hugetlbfs: remove superfluous page unlock in VM_SHARED case
>> 75dddef32514 mm: ratelimit PFNs busy info message
>> d507e2ebd2c7 mm: fix global NR_SLAB_.*CLAIMABLE counter reads
> 
> Don’t blame me for the TLB stuff... My money is on aac2fea94f7a .

Amit, thanks for your courage to expose your patch!

I'm more and more confident that aac2fea94f7a is the culprit. Maybe it just accelerates the triggering of the splash. To be more sure the kernel needs to be tested for a couple of days. It would be great if others could assist in testing aac2fea94f7a.

Have fun,
Bernhard

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

#1722453

On Tue, Aug 29, 2017 at 02:45:41PM +0200, Takashi Iwai wrote:
> [Put more people to Cc, sorry for growing too much...]

We're all interested in 4.13.0 not crashing on us, so that's ok.

> On Tue, 29 Aug 2017 11:19:13 +0200,
> Bernhard Held wrote:
> > 
> > On 08/28/2017 at 06:56 PM, Nadav Amit wrote:
> > > Don’t blame me for the TLB stuff... My money is on aac2fea94f7a .
> > 
> > Amit, thanks for your courage to expose your patch!
> > 
> > I'm more and more confident that aac2fea94f7a is the culprit.  Maybe it
> > just accelerates the triggering of the splash.  To be more sure the
> > kernel needs to be tested for a couple of days.  It would be great if
> > others could assist in testing aac2fea94f7a.
> 
> I'm testing with the revert for a while and it seems working.

With nothing but aac2fea94f7a reverted, no explosions for me either.

-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢰⠒⠀⣿⡁ Vat kind uf sufficiently advanced technology iz dis!?
⢿⡄⠘⠷⠚⠋⠀                                 -- Genghis Ht'rok'din
⠈⠳⣄⠀⠀⠀⠀ 

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

#1722521

Hello,

On Tue, Aug 29, 2017 at 02:59:23PM +0200, Adam Borowski wrote:
> On Tue, Aug 29, 2017 at 02:45:41PM +0200, Takashi Iwai wrote:
> > [Put more people to Cc, sorry for growing too much...]
> 
> We're all interested in 4.13.0 not crashing on us, so that's ok.
> 
> > On Tue, 29 Aug 2017 11:19:13 +0200,
> > Bernhard Held wrote:
> > > 
> > > On 08/28/2017 at 06:56 PM, Nadav Amit wrote:
> > > > Don’t blame me for the TLB stuff... My money is on aac2fea94f7a .
> > > 
> > > Amit, thanks for your courage to expose your patch!
> > > 
> > > I'm more and more confident that aac2fea94f7a is the culprit.  Maybe it
> > > just accelerates the triggering of the splash.  To be more sure the
> > > kernel needs to be tested for a couple of days.  It would be great if
> > > others could assist in testing aac2fea94f7a.
> > 
> > I'm testing with the revert for a while and it seems working.
> 
> With nothing but aac2fea94f7a reverted, no explosions for me either.

The aforementioned commit has 3 bugs.

1) mmu_notifier_invalidate_range cannot be used in replacement of
   mmu_notifier_invalidate_range_start/end. For KVM
   mmu_notifier_invalidate_range is a noop and rightfully so. A MMU
   notifier implementation has to implement either
   ->invalidate_range method or the invalidate_range_start/end
   methods, not both. And if you implement invalidate_range_start/end
   like KVM is forced to do, calling mmu_notifier_invalidate_range in
   common code is a noop for KVM.

   For those MMU notifiers that can get away only implementing
   ->invalidate_range, the ->invalidate_range is implicitly called by
   mmu_notifier_invalidate_range_end(). And only those secondary MMUs
   that share the same pagetable with the primary MMU (like AMD
   iommuv2) can get away only implementing ->invalidate_range.

   So all cases (THP on/off) are broken right now.

   To fix this is enough to replace mmu_notifier_invalidate_range with
   mmu_notifier_invalidate_range_start;mmu_notifier_invalidate_range_end. Either
   that or call multiple mmu_notifier_invalidate_page like before.

2) address + (1UL << compound_order(page) is buggy, it should be
   PAGE_SIZE << compound_order(page), it's bytes not pages, 2M not 512.

3) The whole invalidate_range thing was an attempt to call a single
   invalidate while walking multiple 4k ptes that maps the same THP
   (after a pmd virtual split without physical compound page THP
   split). It's unclear if the rmap_walk will always provide an
   address that is 2M aligned as parameter to try_to_unmap_one, in
   presence of THP. I think it needs also an address &= (PAGE_SIZE <<
   compound_order(page)) - 1 to be safe.

The other bug where you can reproduce the same corruption with OOM is
unrelated and caused by the OOM reaper. OOM reaper was even corrupting
data if a task was writing to disk and stuck in OOM in write() syscall
or async io write.

To fix the KVM corruption in the OOM reaper, it needs to call
mmu_notifier_invalidate_start/end around
oom_kill.c:unmap_page_range. This additional
mmu_notifier_invalidate_start will not be good for the OOM reaper
because it's yet another case (like the mmap_sem for writing) that
will prevent the OOM reaper to run, so hindering its ability to hide
XFS OOM deadlocks, and making those resurface. Not in KVM case because
we use a spinlock to serialize against the secondary MMU activity and
the KVM critical section under spinlock isn't going to allocate
memory, but range_start can schedule or block on slow hardware where
the secondary MMU is accessed through PCI (not KVM case).

My preference is still to make the OOM reaper a config option and let
it grow into the VM at zero cost if disabled, while at the same time
having the option to keep the VM simpler and spend the time fixing the
filesystem bugs instead (while still being able to reproduce them more
easily with OOM reaper disabled).

Thanks,
Andrea

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

#1722597

On Tue, Aug 29, 2017 at 7:09 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> Hello,
>
> On Tue, Aug 29, 2017 at 02:59:23PM +0200, Adam Borowski wrote:
>> On Tue, Aug 29, 2017 at 02:45:41PM +0200, Takashi Iwai wrote:
>> > [Put more people to Cc, sorry for growing too much...]
>>
>> We're all interested in 4.13.0 not crashing on us, so that's ok.
>>
>> > On Tue, 29 Aug 2017 11:19:13 +0200,
>> > Bernhard Held wrote:
>> > >
>> > > On 08/28/2017 at 06:56 PM, Nadav Amit wrote:
>> > > > Don’t blame me for the TLB stuff... My money is on aac2fea94f7a .
>> > >
>> > > Amit, thanks for your courage to expose your patch!
>> > >
>> > > I'm more and more confident that aac2fea94f7a is the culprit.  Maybe it
>> > > just accelerates the triggering of the splash.  To be more sure the
>> > > kernel needs to be tested for a couple of days.  It would be great if
>> > > others could assist in testing aac2fea94f7a.
>> >
>> > I'm testing with the revert for a while and it seems working.
>>
>> With nothing but aac2fea94f7a reverted, no explosions for me either.
>
> The aforementioned commit has 3 bugs.

Yes. I'm reverting it from my tree.

We should really *really* just tell the stupid MMU notifier users that
they can't sleep.

The MMU notifiers are not going to destroy our VM layer. I hate the
damn crap, and this kind of garbage is an example of why.

So Andrew - please stop taking "MMU notifier can sleep" patches. Start
taking patches that fix the users, not break the VM.

                    Linus

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

#1722687

On Tue, Aug 29, 2017 at 09:10:59AM -0700, Linus Torvalds wrote:
> On Tue, Aug 29, 2017 at 7:09 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> > Hello,
> >
> > On Tue, Aug 29, 2017 at 02:59:23PM +0200, Adam Borowski wrote:
> >> On Tue, Aug 29, 2017 at 02:45:41PM +0200, Takashi Iwai wrote:
> >> > [Put more people to Cc, sorry for growing too much...]
> >>
> >> We're all interested in 4.13.0 not crashing on us, so that's ok.
> >>
> >> > On Tue, 29 Aug 2017 11:19:13 +0200,
> >> > Bernhard Held wrote:
> >> > >
> >> > > On 08/28/2017 at 06:56 PM, Nadav Amit wrote:
> >> > > > Don’t blame me for the TLB stuff... My money is on aac2fea94f7a .
> >> > >
> >> > > Amit, thanks for your courage to expose your patch!
> >> > >
> >> > > I'm more and more confident that aac2fea94f7a is the culprit.  Maybe it
> >> > > just accelerates the triggering of the splash.  To be more sure the
> >> > > kernel needs to be tested for a couple of days.  It would be great if
> >> > > others could assist in testing aac2fea94f7a.
> >> >
> >> > I'm testing with the revert for a while and it seems working.
> >>
> >> With nothing but aac2fea94f7a reverted, no explosions for me either.
> >
> > The aforementioned commit has 3 bugs.
> 
> Yes. I'm reverting it from my tree.
> 
> We should really *really* just tell the stupid MMU notifier users that
> they can't sleep.

There is no way around sleeping if we ever want to support thing like
GPU. To invalidate page table on GPU you need to schedule commands to
do so on GPU command queue and wait for the GPU to signal that it has
invalidated its page table/tlb and caches.

We had this discussion before. Either we want to support all the new
fancy GPGPU, AI and all the API they rely on or we should tell them
sorry guys not on linux.

> 
> The MMU notifiers are not going to destroy our VM layer. I hate the
> damn crap, and this kind of garbage is an example of why.

Issue here is that nobody calls mmu_notifier_invalidate_range_start/end()
hence why people relied on invalidate_range() to not sleep like start/end
Now we can make the decission that start/end can sleep while the range
can't but then we also need to make sure that range_start/end is always
called.

Cheers,
Jérôme

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

#1722690

On Tue, Aug 29, 2017 at 04:09:24PM +0200, Andrea Arcangeli wrote:
> Hello,
> 
> On Tue, Aug 29, 2017 at 02:59:23PM +0200, Adam Borowski wrote:
> > On Tue, Aug 29, 2017 at 02:45:41PM +0200, Takashi Iwai wrote:
> > > [Put more people to Cc, sorry for growing too much...]
> > 
> > We're all interested in 4.13.0 not crashing on us, so that's ok.
> > 
> > > On Tue, 29 Aug 2017 11:19:13 +0200,
> > > Bernhard Held wrote:
> > > > 
> > > > On 08/28/2017 at 06:56 PM, Nadav Amit wrote:
> > > > > Don’t blame me for the TLB stuff... My money is on aac2fea94f7a .
> > > > 
> > > > Amit, thanks for your courage to expose your patch!
> > > > 
> > > > I'm more and more confident that aac2fea94f7a is the culprit.  Maybe it
> > > > just accelerates the triggering of the splash.  To be more sure the
> > > > kernel needs to be tested for a couple of days.  It would be great if
> > > > others could assist in testing aac2fea94f7a.
> > > 
> > > I'm testing with the revert for a while and it seems working.
> > 
> > With nothing but aac2fea94f7a reverted, no explosions for me either.
> 
> The aforementioned commit has 3 bugs.
> 
> 1) mmu_notifier_invalidate_range cannot be used in replacement of
>    mmu_notifier_invalidate_range_start/end. For KVM
>    mmu_notifier_invalidate_range is a noop and rightfully so. A MMU
>    notifier implementation has to implement either
>    ->invalidate_range method or the invalidate_range_start/end
>    methods, not both. And if you implement invalidate_range_start/end
>    like KVM is forced to do, calling mmu_notifier_invalidate_range in
>    common code is a noop for KVM.
> 
>    For those MMU notifiers that can get away only implementing
>    ->invalidate_range, the ->invalidate_range is implicitly called by
>    mmu_notifier_invalidate_range_end(). And only those secondary MMUs
>    that share the same pagetable with the primary MMU (like AMD
>    iommuv2) can get away only implementing ->invalidate_range.
> 
>    So all cases (THP on/off) are broken right now.
> 
>    To fix this is enough to replace mmu_notifier_invalidate_range with
>    mmu_notifier_invalidate_range_start;mmu_notifier_invalidate_range_end. Either
>    that or call multiple mmu_notifier_invalidate_page like before.

Kirill did regress invalidate_page as it use to be call outside the
spinlock and now it is call inside the spinlock thus reverting will
introduce back a regression.

You can refer to the thread about it:

https://lkml.org/lkml/2017/8/9/418

Jérôme

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

#1722703

On Tue, Aug 29, 2017 at 11:34 AM, Jerome Glisse <jglisse@redhat.com> wrote:
>
> Kirill did regress invalidate_page as it use to be call outside the
> spinlock and now it is call inside the spinlock thus reverting will
> introduce back a regression.

Honestly, this MMU notifier thing has been nothing but a badly
designed mistake from beginning to end, and bad rules for what can
sleep and what can not are one fundamental problem.

There are fundamentally two levels of VM locking, and those two levels
are not going to go away, and we're not budging on them:

 - there's the "virtual address" level, which can block. We have a
nice mmap_semaphore, and we guarantee that it's held for writing for
all changes to the virtual memory layout

   This is the "mmap/munmap" kind of granularity. The mmu callbacks at
*this* level are fine to block.

 - then there is the "page level" VM handling, and honestly, that
*fundamentally* uses a spinlock. If we look at a particular page, that
page is meaningless without the lock. Really.

   I honestly believe that any MMU callback at this level needs to be
atomic. Some of the absolutely *have* to be (that "change_pte", for
example).

In that second case, we might have a "begin/end" surrounding the
actual page table walk. And that might sleep, but then it
*fundamentally*  cannot actually be able some particular single page
or stable range. Because without the page table spinlock, no such
stability exists. It's purely a "we are not going to start looking at
this range" kind of thing.

I really don't understand why the nVidia crap cannot follow those
simple rules. Because either

 (a) you're working with virtual addresses, and you should be able to
work on that virtual layer

 (b) you're actually working with physical pages, and you can just
hold on to those physical pages yourself.

I really detest our MMU callbacks. I shouldn't have allowed them to be
merged. And I definitely shoul.dn't allow them to screw up our VM
layer.

But we have them, and we should work at making sure people do sane things.

And yes, those sane things may include

 (a) guaranteeing that the start/end range calls are always done
around the actual locked region.

 (b) adding a ton of validation so that people *see* then they break
the rules. Even when they don't use some random notifier crud.

That (b) may involve adding a number of "might_sleep()" calls (not
deep in the notifiers themselves, but in the actual wrapper functions
even when notifiers are compiled out entirely!), but also adding calls
to validate those kinds of "you can't call
mmu_notifier_invalidate_page() without having first called
mmu_notifier_invalidate_range_start() in a sleepable context".

But (b) definitely should also be a very real onus on the mmu
notifiers themselves. No way can we sleep when we're traversing page
tables. We hold a page table lock. We can sleep before and after, but
not during actual page traversal.

            Linus

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

#1722720

On Tue, Aug 29, 2017 at 12:06:42PM -0700, Linus Torvalds wrote:
> On Tue, Aug 29, 2017 at 11:34 AM, Jerome Glisse <jglisse@redhat.com> wrote:
> >
> > Kirill did regress invalidate_page as it use to be call outside the
> > spinlock and now it is call inside the spinlock thus reverting will
> > introduce back a regression.
> 
> Honestly, this MMU notifier thing has been nothing but a badly
> designed mistake from beginning to end, and bad rules for what can
> sleep and what can not are one fundamental problem.
> 
> There are fundamentally two levels of VM locking, and those two levels
> are not going to go away, and we're not budging on them:
> 
>  - there's the "virtual address" level, which can block. We have a
> nice mmap_semaphore, and we guarantee that it's held for writing for
> all changes to the virtual memory layout
> 
>    This is the "mmap/munmap" kind of granularity. The mmu callbacks at
> *this* level are fine to block.
> 
>  - then there is the "page level" VM handling, and honestly, that
> *fundamentally* uses a spinlock. If we look at a particular page, that
> page is meaningless without the lock. Really.
> 
>    I honestly believe that any MMU callback at this level needs to be
> atomic. Some of the absolutely *have* to be (that "change_pte", for
> example).
> 
> In that second case, we might have a "begin/end" surrounding the
> actual page table walk. And that might sleep, but then it
> *fundamentally*  cannot actually be able some particular single page
> or stable range. Because without the page table spinlock, no such
> stability exists. It's purely a "we are not going to start looking at
> this range" kind of thing.
> 
> I really don't understand why the nVidia crap cannot follow those
> simple rules. Because either
> 
>  (a) you're working with virtual addresses, and you should be able to
> work on that virtual layer
> 
>  (b) you're actually working with physical pages, and you can just
> hold on to those physical pages yourself.
> 
> I really detest our MMU callbacks. I shouldn't have allowed them to be
> merged. And I definitely shoul.dn't allow them to screw up our VM
> layer.
> 
> But we have them, and we should work at making sure people do sane things.
> 
> And yes, those sane things may include
> 
>  (a) guaranteeing that the start/end range calls are always done
> around the actual locked region.
> 
>  (b) adding a ton of validation so that people *see* then they break
> the rules. Even when they don't use some random notifier crud.
> 
> That (b) may involve adding a number of "might_sleep()" calls (not
> deep in the notifiers themselves, but in the actual wrapper functions
> even when notifiers are compiled out entirely!), but also adding calls
> to validate those kinds of "you can't call
> mmu_notifier_invalidate_page() without having first called
> mmu_notifier_invalidate_range_start() in a sleepable context".
> 
> But (b) definitely should also be a very real onus on the mmu
> notifiers themselves. No way can we sleep when we're traversing page
> tables. We hold a page table lock. We can sleep before and after, but
> not during actual page traversal.

Yes and i am fine with page traversal being under spinlock and not
being able to sleep during that. I agree doing otherwise would be
insane. It is just that the existing behavior of try_to_unmap_one()
and page_mkclean_one() have been broken and that no mmu_notifier
calls were added around the lock section.

I sent a patch that properly compute the range to invalidate and move
to invalidate_range() but is lacking the invalidate_range_start()/
end() so i am gonna respin that with range_start/end bracketing and
assume the worse for the range of address.

Cheers,
Jérôme

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

#1722755

On Tue, Aug 29, 2017 at 12:13 PM, Jerome Glisse <jglisse@redhat.com> wrote:
>
> Yes and i am fine with page traversal being under spinlock and not
> being able to sleep during that. I agree doing otherwise would be
> insane. It is just that the existing behavior of try_to_unmap_one()
> and page_mkclean_one() have been broken and that no mmu_notifier
> calls were added around the lock section.

Yeah, I'm actually surprised that ever worked. I'm surprised that
try_to_unmap_one didn't hold any locks earlier.

In fact, I think at least some of them *did* already hold the page
table locks: ptep_clear_flush_young_notify() and friends very much
should have always held them.

So it's literally just that mmu_notifier_invalidate_page() call that
used to be outside all the locks, but honestly, I think that was
always a bug. It means that you got notified of the page removal
*after* the page was already gone and all locks had been released, so
a completely *different* page could already have been mapped to that
address.

So I think the old code was always broken exactly because the callback
wasn't serialized with the actual action.

> I sent a patch that properly compute the range to invalidate and move
> to invalidate_range() but is lacking the invalidate_range_start()/
> end() so i am gonna respin that with range_start/end bracketing and
> assume the worse for the range of address.

So surrounding it with start/end _should_ make KVM happy.

KVM people, can you confirm?

But I do note that there's a number of other users of that
"invalidate_page" callback.

I think ib_umem_notifier_invalidate_page() the exact same blocking
issue, but changing to range_start/end should be good there too.

amdgpu_mn_invalidate_page() and the xen/gntdev also seem to be happy
being replaced with start/end.

In fact, I'm wondering if this actually means that we could get rid of
mmu_notifier_invalidate_page() entirely. There's only a couple of
callers, and the other one seems to be fs/dax.c, and it actually seems
to have the exact same issue that the try_to_unmap_one() code had: it
tried to invalidate an address too late - by the time it was called,
the page gad already been cleaned and locks had been released.

So the more I look at that "turn mmu_notifier_invalidate_page() into
invalidate_range_start/end()" the more I think that's fundamentally
the right thing to do.

                 Linus

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

#1722848

Hello Linus,

On Tue, Aug 29, 2017 at 12:38:43PM -0700, Linus Torvalds wrote:
> On Tue, Aug 29, 2017 at 12:13 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> >
> > Yes and i am fine with page traversal being under spinlock and not
> > being able to sleep during that. I agree doing otherwise would be
> > insane. It is just that the existing behavior of try_to_unmap_one()
> > and page_mkclean_one() have been broken and that no mmu_notifier
> > calls were added around the lock section.
> 
> Yeah, I'm actually surprised that ever worked. I'm surprised that
> try_to_unmap_one didn't hold any locks earlier.
> 
> In fact, I think at least some of them *did* already hold the page
> table locks: ptep_clear_flush_young_notify() and friends very much
> should have always held them.
> 
> So it's literally just that mmu_notifier_invalidate_page() call that
> used to be outside all the locks, but honestly, I think that was
> always a bug. It means that you got notified of the page removal
> *after* the page was already gone and all locks had been released, so
> a completely *different* page could already have been mapped to that
> address.
> 
> So I think the old code was always broken exactly because the callback
> wasn't serialized with the actual action.

That's a very interesting point indeed.

I thought the old code was safe because even if the secondary MMU view
keeps crunching on the old page for a little while after the primary
MMU got a new page mapped, you cannot measure it.

If you cannot measure it, it doesn't exist? I.e. undefined.

The same runtime you'd get by letting the guest mode crunch on the old
page for a little while after the primary MMU is already computing on
a brand new page, could materialize if the guest mode CPU just got a
turbo boost and computed faster before the ->invalidate_page was run
inside the PT lock.

If the page is swapped-in and is the same page that was unmapped by
try_to_unmap_one, then the primary MMU will return using the same page
that the secondary MMU was still using and it'll even be coherent
(swapin is probably prevented by the page lock, but still even if it
happens it doesn't look an issue).

If the page is suddenly replaced while vcpu is in guest mode, no
coherency could be provided anyway if the guest mode wasn't serialize
by something other than the PT lock.

I'll keep thinking about it, this is a quick answer.

On a side note, the other major requirement for the code not to have
been always broken before is that the caller must hold a refcount: the
put_page executed by try_to_unmap_one before calling
mmu_notifier_invalidate_page cannot free the page or it's unsafe. This
requirement also goes away if using range_start/end.

> > I sent a patch that properly compute the range to invalidate and move
> > to invalidate_range() but is lacking the invalidate_range_start()/
> > end() so i am gonna respin that with range_start/end bracketing and
> > assume the worse for the range of address.
> 
> So surrounding it with start/end _should_ make KVM happy.
> 
> KVM people, can you confirm?

Yes that always works as far as I can tell.

For our current problem using start/end like you suggested would
definitely make KVM happy and it is obviously safe.

It would also work to teach page_vma_mapped_walk(&pvmw) to restart
after dropping the lock with page_vma_mapped_walk_done() to call
mmu_notifier_invalidate_page outside the lock. If such an alternative
would be safe however, entirely depends if it was always broken before
which again is a very good point to think about.

The common case for such function is a single pte being unmapped.

> 
> But I do note that there's a number of other users of that
> "invalidate_page" callback.
> 
> I think ib_umem_notifier_invalidate_page() the exact same blocking
> issue, but changing to range_start/end should be good there too.
> 
> amdgpu_mn_invalidate_page() and the xen/gntdev also seem to be happy
> being replaced with start/end.
> 
> In fact, I'm wondering if this actually means that we could get rid of
> mmu_notifier_invalidate_page() entirely. There's only a couple of
> callers, and the other one seems to be fs/dax.c, and it actually seems
> to have the exact same issue that the try_to_unmap_one() code had: it
> tried to invalidate an address too late - by the time it was called,
> the page gad already been cleaned and locks had been released.
> 
> So the more I look at that "turn mmu_notifier_invalidate_page() into
> invalidate_range_start/end()" the more I think that's fundamentally
> the right thing to do.

mmu_notifier_invalidate_page exists purely as an optimized version of
mmu_notifier_invalidate_range_start/end. Furthermore when it was
always run inside the PT lock it was quite handy to replace a
ptep_clear_flush into a ptep_clear_flush_notify and be done with
it. It also requires a single branch when the MMU notifier is unarmed.

mmu_notifier_invalidate_range_start has to first block all secondary
MMU page faults, and invalidate the secondary MMU _before_ the pages
are freed. mmu_notifier_invalidate_range_end unblocks the secondary
MMU page faults after the primary MMU has been invalidated too
(i.e. after zapping the ptes).

mmu_notifier_invalidate_page has the advantage that it takes the
secondary MMU KVM srcu and spinlock a single time.

Thanks!
Andrea

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

#1722852

On Tue, Aug 29, 2017 at 1:49 PM, Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> mmu_notifier_invalidate_page has the advantage that it takes the
> secondary MMU KVM srcu and spinlock a single time.

Note that that isn't actually all that much of an advantage - it turns
out that a lot of users have "range_start", but not "range_end".

So in a lot of cases, the "range_start/end()" seems to be exactly as
expensive as just the single "page()" call, simply because the code
didn't really need the whole range, it only wanted to make sure it was
invalidating its data before the range got modified.

KVM ends up doing a partial case of that optimization too: it doesn't
do the srcu lock in the end case, for example.  It does want to keep
the sequence numbers for the end case, but that's fairly cheap.

So I'd much rather have the simpler rules than have duplicated
interfaces for some very dubious performance advantage.

               Linus

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

#1723089

On Tue 29-08-17 16:09:24, Andrea Arcangeli wrote:
[...]
> The other bug where you can reproduce the same corruption with OOM is
> unrelated and caused by the OOM reaper. OOM reaper was even corrupting
> data if a task was writing to disk and stuck in OOM in write() syscall
> or async io write.
> 
> To fix the KVM corruption in the OOM reaper, it needs to call
> mmu_notifier_invalidate_start/end around
> oom_kill.c:unmap_page_range. This additional
> mmu_notifier_invalidate_start will not be good for the OOM reaper
> because it's yet another case (like the mmap_sem for writing) that
> will prevent the OOM reaper to run, so hindering its ability to hide
> XFS OOM deadlocks, and making those resurface. Not in KVM case because
> we use a spinlock to serialize against the secondary MMU activity and
> the KVM critical section under spinlock isn't going to allocate
> memory, but range_start can schedule or block on slow hardware where
> the secondary MMU is accessed through PCI (not KVM case).

I am not really familiar with mmu notifiers and what they can actually
do. But from what you wrote above it is indeed not very safe to call
them from the oom reaper. So I will prepare and post a patch to disable
the reaper when mm_has_notifiers().

Thanks for pointing this out.

-- 
Michal Hocko
SUSE Labs

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web