[PATCH] KVM: MMU: Inform users of mmio generation wraparound

[PATCH] KVM: MMU: Inform users of mmio generation wraparound

[Takuya Yoshikawa]

Without this information, users will just see unexpected performance
problems and there is little chance we will get good reports from them:
note that mmio generation is increased even when we just start, or stop,
dirty logging for some memory slot, in which case users should never
expect all shadow pages to be zapped.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 arch/x86/kvm/mmu.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index c60c5da..bc8302f 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
 	 * The max value is MMIO_MAX_GEN - 1 since it is not called
 	 * when mark memslot invalid.
 	 */
-	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
+	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
+		printk(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound");
 		kvm_mmu_invalidate_zap_all_pages(kvm);
+	}
 }
 
 static int mmu_shrink(struct shrinker *shrink, struct shrink_control *sc)
-- 
1.7.9.5

Re: [PATCH] KVM: MMU: Inform users of mmio generation wraparound

[Paolo Bonzini]

Il 20/06/2013 10:59, Takuya Yoshikawa ha scritto:
> Without this information, users will just see unexpected performance
> problems and there is little chance we will get good reports from them:
> note that mmio generation is increased even when we just start, or stop,
> dirty logging for some memory slot, in which case users should never
> expect all shadow pages to be zapped.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
>  arch/x86/kvm/mmu.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index c60c5da..bc8302f 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
>  	 * The max value is MMIO_MAX_GEN - 1 since it is not called
>  	 * when mark memslot invalid.
>  	 */
> -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
> +	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
> +		printk(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound");

This should at least be rate-limited, because it is guest triggerable.

But why isn't the kvm_mmu_invalidate_zap_all_pages tracepoint enough?

Paolo

>  		kvm_mmu_invalidate_zap_all_pages(kvm);
> +	}
>  }
>  
>  static int mmu_shrink(struct shrinker *shrink, struct shrink_control *sc)
> 

Re: [PATCH] KVM: MMU: Inform users of mmio generation wraparound

[Gleb Natapov]

On Thu, Jun 20, 2013 at 12:59:54PM +0200, Paolo Bonzini wrote:
> Il 20/06/2013 10:59, Takuya Yoshikawa ha scritto:
> > Without this information, users will just see unexpected performance
> > problems and there is little chance we will get good reports from them:
> > note that mmio generation is increased even when we just start, or stop,
> > dirty logging for some memory slot, in which case users should never
> > expect all shadow pages to be zapped.
> > 
> > Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> > ---
> >  arch/x86/kvm/mmu.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > index c60c5da..bc8302f 100644
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
> >  	 * The max value is MMIO_MAX_GEN - 1 since it is not called
> >  	 * when mark memslot invalid.
> >  	 */
> > -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
> > +	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
> > +		printk(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound");
> 
> This should at least be rate-limited, because it is guest triggerable.
> 
It will be hard for guest to triggers it 1 << 19 times too fast though.

> But why isn't the kvm_mmu_invalidate_zap_all_pages tracepoint enough?
> 
This one will trigger during slot deletion/move too.

I would put it in to see if it actually triggers in some real world
workloads (skipping the firs wraparound since it is intentional),
we can always drop it if it will turn out to create a lot of noise.
 
--
			Gleb.

Re: [PATCH] KVM: MMU: Inform users of mmio generation wraparound

[Takuya Yoshikawa]

On Thu, 20 Jun 2013 14:45:04 +0300
Gleb Natapov <gleb@redhat.com> wrote:

> On Thu, Jun 20, 2013 at 12:59:54PM +0200, Paolo Bonzini wrote:
> > Il 20/06/2013 10:59, Takuya Yoshikawa ha scritto:
> > > Without this information, users will just see unexpected performance
> > > problems and there is little chance we will get good reports from them:
> > > note that mmio generation is increased even when we just start, or stop,
> > > dirty logging for some memory slot, in which case users should never
> > > expect all shadow pages to be zapped.
> > > 
> > > Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> > > ---
> > >  arch/x86/kvm/mmu.c |    4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > > index c60c5da..bc8302f 100644
> > > --- a/arch/x86/kvm/mmu.c
> > > +++ b/arch/x86/kvm/mmu.c
> > > @@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
> > >  	 * The max value is MMIO_MAX_GEN - 1 since it is not called
> > >  	 * when mark memslot invalid.
> > >  	 */
> > > -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
> > > +	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
> > > +		printk(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound");
> > 
> > This should at least be rate-limited, because it is guest triggerable.
> > 
> It will be hard for guest to triggers it 1 << 19 times too fast though.

I think guest-triggerable zap_all itself is a threat for the host, rather
than a matter of log flooding, even if it can be preempted.

> 
> > But why isn't the kvm_mmu_invalidate_zap_all_pages tracepoint enough?
> > 
> This one will trigger during slot deletion/move too.
> 
> I would put it in to see if it actually triggers in some real world
> workloads (skipping the firs wraparound since it is intentional),
> we can always drop it if it will turn out to create a lot of noise.
>  

This patch is not for developers but for end users: of course they do not
use tracers during running their services normally.

If they see mysterious peformance problems induced by this wraparound, the only
way to know the cause later is by this kind of information in the syslog.
So even the first wraparound may better be printed out IMO.

I want to let administrators know the cause if possible, any better way?

	Takuya

Re: [PATCH] KVM: MMU: Inform users of mmio generation wraparound

[Gleb Natapov]

On Thu, Jun 20, 2013 at 09:28:37PM +0900, Takuya Yoshikawa wrote:
> On Thu, 20 Jun 2013 14:45:04 +0300
> Gleb Natapov <gleb@redhat.com> wrote:
> 
> > On Thu, Jun 20, 2013 at 12:59:54PM +0200, Paolo Bonzini wrote:
> > > Il 20/06/2013 10:59, Takuya Yoshikawa ha scritto:
> > > > Without this information, users will just see unexpected performance
> > > > problems and there is little chance we will get good reports from them:
> > > > note that mmio generation is increased even when we just start, or stop,
> > > > dirty logging for some memory slot, in which case users should never
> > > > expect all shadow pages to be zapped.
> > > > 
> > > > Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> > > > ---
> > > >  arch/x86/kvm/mmu.c |    4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > > > index c60c5da..bc8302f 100644
> > > > --- a/arch/x86/kvm/mmu.c
> > > > +++ b/arch/x86/kvm/mmu.c
> > > > @@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
> > > >  	 * The max value is MMIO_MAX_GEN - 1 since it is not called
> > > >  	 * when mark memslot invalid.
> > > >  	 */
> > > > -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
> > > > +	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
> > > > +		printk(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound");
> > > 
> > > This should at least be rate-limited, because it is guest triggerable.
> > > 
> > It will be hard for guest to triggers it 1 << 19 times too fast though.
> 
> I think guest-triggerable zap_all itself is a threat for the host, rather
> than a matter of log flooding, even if it can be preempted.
> 
It's not much we can do about it. Slot removal/creation is triggerable
through HW emulation registers.

> > 
> > > But why isn't the kvm_mmu_invalidate_zap_all_pages tracepoint enough?
> > > 
> > This one will trigger during slot deletion/move too.
> > 
> > I would put it in to see if it actually triggers in some real world
> > workloads (skipping the firs wraparound since it is intentional),
> > we can always drop it if it will turn out to create a lot of noise.
> >  
> 
> This patch is not for developers but for end users: of course they do not
> use tracers during running their services normally.
> 
> If they see mysterious peformance problems induced by this wraparound, the only
> way to know the cause later is by this kind of information in the syslog.
> So even the first wraparound may better be printed out IMO.
Think about starting hundreds VMs on a freshly booted host. You will see
hundreds of those pretty quickly.

> 
> I want to let administrators know the cause if possible, any better way?
> 
Not that I can think of. Paolo what about print_once() and ignore first
wraparound?

--
			Gleb.

Re: [PATCH] KVM: MMU: Inform users of mmio generation wraparound

[Paolo Bonzini]

Il 20/06/2013 14:54, Gleb Natapov ha scritto:
>> If they see mysterious peformance problems induced by this wraparound, the only
>> way to know the cause later is by this kind of information in the syslog.
>> So even the first wraparound may better be printed out IMO.
> Think about starting hundreds VMs on a freshly booted host. You will see
> hundreds of those pretty quickly.

With the change I made to Xiao's patch (changing -13 to -150) you won't
see it immediately after startup, but the first wraparound may still
come very soon with a loop that reads the ROM.  (The second takes 5
minutes).

>> I want to let administrators know the cause if possible, any better way?
>>
> Not that I can think of. Paolo what about print_once() and ignore first
> wraparound?

printk_ratelimited is enough, even without ignoring the first
wraparound.  It will handle the case of multiple VMs too.

Paolo

Re: [PATCH] KVM: MMU: Inform users of mmio generation wraparound

[Takuya Yoshikawa]

On Thu, 20 Jun 2013 15:14:42 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 20/06/2013 14:54, Gleb Natapov ha scritto:
> >> If they see mysterious peformance problems induced by this wraparound, the only
> >> way to know the cause later is by this kind of information in the syslog.
> >> So even the first wraparound may better be printed out IMO.
> > Think about starting hundreds VMs on a freshly booted host. You will see
> > hundreds of those pretty quickly.
> 
> With the change I made to Xiao's patch (changing -13 to -150) you won't
> see it immediately after startup, but the first wraparound may still
> come very soon with a loop that reads the ROM.  (The second takes 5
> minutes).
> 
> >> I want to let administrators know the cause if possible, any better way?
> >>
> > Not that I can think of. Paolo what about print_once() and ignore first
> > wraparound?
> 
> printk_ratelimited is enough, even without ignoring the first
> wraparound.  It will handle the case of multiple VMs too.

OK, I'm now trying printk_ratelimited() version.
Will send v2 if it works.

	Takuya

Re: [PATCH] KVM: MMU: Inform users of mmio generation wraparound

[Takuya Yoshikawa]

On Thu, 20 Jun 2013 15:54:38 +0300
Gleb Natapov <gleb@redhat.com> wrote:

> On Thu, Jun 20, 2013 at 09:28:37PM +0900, Takuya Yoshikawa wrote:
> > On Thu, 20 Jun 2013 14:45:04 +0300
> > Gleb Natapov <gleb@redhat.com> wrote:
> > 
> > > On Thu, Jun 20, 2013 at 12:59:54PM +0200, Paolo Bonzini wrote:
> > > > Il 20/06/2013 10:59, Takuya Yoshikawa ha scritto:
> > > > > Without this information, users will just see unexpected performance
> > > > > problems and there is little chance we will get good reports from them:
> > > > > note that mmio generation is increased even when we just start, or stop,
> > > > > dirty logging for some memory slot, in which case users should never
> > > > > expect all shadow pages to be zapped.
> > > > > 
> > > > > Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> > > > > ---
> > > > >  arch/x86/kvm/mmu.c |    4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > > > > index c60c5da..bc8302f 100644
> > > > > --- a/arch/x86/kvm/mmu.c
> > > > > +++ b/arch/x86/kvm/mmu.c
> > > > > @@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
> > > > >  	 * The max value is MMIO_MAX_GEN - 1 since it is not called
> > > > >  	 * when mark memslot invalid.
> > > > >  	 */
> > > > > -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
> > > > > +	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
> > > > > +		printk(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound");
> > > > 
> > > > This should at least be rate-limited, because it is guest triggerable.
> > > > 
> > > It will be hard for guest to triggers it 1 << 19 times too fast though.
> > 
> > I think guest-triggerable zap_all itself is a threat for the host, rather
> > than a matter of log flooding, even if it can be preempted.
> > 
> It's not much we can do about it. Slot removal/creation is triggerable
> through HW emulation registers.

OK, I see.

> 
> > > 
> > > > But why isn't the kvm_mmu_invalidate_zap_all_pages tracepoint enough?
> > > > 
> > > This one will trigger during slot deletion/move too.
> > > 
> > > I would put it in to see if it actually triggers in some real world
> > > workloads (skipping the firs wraparound since it is intentional),
> > > we can always drop it if it will turn out to create a lot of noise.
> > >  
> > 
> > This patch is not for developers but for end users: of course they do not
> > use tracers during running their services normally.
> > 
> > If they see mysterious peformance problems induced by this wraparound, the only
> > way to know the cause later is by this kind of information in the syslog.
> > So even the first wraparound may better be printed out IMO.
> Think about starting hundreds VMs on a freshly booted host. You will see
> hundreds of those pretty quickly.

Yes.

> 
> > 
> > I want to let administrators know the cause if possible, any better way?
> > 
> Not that I can think of. Paolo what about print_once() and ignore first
> wraparound?

Assuming that the first one will be removed someday, it's for debugging anyway,
we can just do print_once() in the future?

That way, admins can check if there is any guest which did some problematic
things.

	Takuya

Re: [PATCH] KVM: MMU: Inform users of mmio generation wraparound

[Paolo Bonzini]

Il 20/06/2013 13:45, Gleb Natapov ha scritto:
> On Thu, Jun 20, 2013 at 12:59:54PM +0200, Paolo Bonzini wrote:
>> Il 20/06/2013 10:59, Takuya Yoshikawa ha scritto:
>>> Without this information, users will just see unexpected performance
>>> problems and there is little chance we will get good reports from them:
>>> note that mmio generation is increased even when we just start, or stop,
>>> dirty logging for some memory slot, in which case users should never
>>> expect all shadow pages to be zapped.
>>>
>>> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
>>> ---
>>>  arch/x86/kvm/mmu.c |    4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>>> index c60c5da..bc8302f 100644
>>> --- a/arch/x86/kvm/mmu.c
>>> +++ b/arch/x86/kvm/mmu.c
>>> @@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
>>>  	 * The max value is MMIO_MAX_GEN - 1 since it is not called
>>>  	 * when mark memslot invalid.
>>>  	 */
>>> -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
>>> +	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
>>> +		printk(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound");
>>
>> This should at least be rate-limited, because it is guest triggerable.
>>
> It will be hard for guest to triggers it 1 << 19 times too fast though.
> 
>> But why isn't the kvm_mmu_invalidate_zap_all_pages tracepoint enough?
>
> This one will trigger during slot deletion/move too.
> 
> I would put it in to see if it actually triggers in some real world
> workloads (skipping the firs wraparound since it is intentional),
> we can always drop it if it will turn out to create a lot of noise.

Reading a ROM in a loop can trigger it in less than 5 minutes on my
machine.  Not a lot of noise, but enough to be annoying.  I think the
existing tracepoint is enough, or we can add a more specific one here.

Paolo

Re: [PATCH] KVM: MMU: Inform users of mmio generation wraparound

[Gleb Natapov]

On Thu, Jun 20, 2013 at 02:35:36PM +0200, Paolo Bonzini wrote:
> Il 20/06/2013 13:45, Gleb Natapov ha scritto:
> > On Thu, Jun 20, 2013 at 12:59:54PM +0200, Paolo Bonzini wrote:
> >> Il 20/06/2013 10:59, Takuya Yoshikawa ha scritto:
> >>> Without this information, users will just see unexpected performance
> >>> problems and there is little chance we will get good reports from them:
> >>> note that mmio generation is increased even when we just start, or stop,
> >>> dirty logging for some memory slot, in which case users should never
> >>> expect all shadow pages to be zapped.
> >>>
> >>> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> >>> ---
> >>>  arch/x86/kvm/mmu.c |    4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> >>> index c60c5da..bc8302f 100644
> >>> --- a/arch/x86/kvm/mmu.c
> >>> +++ b/arch/x86/kvm/mmu.c
> >>> @@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
> >>>  	 * The max value is MMIO_MAX_GEN - 1 since it is not called
> >>>  	 * when mark memslot invalid.
> >>>  	 */
> >>> -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
> >>> +	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
> >>> +		printk(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound");
> >>
> >> This should at least be rate-limited, because it is guest triggerable.
> >>
> > It will be hard for guest to triggers it 1 << 19 times too fast though.
> > 
> >> But why isn't the kvm_mmu_invalidate_zap_all_pages tracepoint enough?
> >
> > This one will trigger during slot deletion/move too.
> > 
> > I would put it in to see if it actually triggers in some real world
> > workloads (skipping the firs wraparound since it is intentional),
> > we can always drop it if it will turn out to create a lot of noise.
> 
> Reading a ROM in a loop can trigger it in less than 5 minutes on my
> machine.  Not a lot of noise, but enough to be annoying.  I think the
> existing tracepoint is enough, or we can add a more specific one here.
> 
5 minutes after first wraparound?

--
			Gleb.