* [bug report] KVM: x86/mmu: Use an rwlock for the x86 MMU
@ 2021-07-26 7:52 Dan Carpenter
2021-07-26 16:47 ` Ben Gardon
2021-07-26 16:53 ` David Matlack
0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-07-26 7:52 UTC (permalink / raw)
To: bgardon; +Cc: kvm
[ This is not the correct patch to blame, but there is something going
on here which I don't understand so this email is more about me
learning rather than reporting bugs. - dan ]
Hello Ben Gardon,
The patch 531810caa9f4: "KVM: x86/mmu: Use an rwlock for the x86 MMU"
from Feb 2, 2021, leads to the following static checker warning:
arch/x86/kvm/mmu/mmu.c:5769 kvm_mmu_zap_all()
warn: sleeping in atomic context
arch/x86/kvm/mmu/mmu.c
5756 void kvm_mmu_zap_all(struct kvm *kvm)
5757 {
5758 struct kvm_mmu_page *sp, *node;
5759 LIST_HEAD(invalid_list);
5760 int ign;
5761
5762 write_lock(&kvm->mmu_lock);
^^^^^^^^^^^^^^^^^^^^^^^^^^
This line bumps the preempt count.
5763 restart:
5764 list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
5765 if (WARN_ON(sp->role.invalid))
5766 continue;
5767 if (__kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list, &ign))
5768 goto restart;
--> 5769 if (cond_resched_rwlock_write(&kvm->mmu_lock))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This line triggers a sleeping in atomic warning. What's going on here
that I'm not understanding?
5770 goto restart;
5771 }
5772
5773 kvm_mmu_commit_zap_page(kvm, &invalid_list);
5774
5775 if (is_tdp_mmu_enabled(kvm))
5776 kvm_tdp_mmu_zap_all(kvm);
5777
5778 write_unlock(&kvm->mmu_lock);
5779 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] KVM: x86/mmu: Use an rwlock for the x86 MMU
2021-07-26 7:52 [bug report] KVM: x86/mmu: Use an rwlock for the x86 MMU Dan Carpenter
@ 2021-07-26 16:47 ` Ben Gardon
2021-07-27 0:16 ` Sean Christopherson
2021-07-26 16:53 ` David Matlack
1 sibling, 1 reply; 5+ messages in thread
From: Ben Gardon @ 2021-07-26 16:47 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kvm
On Mon, Jul 26, 2021 at 12:52 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> [ This is not the correct patch to blame, but there is something going
> on here which I don't understand so this email is more about me
> learning rather than reporting bugs. - dan ]
>
> Hello Ben Gardon,
>
> The patch 531810caa9f4: "KVM: x86/mmu: Use an rwlock for the x86 MMU"
> from Feb 2, 2021, leads to the following static checker warning:
>
> arch/x86/kvm/mmu/mmu.c:5769 kvm_mmu_zap_all()
> warn: sleeping in atomic context
>
> arch/x86/kvm/mmu/mmu.c
> 5756 void kvm_mmu_zap_all(struct kvm *kvm)
> 5757 {
> 5758 struct kvm_mmu_page *sp, *node;
> 5759 LIST_HEAD(invalid_list);
> 5760 int ign;
> 5761
> 5762 write_lock(&kvm->mmu_lock);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> This line bumps the preempt count.
>
> 5763 restart:
> 5764 list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> 5765 if (WARN_ON(sp->role.invalid))
> 5766 continue;
> 5767 if (__kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list, &ign))
> 5768 goto restart;
> --> 5769 if (cond_resched_rwlock_write(&kvm->mmu_lock))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This line triggers a sleeping in atomic warning. What's going on here
> that I'm not understanding?
Hi Dan,
Thanks for sending this. I'm confused by this sequence too. I'm not
sure how this could sleep in an atomic context.
My first thought was that there might be something going on with the
qrwlock's wait_lock, but since this thread already acquired the
rwlock, it can't be holding / waiting on the wait_lock.
Then I thought the __might_sleep could be in the wrong place, but it's
in the same place for a regular spinlock, so I think that's fine.
I do note that __cond_resched_rwlock does not check rwlock_needbreak
like __cond_resched_lock checks spin_needbreak. That seems like an
oversight, but I don't see how it could cause this warning.
I'm as confused by this as you. Did you confirm that this sleeping in
atomic warning does not happen before this commit? What kind of
configuration are you able to reproduce this on?
It might be worth asking some sched / locking folks about this as
they'll likely have a better understanding of all the intricacies of
the layers of locking macros.
I'm very curious to understand what's causing this too.
Ben
>
>
> 5770 goto restart;
> 5771 }
> 5772
> 5773 kvm_mmu_commit_zap_page(kvm, &invalid_list);
> 5774
> 5775 if (is_tdp_mmu_enabled(kvm))
> 5776 kvm_tdp_mmu_zap_all(kvm);
> 5777
> 5778 write_unlock(&kvm->mmu_lock);
> 5779 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] KVM: x86/mmu: Use an rwlock for the x86 MMU
2021-07-26 7:52 [bug report] KVM: x86/mmu: Use an rwlock for the x86 MMU Dan Carpenter
2021-07-26 16:47 ` Ben Gardon
@ 2021-07-26 16:53 ` David Matlack
1 sibling, 0 replies; 5+ messages in thread
From: David Matlack @ 2021-07-26 16:53 UTC (permalink / raw)
To: Dan Carpenter; +Cc: bgardon, kvm
On Mon, Jul 26, 2021 at 10:52:38AM +0300, Dan Carpenter wrote:
> [ This is not the correct patch to blame, but there is something going
> on here which I don't understand so this email is more about me
> learning rather than reporting bugs. - dan ]
>
> Hello Ben Gardon,
>
> The patch 531810caa9f4: "KVM: x86/mmu: Use an rwlock for the x86 MMU"
> from Feb 2, 2021, leads to the following static checker warning:
>
> arch/x86/kvm/mmu/mmu.c:5769 kvm_mmu_zap_all()
> warn: sleeping in atomic context
>
> arch/x86/kvm/mmu/mmu.c
> 5756 void kvm_mmu_zap_all(struct kvm *kvm)
> 5757 {
> 5758 struct kvm_mmu_page *sp, *node;
> 5759 LIST_HEAD(invalid_list);
> 5760 int ign;
> 5761
> 5762 write_lock(&kvm->mmu_lock);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> This line bumps the preempt count.
>
> 5763 restart:
> 5764 list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> 5765 if (WARN_ON(sp->role.invalid))
> 5766 continue;
> 5767 if (__kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list, &ign))
> 5768 goto restart;
> --> 5769 if (cond_resched_rwlock_write(&kvm->mmu_lock))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This line triggers a sleeping in atomic warning. What's going on here
> that I'm not understanding?
cond_resched_rwlock_write drops the provided lock (kvm->mmu_lock in this
case) before scheduling and then re-acquires it afterwards. So this
warning looks like a false positive.
>
> 5770 goto restart;
> 5771 }
> 5772
> 5773 kvm_mmu_commit_zap_page(kvm, &invalid_list);
> 5774
> 5775 if (is_tdp_mmu_enabled(kvm))
> 5776 kvm_tdp_mmu_zap_all(kvm);
> 5777
> 5778 write_unlock(&kvm->mmu_lock);
> 5779 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] KVM: x86/mmu: Use an rwlock for the x86 MMU
2021-07-26 16:47 ` Ben Gardon
@ 2021-07-27 0:16 ` Sean Christopherson
2021-07-27 8:06 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2021-07-27 0:16 UTC (permalink / raw)
To: Ben Gardon; +Cc: Dan Carpenter, kvm
On Mon, Jul 26, 2021, Ben Gardon wrote:
> On Mon, Jul 26, 2021 at 12:52 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > [ This is not the correct patch to blame, but there is something going
> > on here which I don't understand so this email is more about me
> > learning rather than reporting bugs. - dan ]
> >
> > Hello Ben Gardon,
> >
> > The patch 531810caa9f4: "KVM: x86/mmu: Use an rwlock for the x86 MMU"
> > from Feb 2, 2021, leads to the following static checker warning:
> >
> > arch/x86/kvm/mmu/mmu.c:5769 kvm_mmu_zap_all()
> > warn: sleeping in atomic context
> >
> > arch/x86/kvm/mmu/mmu.c
> > 5756 void kvm_mmu_zap_all(struct kvm *kvm)
> > 5757 {
> > 5758 struct kvm_mmu_page *sp, *node;
> > 5759 LIST_HEAD(invalid_list);
> > 5760 int ign;
> > 5761
> > 5762 write_lock(&kvm->mmu_lock);
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This line bumps the preempt count.
> >
> > 5763 restart:
> > 5764 list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> > 5765 if (WARN_ON(sp->role.invalid))
> > 5766 continue;
> > 5767 if (__kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list, &ign))
> > 5768 goto restart;
> > --> 5769 if (cond_resched_rwlock_write(&kvm->mmu_lock))
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This line triggers a sleeping in atomic warning. What's going on here
> > that I'm not understanding?
>
>
> Hi Dan,
>
> Thanks for sending this. I'm confused by this sequence too. I'm not
> sure how this could sleep in an atomic context.
> My first thought was that there might be something going on with the
> qrwlock's wait_lock, but since this thread already acquired the
> rwlock, it can't be holding / waiting on the wait_lock.
>
> Then I thought the __might_sleep could be in the wrong place, but it's
> in the same place for a regular spinlock, so I think that's fine.
The PREEMPT_LOCK_OFFSET parameter to __might_sleep()
__might_sleep(__FILE__, __LINE__, PREEMPT_LOCK_OFFSET); \
effectively tells it to exempt a single preemption count via preempt_count_equals()
void ___might_sleep(const char *file, int line, int preempt_offset)
{
...
if ((preempt_count_equals(preempt_offset) && !irqs_disabled() &&
!is_idle_task(current) && !current->non_block_count) ||
system_state == SYSTEM_BOOTING || system_state > SYSTEM_RUNNING ||
oops_in_progress)
return;
...
}
which returns true if the preempt count equals the passed in offset.
PREEMPT_LOCK_OFFSET is just the vanilla preempt_disable() offset, which is why
there's no special preemption call in the lock/unlock paths.
#define PREEMPT_LOCK_OFFSET PREEMPT_DISABLE_OFFSET
Dan, is this coming from Smatch? If so, is this by chance a new, in-progress
warning that has special code to handle cond_resched_lock()? I couldn't find
any matches on "sleeping in atomic context" in Smatch. The rwlock variants,
cond_resched_rwlock_{read,write}() were added specifically for KVM's TDP MMU,
maybe they snuck in after a waiver for cond_resched_lock() was added?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] KVM: x86/mmu: Use an rwlock for the x86 MMU
2021-07-27 0:16 ` Sean Christopherson
@ 2021-07-27 8:06 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-07-27 8:06 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Ben Gardon, kvm
Yeah. Thanks so much. This is a new Smatch warning I'm working on.
It triggers a warning whenever __might_sleep() or __might_sleep() are
called and the prempt count is non-zero. I didn't know what the
"preempt_offset" argument was for.
I'll modify the check to only warn when "preempt_offset" is zero.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-27 8:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-26 7:52 [bug report] KVM: x86/mmu: Use an rwlock for the x86 MMU Dan Carpenter
2021-07-26 16:47 ` Ben Gardon
2021-07-27 0:16 ` Sean Christopherson
2021-07-27 8:06 ` Dan Carpenter
2021-07-26 16:53 ` David Matlack
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.