linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][RFC] VM: x86: Return ealier if clocksource has not changed
@ 2016-12-23  8:41 Chen Yu
  2016-12-26 19:44 ` Marcelo Tosatti
  0 siblings, 1 reply; 7+ messages in thread
From: Chen Yu @ 2016-12-23  8:41 UTC (permalink / raw)
  To: kvm
  Cc: x86, linux-kernel, Rafael J. Wysocki, Len Brown, Chen Yu,
	Paolo Bonzini, Radim Krcmar, Marcelo Tosatti, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, Wanpeng Li

Currently the notifier of pvclock_gtod_notify() get invoked
frequently due to the periodic update_wall_time(). This might
slow down the system a little bit as there might be redundant
execution code path and unnecessary lock contention
in update_pvclock_gtod(), which was found when I was doing
suspend/resume speed testings. As pvclock_gtod_notify()
should be invoked only when clocksource has changed, according to
Commit 16e8d74d2da9 ("KVM: x86: notifier for clocksource changes")
, either we can add a new notifier for clocksource switch,
or we can simply bypass the following code in pvclock_gtod_notify()
earlier if there is no clocksource switch.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krcmar" <rkrcmar@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 arch/x86/kvm/x86.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 445c51b..54aa32d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5961,13 +5961,14 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
 	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
 	struct timekeeper *tk = priv;
 
+	if (likely(gtod->clock.vclock_mode == VCLOCK_TSC))
+		return 0;
 	update_pvclock_gtod(tk);
 
 	/* disable master clock if host does not trust, or does not
 	 * use, TSC clocksource
 	 */
-	if (gtod->clock.vclock_mode != VCLOCK_TSC &&
-	    atomic_read(&kvm_guest_has_master_clock) != 0)
+	if (atomic_read(&kvm_guest_has_master_clock) != 0)
 		queue_work(system_long_wq, &pvclock_gtod_work);
 
 	return 0;
-- 
2.7.4

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

* Re: [PATCH][RFC] VM: x86: Return ealier if clocksource has not changed
  2016-12-23  8:41 [PATCH][RFC] VM: x86: Return ealier if clocksource has not changed Chen Yu
@ 2016-12-26 19:44 ` Marcelo Tosatti
  2016-12-27  8:06   ` Chen Yu
  0 siblings, 1 reply; 7+ messages in thread
From: Marcelo Tosatti @ 2016-12-26 19:44 UTC (permalink / raw)
  To: Chen Yu
  Cc: kvm, x86, linux-kernel, Rafael J. Wysocki, Len Brown,
	Paolo Bonzini, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, Wanpeng Li

On Fri, Dec 23, 2016 at 04:41:53PM +0800, Chen Yu wrote:
> Currently the notifier of pvclock_gtod_notify() get invoked
> frequently due to the periodic update_wall_time(). This might
> slow down the system a little bit as there might be redundant
> execution code path and unnecessary lock contention
> in update_pvclock_gtod(), which was found when I was doing
> suspend/resume speed testings. As pvclock_gtod_notify()
> should be invoked only when clocksource has changed, according to
> Commit 16e8d74d2da9 ("KVM: x86: notifier for clocksource changes")
> , either we can add a new notifier for clocksource switch,
> or we can simply bypass the following code in pvclock_gtod_notify()
> earlier if there is no clocksource switch.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krcmar" <rkrcmar@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Wanpeng Li <wanpeng.li@hotmail.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  arch/x86/kvm/x86.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 445c51b..54aa32d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5961,13 +5961,14 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
>  	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
>  	struct timekeeper *tk = priv;
>  
> +	if (likely(gtod->clock.vclock_mode == VCLOCK_TSC))
> +		return 0;

I think this is only safe if any of the values in "struct
pvclock_gtod_data" are unchanged. Otherwise the local (KVM) copy is 
kept incorrect.

>  	update_pvclock_gtod(tk);
>  
>  	/* disable master clock if host does not trust, or does not
>  	 * use, TSC clocksource
>  	 */
> -	if (gtod->clock.vclock_mode != VCLOCK_TSC &&
> -	    atomic_read(&kvm_guest_has_master_clock) != 0)
> +	if (atomic_read(&kvm_guest_has_master_clock) != 0)
>  		queue_work(system_long_wq, &pvclock_gtod_work);
>  
>  	return 0;
> -- 
> 2.7.4

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

* Re: [PATCH][RFC] VM: x86: Return ealier if clocksource has not changed
  2016-12-26 19:44 ` Marcelo Tosatti
@ 2016-12-27  8:06   ` Chen Yu
  2016-12-27 15:32     ` Marcelo Tosatti
  0 siblings, 1 reply; 7+ messages in thread
From: Chen Yu @ 2016-12-27  8:06 UTC (permalink / raw)
  To: Marcelo Tosatti
  Cc: kvm, x86, linux-kernel, Rafael J. Wysocki, Len Brown,
	Paolo Bonzini, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, Wanpeng Li

Hi Marcelo,
On Mon, Dec 26, 2016 at 05:44:25PM -0200, Marcelo Tosatti wrote:
> On Fri, Dec 23, 2016 at 04:41:53PM +0800, Chen Yu wrote:
> > Currently the notifier of pvclock_gtod_notify() get invoked
> > frequently due to the periodic update_wall_time(). This might
> > slow down the system a little bit as there might be redundant
> > execution code path and unnecessary lock contention
> > in update_pvclock_gtod(), which was found when I was doing
> > suspend/resume speed testings. As pvclock_gtod_notify()
> > should be invoked only when clocksource has changed, according to
> > Commit 16e8d74d2da9 ("KVM: x86: notifier for clocksource changes")
> > , either we can add a new notifier for clocksource switch,
> > or we can simply bypass the following code in pvclock_gtod_notify()
> > earlier if there is no clocksource switch.
> > 
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: "Radim Krcmar" <rkrcmar@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Wanpeng Li <wanpeng.li@hotmail.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  arch/x86/kvm/x86.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 445c51b..54aa32d 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5961,13 +5961,14 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
> >  	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> >  	struct timekeeper *tk = priv;
> >  
> > +	if (likely(gtod->clock.vclock_mode == VCLOCK_TSC))
> > +		return 0;
> 
> I think this is only safe if any of the values in "struct
> pvclock_gtod_data" are unchanged. Otherwise the local (KVM) copy is 
> kept incorrect.
I missread the code previously and I thought only under the condition
the clocksource has been switched to another one will the KVM copy
be touched. Apparently it is not the case because the copy should
be updated on-time during normal tick, right?
thanks for your reply,
Yu
> 
> >  	update_pvclock_gtod(tk);
> >  
> >  	/* disable master clock if host does not trust, or does not
> >  	 * use, TSC clocksource
> >  	 */
> > -	if (gtod->clock.vclock_mode != VCLOCK_TSC &&
> > -	    atomic_read(&kvm_guest_has_master_clock) != 0)
> > +	if (atomic_read(&kvm_guest_has_master_clock) != 0)
> >  		queue_work(system_long_wq, &pvclock_gtod_work);
> >  
> >  	return 0;
> > -- 
> > 2.7.4

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

* Re: [PATCH][RFC] VM: x86: Return ealier if clocksource has not changed
  2016-12-27  8:06   ` Chen Yu
@ 2016-12-27 15:32     ` Marcelo Tosatti
  2016-12-29  8:59       ` Chen Yu
  0 siblings, 1 reply; 7+ messages in thread
From: Marcelo Tosatti @ 2016-12-27 15:32 UTC (permalink / raw)
  To: Chen Yu
  Cc: kvm, x86, linux-kernel, Rafael J. Wysocki, Len Brown,
	Paolo Bonzini, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, Wanpeng Li

On Tue, Dec 27, 2016 at 04:06:44PM +0800, Chen Yu wrote:
> Hi Marcelo,
> On Mon, Dec 26, 2016 at 05:44:25PM -0200, Marcelo Tosatti wrote:
> > On Fri, Dec 23, 2016 at 04:41:53PM +0800, Chen Yu wrote:
> > > Currently the notifier of pvclock_gtod_notify() get invoked
> > > frequently due to the periodic update_wall_time(). This might
> > > slow down the system a little bit as there might be redundant
> > > execution code path and unnecessary lock contention
> > > in update_pvclock_gtod(), which was found when I was doing
> > > suspend/resume speed testings. As pvclock_gtod_notify()
> > > should be invoked only when clocksource has changed, according to
> > > Commit 16e8d74d2da9 ("KVM: x86: notifier for clocksource changes")
> > > , either we can add a new notifier for clocksource switch,
> > > or we can simply bypass the following code in pvclock_gtod_notify()
> > > earlier if there is no clocksource switch.
> > > 
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: "Radim Krcmar" <rkrcmar@redhat.com>
> > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Ingo Molnar <mingo@redhat.com>
> > > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > > Cc: Wanpeng Li <wanpeng.li@hotmail.com>
> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > ---
> > >  arch/x86/kvm/x86.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 445c51b..54aa32d 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -5961,13 +5961,14 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
> > >  	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> > >  	struct timekeeper *tk = priv;
> > >  
> > > +	if (likely(gtod->clock.vclock_mode == VCLOCK_TSC))
> > > +		return 0;
> > 
> > I think this is only safe if any of the values in "struct
> > pvclock_gtod_data" are unchanged. Otherwise the local (KVM) copy is 
> > kept incorrect.
> I missread the code previously and I thought only under the condition
> the clocksource has been switched to another one will the KVM copy
> be touched. Apparently it is not the case because the copy should
> be updated on-time during normal tick, right?
> thanks for your reply,

Yes, it is updated during the normal tick, and mult/freq values change.

However, if none of them change, its not necessary to call the callback.
Perhaps you can check if any of the values changed and only 
invoke the callback in that case?

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

* Re: [PATCH][RFC] VM: x86: Return ealier if clocksource has not changed
  2016-12-27 15:32     ` Marcelo Tosatti
@ 2016-12-29  8:59       ` Chen Yu
  2016-12-29  9:57         ` Marcelo Tosatti
  0 siblings, 1 reply; 7+ messages in thread
From: Chen Yu @ 2016-12-29  8:59 UTC (permalink / raw)
  To: Marcelo Tosatti
  Cc: kvm, x86, linux-kernel, Rafael J. Wysocki, Len Brown,
	Paolo Bonzini, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, Wanpeng Li

On Tue, Dec 27, 2016 at 01:32:47PM -0200, Marcelo Tosatti wrote:
> On Tue, Dec 27, 2016 at 04:06:44PM +0800, Chen Yu wrote:
> > Hi Marcelo,
> > On Mon, Dec 26, 2016 at 05:44:25PM -0200, Marcelo Tosatti wrote:
> > > On Fri, Dec 23, 2016 at 04:41:53PM +0800, Chen Yu wrote:
> > > > Currently the notifier of pvclock_gtod_notify() get invoked
> > > > frequently due to the periodic update_wall_time(). This might
> > > > slow down the system a little bit as there might be redundant
> > > > execution code path and unnecessary lock contention
> > > > in update_pvclock_gtod(), which was found when I was doing
> > > > suspend/resume speed testings. As pvclock_gtod_notify()
> > > > should be invoked only when clocksource has changed, according to
> > > > Commit 16e8d74d2da9 ("KVM: x86: notifier for clocksource changes")
> > > > , either we can add a new notifier for clocksource switch,
> > > > or we can simply bypass the following code in pvclock_gtod_notify()
> > > > earlier if there is no clocksource switch.
> > > > 
> > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > Cc: "Radim Krcmar" <rkrcmar@redhat.com>
> > > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > Cc: Ingo Molnar <mingo@redhat.com>
> > > > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > > > Cc: Wanpeng Li <wanpeng.li@hotmail.com>
> > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > ---
> > > >  arch/x86/kvm/x86.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > index 445c51b..54aa32d 100644
> > > > --- a/arch/x86/kvm/x86.c
> > > > +++ b/arch/x86/kvm/x86.c
> > > > @@ -5961,13 +5961,14 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
> > > >  	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> > > >  	struct timekeeper *tk = priv;
> > > >  
> > > > +	if (likely(gtod->clock.vclock_mode == VCLOCK_TSC))
> > > > +		return 0;
> > > 
> > > I think this is only safe if any of the values in "struct
> > > pvclock_gtod_data" are unchanged. Otherwise the local (KVM) copy is 
> > > kept incorrect.
> > I missread the code previously and I thought only under the condition
> > the clocksource has been switched to another one will the KVM copy
> > be touched. Apparently it is not the case because the copy should
> > be updated on-time during normal tick, right?
> > thanks for your reply,
> 
> Yes, it is updated during the normal tick, and mult/freq values change.
> 
> However, if none of them change, its not necessary to call the callback.
> Perhaps you can check if any of the values changed and only 
> invoke the callback in that case?
>
Yes, this should be an optimization, but most of the callers(workload) come
from update_wall_time(), and in this code path the clock source's cycle
should already be updated in most cases, so this optimization should not take
much effect to reduce the burden I guess?

Thanks,
Yu

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

* Re: [PATCH][RFC] VM: x86: Return ealier if clocksource has not changed
  2016-12-29  8:59       ` Chen Yu
@ 2016-12-29  9:57         ` Marcelo Tosatti
  2016-12-29 13:54           ` Chen Yu
  0 siblings, 1 reply; 7+ messages in thread
From: Marcelo Tosatti @ 2016-12-29  9:57 UTC (permalink / raw)
  To: Chen Yu
  Cc: kvm, x86, linux-kernel, Rafael J. Wysocki, Len Brown,
	Paolo Bonzini, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, Wanpeng Li

On Thu, Dec 29, 2016 at 04:59:02PM +0800, Chen Yu wrote:
> On Tue, Dec 27, 2016 at 01:32:47PM -0200, Marcelo Tosatti wrote:
> > On Tue, Dec 27, 2016 at 04:06:44PM +0800, Chen Yu wrote:
> > > Hi Marcelo,
> > > On Mon, Dec 26, 2016 at 05:44:25PM -0200, Marcelo Tosatti wrote:
> > > > On Fri, Dec 23, 2016 at 04:41:53PM +0800, Chen Yu wrote:
> > > > > Currently the notifier of pvclock_gtod_notify() get invoked
> > > > > frequently due to the periodic update_wall_time(). This might
> > > > > slow down the system a little bit as there might be redundant
> > > > > execution code path and unnecessary lock contention
> > > > > in update_pvclock_gtod(), which was found when I was doing
> > > > > suspend/resume speed testings. As pvclock_gtod_notify()
> > > > > should be invoked only when clocksource has changed, according to
> > > > > Commit 16e8d74d2da9 ("KVM: x86: notifier for clocksource changes")
> > > > > , either we can add a new notifier for clocksource switch,
> > > > > or we can simply bypass the following code in pvclock_gtod_notify()
> > > > > earlier if there is no clocksource switch.
> > > > > 
> > > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > > Cc: "Radim Krcmar" <rkrcmar@redhat.com>
> > > > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > Cc: Ingo Molnar <mingo@redhat.com>
> > > > > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > > > > Cc: Wanpeng Li <wanpeng.li@hotmail.com>
> > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > > ---
> > > > >  arch/x86/kvm/x86.c | 5 +++--
> > > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > > index 445c51b..54aa32d 100644
> > > > > --- a/arch/x86/kvm/x86.c
> > > > > +++ b/arch/x86/kvm/x86.c
> > > > > @@ -5961,13 +5961,14 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
> > > > >  	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> > > > >  	struct timekeeper *tk = priv;
> > > > >  
> > > > > +	if (likely(gtod->clock.vclock_mode == VCLOCK_TSC))
> > > > > +		return 0;
> > > > 
> > > > I think this is only safe if any of the values in "struct
> > > > pvclock_gtod_data" are unchanged. Otherwise the local (KVM) copy is 
> > > > kept incorrect.
> > > I missread the code previously and I thought only under the condition
> > > the clocksource has been switched to another one will the KVM copy
> > > be touched. Apparently it is not the case because the copy should
> > > be updated on-time during normal tick, right?
> > > thanks for your reply,
> > 
> > Yes, it is updated during the normal tick, and mult/freq values change.
> > 
> > However, if none of them change, its not necessary to call the callback.
> > Perhaps you can check if any of the values changed and only 
> > invoke the callback in that case?
> >
> Yes, this should be an optimization, but most of the callers(workload) come
> from update_wall_time(), and in this code path the clock source's cycle
> should already be updated in most cases, so this optimization should not take
> much effect to reduce the burden I guess?
> 
> Thanks,
> Yu

I don't understand your reasoning.

"If the clock source parameters are already updated then optimization
does not make much effect".

If the clock source parameters are updated (that is there has been no
change in any of the values in pvclock_gtod_data), then you can skip
the callback. This case reduces the burden.

Right?

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

* Re: [PATCH][RFC] VM: x86: Return ealier if clocksource has not changed
  2016-12-29  9:57         ` Marcelo Tosatti
@ 2016-12-29 13:54           ` Chen Yu
  0 siblings, 0 replies; 7+ messages in thread
From: Chen Yu @ 2016-12-29 13:54 UTC (permalink / raw)
  To: Marcelo Tosatti
  Cc: kvm, x86, linux-kernel, Rafael J. Wysocki, Len Brown,
	Paolo Bonzini, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, Wanpeng Li

On Thu, Dec 29, 2016 at 07:57:33AM -0200, Marcelo Tosatti wrote:
> On Thu, Dec 29, 2016 at 04:59:02PM +0800, Chen Yu wrote:
> > On Tue, Dec 27, 2016 at 01:32:47PM -0200, Marcelo Tosatti wrote:
> > > On Tue, Dec 27, 2016 at 04:06:44PM +0800, Chen Yu wrote:
> > > > Hi Marcelo,
> > > > On Mon, Dec 26, 2016 at 05:44:25PM -0200, Marcelo Tosatti wrote:
> > > > > On Fri, Dec 23, 2016 at 04:41:53PM +0800, Chen Yu wrote:
> > > > > > Currently the notifier of pvclock_gtod_notify() get invoked
> > > > > > frequently due to the periodic update_wall_time(). This might
> > > > > > slow down the system a little bit as there might be redundant
> > > > > > execution code path and unnecessary lock contention
> > > > > > in update_pvclock_gtod(), which was found when I was doing
> > > > > > suspend/resume speed testings. As pvclock_gtod_notify()
> > > > > > should be invoked only when clocksource has changed, according to
> > > > > > Commit 16e8d74d2da9 ("KVM: x86: notifier for clocksource changes")
> > > > > > , either we can add a new notifier for clocksource switch,
> > > > > > or we can simply bypass the following code in pvclock_gtod_notify()
> > > > > > earlier if there is no clocksource switch.
> > > > > > 
> > > > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > > > Cc: "Radim Krcmar" <rkrcmar@redhat.com>
> > > > > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > > Cc: Ingo Molnar <mingo@redhat.com>
> > > > > > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > > > > > Cc: Wanpeng Li <wanpeng.li@hotmail.com>
> > > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > > > > ---
> > > > > >  arch/x86/kvm/x86.c | 5 +++--
> > > > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > > > index 445c51b..54aa32d 100644
> > > > > > --- a/arch/x86/kvm/x86.c
> > > > > > +++ b/arch/x86/kvm/x86.c
> > > > > > @@ -5961,13 +5961,14 @@ static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
> > > > > >  	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> > > > > >  	struct timekeeper *tk = priv;
> > > > > >  
> > > > > > +	if (likely(gtod->clock.vclock_mode == VCLOCK_TSC))
> > > > > > +		return 0;
> > > > > 
> > > > > I think this is only safe if any of the values in "struct
> > > > > pvclock_gtod_data" are unchanged. Otherwise the local (KVM) copy is 
> > > > > kept incorrect.
> > > > I missread the code previously and I thought only under the condition
> > > > the clocksource has been switched to another one will the KVM copy
> > > > be touched. Apparently it is not the case because the copy should
> > > > be updated on-time during normal tick, right?
> > > > thanks for your reply,
> > > 
> > > Yes, it is updated during the normal tick, and mult/freq values change.
> > > 
> > > However, if none of them change, its not necessary to call the callback.
> > > Perhaps you can check if any of the values changed and only 
> > > invoke the callback in that case?
> > >
> > Yes, this should be an optimization, but most of the callers(workload) come
> > from update_wall_time(), and in this code path the clock source's cycle
> > should already be updated in most cases, so this optimization should not take
> > much effect to reduce the burden I guess?
> > 
> > Thanks,
> > Yu
> 
> I don't understand your reasoning.
> 
> "If the clock source parameters are already updated then optimization
> does not make much effect".
> 
> If the clock source parameters are updated (that is there has been no
> change in any of the values in pvclock_gtod_data), then you can skip
> the callback. This case reduces the burden.
> 
> Right?
> 
> 
Yes, in general case we can improve the code logic.
Previously I was thinking of the case I encountered:

1. There are quite some invokes of pvclock_gtod_notify() caught by ftrace, and
   most of them should be triggered by update_wall_time()
2. If we optimize the code not to invoke pvclock_gtod_notify() if there is no
   modification of pvclock_gtod_data, it will reduce the burden for general use
   cases, but not for update_wall_time(), as it has already been modified
   in update_wall_time.

But yes, you are right, above is just my scenario, the optimization you mentioned
is a generic solution for most cases. Do you mean the following solution?

Index: linux/kernel/time/timekeeping.c
===================================================================
--- linux.orig/kernel/time/timekeeping.c
+++ linux/kernel/time/timekeeping.c
@@ -527,7 +527,13 @@ static RAW_NOTIFIER_HEAD(pvclock_gtod_ch
 
 static void update_pvclock_gtod(struct timekeeper *tk, bool was_set)
 {
-	raw_notifier_call_chain(&pvclock_gtod_chain, was_set, tk);
+	static struct timekeeper prev_timekeeper;
+
+	/* Only notify if the clocksource has changed.*/
+	if (memcmp(tk, &prev_timekeeper, sizeof(struct timekeeper))) {
+		raw_notifier_call_chain(&pvclock_gtod_chain, was_set, tk);
+		memcpy(&prev_timekeeper, tk, sizeof(struct timekeeper));
+	}
 }
 
 /**

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

end of thread, other threads:[~2016-12-29 13:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-23  8:41 [PATCH][RFC] VM: x86: Return ealier if clocksource has not changed Chen Yu
2016-12-26 19:44 ` Marcelo Tosatti
2016-12-27  8:06   ` Chen Yu
2016-12-27 15:32     ` Marcelo Tosatti
2016-12-29  8:59       ` Chen Yu
2016-12-29  9:57         ` Marcelo Tosatti
2016-12-29 13:54           ` Chen Yu

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).