kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] kvm: Cap halt polling at kvm->max_halt_poll_ns
@ 2021-05-06 15:24 Venkatesh Srinivas
  2021-05-06 16:13 ` Sean Christopherson
  0 siblings, 1 reply; 4+ messages in thread
From: Venkatesh Srinivas @ 2021-05-06 15:24 UTC (permalink / raw)
  To: kvm, dmatlack, pbonzini; +Cc: Venkatesh Srinivas

From: David Matlack <dmatlack@google.com>

When growing halt-polling, there is no check that the poll time exceeds
the per-VM limit. It's possible for vcpu->halt_poll_ns to grow past
kvm->max_halt_poll_ns and stay there until a halt which takes longer
than kvm->halt_poll_ns.

Signed-off-by: David Matlack <dmatlack@google.com>
Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org>
---
 virt/kvm/kvm_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2799c6660cce..120817c5f271 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2893,8 +2893,8 @@ static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
 	if (val < grow_start)
 		val = grow_start;
 
-	if (val > halt_poll_ns)
-		val = halt_poll_ns;
+	if (val > vcpu->kvm->max_halt_poll_ns)
+		val = vcpu->kvm->max_halt_poll_ns;
 
 	vcpu->halt_poll_ns = val;
 out:
-- 
2.31.1.607.g51e8a6a459-goog


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] kvm: Cap halt polling at kvm->max_halt_poll_ns
  2021-05-06 15:24 [PATCH] kvm: Cap halt polling at kvm->max_halt_poll_ns Venkatesh Srinivas
@ 2021-05-06 16:13 ` Sean Christopherson
  2021-05-06 16:30   ` David Matlack
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Christopherson @ 2021-05-06 16:13 UTC (permalink / raw)
  To: Venkatesh Srinivas; +Cc: kvm, dmatlack, pbonzini

Prefer capitalizing KVM in the shortlog, if only because I'm lazy with grep :-)

On Thu, May 06, 2021, Venkatesh Srinivas wrote:
> From: David Matlack <dmatlack@google.com>
> 
> When growing halt-polling, there is no check that the poll time exceeds
> the per-VM limit. It's possible for vcpu->halt_poll_ns to grow past
> kvm->max_halt_poll_ns and stay there until a halt which takes longer
> than kvm->halt_poll_ns.
> 

Fixes: acd05785e48c ("kvm: add capability for halt polling")

and probably Cc: stable@ too.


> Signed-off-by: David Matlack <dmatlack@google.com>
> Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org>
> ---
>  virt/kvm/kvm_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2799c6660cce..120817c5f271 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2893,8 +2893,8 @@ static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
>  	if (val < grow_start)
>  		val = grow_start;
>  
> -	if (val > halt_poll_ns)
> -		val = halt_poll_ns;
> +	if (val > vcpu->kvm->max_halt_poll_ns)
> +		val = vcpu->kvm->max_halt_poll_ns;

Hmm, I would argue that the introduction of the capability broke halt_poll_ns.
The halt_poll_ns module param is writable after KVM is loaded.  Prior to the
capability, that meant the admin could adjust the param on the fly and all vCPUs
would honor the new value as it was changed.

By snapshotting the module param at VM creation, those semantics were lost.
That's not necessarily wrong/bad, but I don't see anything in the changelog for
the capability that suggests killing the old behavior was intentional/desirable.

>  
>  	vcpu->halt_poll_ns = val;
>  out:
> -- 
> 2.31.1.607.g51e8a6a459-goog
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] kvm: Cap halt polling at kvm->max_halt_poll_ns
  2021-05-06 16:13 ` Sean Christopherson
@ 2021-05-06 16:30   ` David Matlack
  2021-05-06 17:41     ` Sean Christopherson
  0 siblings, 1 reply; 4+ messages in thread
From: David Matlack @ 2021-05-06 16:30 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: Venkatesh Srinivas, kvm list, Paolo Bonzini

On Thu, May 6, 2021 at 9:13 AM Sean Christopherson <seanjc@google.com> wrote:
>
> Prefer capitalizing KVM in the shortlog, if only because I'm lazy with grep :-)
>
> On Thu, May 06, 2021, Venkatesh Srinivas wrote:
> > From: David Matlack <dmatlack@google.com>
> >
> > When growing halt-polling, there is no check that the poll time exceeds
> > the per-VM limit. It's possible for vcpu->halt_poll_ns to grow past
> > kvm->max_halt_poll_ns and stay there until a halt which takes longer
> > than kvm->halt_poll_ns.
> >
>
> Fixes: acd05785e48c ("kvm: add capability for halt polling")
>
> and probably Cc: stable@ too.
>
>
> > Signed-off-by: David Matlack <dmatlack@google.com>
> > Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org>
> > ---
> >  virt/kvm/kvm_main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 2799c6660cce..120817c5f271 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -2893,8 +2893,8 @@ static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
> >       if (val < grow_start)
> >               val = grow_start;
> >
> > -     if (val > halt_poll_ns)
> > -             val = halt_poll_ns;
> > +     if (val > vcpu->kvm->max_halt_poll_ns)
> > +             val = vcpu->kvm->max_halt_poll_ns;
>
> Hmm, I would argue that the introduction of the capability broke halt_poll_ns.
> The halt_poll_ns module param is writable after KVM is loaded.  Prior to the
> capability, that meant the admin could adjust the param on the fly and all vCPUs
> would honor the new value as it was changed.
>
> By snapshotting the module param at VM creation, those semantics were lost.
> That's not necessarily wrong/bad, but I don't see anything in the changelog for
> the capability that suggests killing the old behavior was intentional/desirable.

api.rst does say the capability overrides halt_poll_ns. But I could
see value in changing the semantics to something like:

- halt_poll_ns sets machine-wide maximum halt poll time.
- kvm->max_halt_poll_ns sets VM-wide maximum halt poll time.
- A vCPU will poll for at most min(halt_poll_ns,
kvm->max_halt_poll_ns) (aside from an in-progress poll when either
parameter is changed).

On a related note, the capability and these subtle details should be
documented in Documentation/virtual/kvm/halt-polling.txt.


>
> >
> >       vcpu->halt_poll_ns = val;
> >  out:
> > --
> > 2.31.1.607.g51e8a6a459-goog
> >

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] kvm: Cap halt polling at kvm->max_halt_poll_ns
  2021-05-06 16:30   ` David Matlack
@ 2021-05-06 17:41     ` Sean Christopherson
  0 siblings, 0 replies; 4+ messages in thread
From: Sean Christopherson @ 2021-05-06 17:41 UTC (permalink / raw)
  To: David Matlack; +Cc: Venkatesh Srinivas, kvm list, Paolo Bonzini

On Thu, May 06, 2021, David Matlack wrote:
> On Thu, May 6, 2021 at 9:13 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > Prefer capitalizing KVM in the shortlog, if only because I'm lazy with grep :-)
> >
> > On Thu, May 06, 2021, Venkatesh Srinivas wrote:
> > > From: David Matlack <dmatlack@google.com>
> > >
> > > When growing halt-polling, there is no check that the poll time exceeds
> > > the per-VM limit. It's possible for vcpu->halt_poll_ns to grow past
> > > kvm->max_halt_poll_ns and stay there until a halt which takes longer
> > > than kvm->halt_poll_ns.
> > >
> >
> > Fixes: acd05785e48c ("kvm: add capability for halt polling")
> >
> > and probably Cc: stable@ too.
> >
> >
> > > Signed-off-by: David Matlack <dmatlack@google.com>
> > > Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org>
> > > ---
> > >  virt/kvm/kvm_main.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 2799c6660cce..120817c5f271 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -2893,8 +2893,8 @@ static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
> > >       if (val < grow_start)
> > >               val = grow_start;
> > >
> > > -     if (val > halt_poll_ns)
> > > -             val = halt_poll_ns;
> > > +     if (val > vcpu->kvm->max_halt_poll_ns)
> > > +             val = vcpu->kvm->max_halt_poll_ns;
> >
> > Hmm, I would argue that the introduction of the capability broke halt_poll_ns.
> > The halt_poll_ns module param is writable after KVM is loaded.  Prior to the
> > capability, that meant the admin could adjust the param on the fly and all vCPUs
> > would honor the new value as it was changed.
> >
> > By snapshotting the module param at VM creation, those semantics were lost.
> > That's not necessarily wrong/bad, but I don't see anything in the changelog for
> > the capability that suggests killing the old behavior was intentional/desirable.
> 
> api.rst does say the capability overrides halt_poll_ns.

Ya, I'm more concerned about old userspace that isn't aware of the capability,
e.g. an old userspace that relies on tuning halt_poll_ns while a VM is running
would break when upgrading to a version of KVM that supports the capability.

> see value in changing the semantics to something like:
> 
> - halt_poll_ns sets machine-wide maximum halt poll time.
> - kvm->max_halt_poll_ns sets VM-wide maximum halt poll time.
> - A vCPU will poll for at most min(halt_poll_ns,
> kvm->max_halt_poll_ns) (aside from an in-progress poll when either
> parameter is changed).

Agreed.  That would also provide a good opportunity to clean up the benign
races in kvm_vcpu_block(); since both the module param and the per-VM variable
can be modified at will, they really should only be read once per instance of
kvm_vcpu_block().

> On a related note, the capability and these subtle details should be
> documented in Documentation/virtual/kvm/halt-polling.txt.
> 
> 
> >
> > >
> > >       vcpu->halt_poll_ns = val;
> > >  out:
> > > --
> > > 2.31.1.607.g51e8a6a459-goog
> > >

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-05-06 17:41 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 15:24 [PATCH] kvm: Cap halt polling at kvm->max_halt_poll_ns Venkatesh Srinivas
2021-05-06 16:13 ` Sean Christopherson
2021-05-06 16:30   ` David Matlack
2021-05-06 17:41     ` Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).