Re: [PATCH 08/12] xen/time: introduce xen_{save,restore}_steal_clock

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
To: Anchal Agarwal <anchalag@amazon.com>,
	tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
	hpa@zytor.com, x86@kernel.org, jgross@suse.com,
	linux-pm@vger.kernel.org, linux-mm@kvack.org, kamatam@amazon.com,
	sstabellini@kernel.org, konrad.wilk@oracle.com,
	roger.pau@citrix.com, axboe@kernel.dk, davem@davemloft.net,
	rjw@rjwysocki.net, len.brown@intel.com, pavel@ucw.cz,
	peterz@infradead.org, eduval@amazon.com, sblbir@amazon.com,
	xen-devel@lists.xenproject.org, vkuznets@redhat.com,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	dwmw@amazon.co.uk, benh@kernel.crashing.org
Subject: Re: [PATCH 08/12] xen/time: introduce xen_{save,restore}_steal_clock
Date: Sat, 30 May 2020 19:32:59 -0400	[thread overview]
Message-ID: <5e1094c5-f6c3-c83d-d86f-0bbeaa9f2086@oracle.com> (raw)
In-Reply-To: <ae90ece495d29f54fc9986a07f45ab6659136573.1589926004.git.anchalag@amazon.com>

On 5/19/20 7:28 PM, Anchal Agarwal wrote:
> From: Munehisa Kamata <kamatam@amazon.com>
>
> Currently, steal time accounting code in scheduler expects steal clock
> callback to provide monotonically increasing value. If the accounting
> code receives a smaller value than previous one, it uses a negative
> value to calculate steal time and results in incorrectly updated idle
> and steal time accounting. This breaks userspace tools which read
> /proc/stat.
>
> top - 08:05:35 up  2:12,  3 users,  load average: 0.00, 0.07, 0.23
> Tasks:  80 total,   1 running,  79 sleeping,   0 stopped,   0 zombie
> Cpu(s):  0.0%us,  0.0%sy,  0.0%ni,30100.0%id,  0.0%wa,  0.0%hi, 0.0%si,-1253874204672.0%st
>
> This can actually happen when a Xen PVHVM guest gets restored from
> hibernation, because such a restored guest is just a fresh domain from
> Xen perspective and the time information in runstate info starts over
> from scratch.
>
> This patch introduces xen_save_steal_clock() which saves current values
> in runstate info into per-cpu variables. Its couterpart,
> xen_restore_steal_clock(), sets offset if it found the current values in
> runstate info are smaller than previous ones. xen_steal_clock() is also
> modified to use the offset to ensure that scheduler only sees
> monotonically increasing number.
>
> Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
> Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
> ---
>  drivers/xen/time.c    | 29 ++++++++++++++++++++++++++++-
>  include/xen/xen-ops.h |  2 ++
>  2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/xen/time.c b/drivers/xen/time.c
> index 0968859c29d0..3560222cc0dd 100644
> --- a/drivers/xen/time.c
> +++ b/drivers/xen/time.c
> @@ -23,6 +23,9 @@ static DEFINE_PER_CPU(struct vcpu_runstate_info, xen_runstate);
>  
>  static DEFINE_PER_CPU(u64[4], old_runstate_time);
>  
> +static DEFINE_PER_CPU(u64, xen_prev_steal_clock);
> +static DEFINE_PER_CPU(u64, xen_steal_clock_offset);

Can you use old_runstate_time here? It is used to solve a similar
problem for pv suspend, isn't it?

-boris