Re: [Xen-devel] [PATCH 1/1] xen/time: do not decrease steal time after live migration on xen
On 10/20/2017 01:37 AM, Dongli Zhang wrote:
> Hi Boris,
>
> - boris.ostrov...@oracle.com wrote:
>
>> On 10/19/2017 04:02 AM, Dongli Zhang wrote:
>>> After guest live migration on xen, steal time in /proc/stat
>>> (cpustat[CPUTIME_STEAL]) might decrease because steal returned by
>>> xen_steal_lock() might be less than this_rq()->prev_steal_time which
>> is
>>> derived from previous return value of xen_steal_clock().
>>>
>>> For instance, steal time of each vcpu is 335 before live migration.
>>>
>>> cpu 198 0 368 200064 1962 0 0 1340 0 0
>>> cpu0 38 0 81 50063 492 0 0 335 0 0
>>> cpu1 65 0 97 49763 634 0 0 335 0 0
>>> cpu2 38 0 81 50098 462 0 0 335 0 0
>>> cpu3 56 0 107 50138 374 0 0 335 0 0
>>>
>>> After live migration, steal time is reduced to 312.
>>>
>>> cpu 200 0 370 200330 1971 0 0 1248 0 0
>>> cpu0 38 0 82 50123 500 0 0 312 0 0
>>> cpu1 65 0 97 49832 634 0 0 312 0 0
>>> cpu2 39 0 82 50167 462 0 0 312 0 0
>>> cpu3 56 0 107 50207 374 0 0 312 0 0
>>>
>>> The code in this patch is borrowed from do_stolen_accounting() which
>> has
>>> already been removed from linux source code since commit
>> ecb23dc6f2ef
>>> ("xen: add steal_clock support on x86"). The core idea of both
>>> do_stolen_accounting() and this patch is to avoid accounting new
>> steal
>>> clock if it is smaller than previous old steal clock.
>>>
>>> Similar and more severe issue would impact prior linux 4.8-4.10 as
>>> discussed by Michael Las at
>>>
>> https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest,
>>> which would overflow steal time and lead to 100% st usage in top
>> command
>>> for linux 4.8-4.10. A backport of this patch would fix that issue.
>>>
>>> References:
>> https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest
>>> Signed-off-by: Dongli Zhang
>>> ---
>>> drivers/xen/time.c | 15 ++-
>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/xen/time.c b/drivers/xen/time.c
>>> index ac5f23f..2b3a996 100644
>>> --- a/drivers/xen/time.c
>>> +++ b/drivers/xen/time.c
>>> @@ -19,6 +19,8 @@
>>> /* runstate info updated by Xen */
>>> static DEFINE_PER_CPU(struct vcpu_runstate_info, xen_runstate);
>>>
>>> +static DEFINE_PER_CPU(u64, xen_old_steal);
>>> +
>>> /* return an consistent snapshot of 64-bit time/counter value */
>>> static u64 get64(const u64 *p)
>>> {
>>> @@ -83,9 +85,20 @@ bool xen_vcpu_stolen(int vcpu)
>>> u64 xen_steal_clock(int cpu)
>>> {
>>> struct vcpu_runstate_info state;
>>> + u64 xen_new_steal;
>>> + s64 steal_delta;
>>>
>>> xen_get_runstate_snapshot_cpu(, cpu);
>>> - return state.time[RUNSTATE_runnable] +
>> state.time[RUNSTATE_offline];
>>> + xen_new_steal = state.time[RUNSTATE_runnable]
>>> + + state.time[RUNSTATE_offline];
>>> + steal_delta = xen_new_steal - per_cpu(xen_old_steal, cpu);
>>> +
>>> + if (steal_delta < 0)
>>> + xen_new_steal = per_cpu(xen_old_steal, cpu);
>>> + else
>>> + per_cpu(xen_old_steal, cpu) = xen_new_steal;
>>> +
>>> + return xen_new_steal;
>>> }
>>>
>>> void xen_setup_runstate_info(int cpu)
>> Can we stash state.time[] during suspend and then add stashed values
>> inside xen_get_runstate_snapshot_cpu()?
>
> Would you like to stash state.time[] during do_suspend() (or xen_suspend()) or
> code below is expected:
>
> -
>
> --- a/drivers/xen/time.c
> +++ b/drivers/xen/time.c
> @@ -19,6 +19,8 @@
> /* runstate info updated by Xen */
> static DEFINE_PER_CPU(struct vcpu_runstate_info, xen_runstate);
>
> +static DEFINE_PER_CPU(u64[4], old_runstate_time);
> +
> /* return an consistent snapshot of 64-bit time/counter value */
> static u64 get64(const u64 *p)
> {
> @@ -52,6 +54,8 @@ static void xen_get_runstate_snapshot_cpu(struct
> vcpu_runstate_info *res,
> {
> u64 state_time;
> struct vcpu_runstate_info *state;
> + int i;
> + s64 time_delta;
>
> BUG_ON(preemptible());
>
> @@ -64,6 +68,17 @@ static void xen_get_runstate_snapshot_cpu(struct
> vcpu_runstate_info *res,
> rmb(); /* Hypervisor might update data. */
> } while (get64(>state_entry_time) != state_time ||
>(state_time & XEN_RUNSTATE_UPDATE));
> +
> + for (i = 0; i < 4; i++) {
> + if (i == RUNSTATE_runnable || i == RUNSTATE_offline) {
> + time_delta = res->time[i] - per_cpu(old_runstate_time,
> cpu)[i];
> +
> + if (unlikely(time_delta < 0))
> + res->time[i] = per_cpu(old_runstate_time,
> cpu)[i];
> + else
> + per_cpu(old_runstate_time, cpu)[i] =
> res->time[i];
> + }
> + }
> }
I was thinking more along the lines of
for (i=0; i<4; i++)
res->time[i] += per_cpu(old_runstate_time, cpu)[i];
time[] array is cumulative and AFAIUI is
Re: [Xen-devel] [PATCH 1/1] xen/time: do not decrease steal time after live migration on xen
Hi Boris,
- boris.ostrov...@oracle.com wrote:
> On 10/19/2017 04:02 AM, Dongli Zhang wrote:
> > After guest live migration on xen, steal time in /proc/stat
> > (cpustat[CPUTIME_STEAL]) might decrease because steal returned by
> > xen_steal_lock() might be less than this_rq()->prev_steal_time which
> is
> > derived from previous return value of xen_steal_clock().
> >
> > For instance, steal time of each vcpu is 335 before live migration.
> >
> > cpu 198 0 368 200064 1962 0 0 1340 0 0
> > cpu0 38 0 81 50063 492 0 0 335 0 0
> > cpu1 65 0 97 49763 634 0 0 335 0 0
> > cpu2 38 0 81 50098 462 0 0 335 0 0
> > cpu3 56 0 107 50138 374 0 0 335 0 0
> >
> > After live migration, steal time is reduced to 312.
> >
> > cpu 200 0 370 200330 1971 0 0 1248 0 0
> > cpu0 38 0 82 50123 500 0 0 312 0 0
> > cpu1 65 0 97 49832 634 0 0 312 0 0
> > cpu2 39 0 82 50167 462 0 0 312 0 0
> > cpu3 56 0 107 50207 374 0 0 312 0 0
> >
> > The code in this patch is borrowed from do_stolen_accounting() which
> has
> > already been removed from linux source code since commit
> ecb23dc6f2ef
> > ("xen: add steal_clock support on x86"). The core idea of both
> > do_stolen_accounting() and this patch is to avoid accounting new
> steal
> > clock if it is smaller than previous old steal clock.
> >
> > Similar and more severe issue would impact prior linux 4.8-4.10 as
> > discussed by Michael Las at
> >
> https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest,
> > which would overflow steal time and lead to 100% st usage in top
> command
> > for linux 4.8-4.10. A backport of this patch would fix that issue.
> >
> > References:
> https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest
> > Signed-off-by: Dongli Zhang
> > ---
> > drivers/xen/time.c | 15 ++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/xen/time.c b/drivers/xen/time.c
> > index ac5f23f..2b3a996 100644
> > --- a/drivers/xen/time.c
> > +++ b/drivers/xen/time.c
> > @@ -19,6 +19,8 @@
> > /* runstate info updated by Xen */
> > static DEFINE_PER_CPU(struct vcpu_runstate_info, xen_runstate);
> >
> > +static DEFINE_PER_CPU(u64, xen_old_steal);
> > +
> > /* return an consistent snapshot of 64-bit time/counter value */
> > static u64 get64(const u64 *p)
> > {
> > @@ -83,9 +85,20 @@ bool xen_vcpu_stolen(int vcpu)
> > u64 xen_steal_clock(int cpu)
> > {
> > struct vcpu_runstate_info state;
> > + u64 xen_new_steal;
> > + s64 steal_delta;
> >
> > xen_get_runstate_snapshot_cpu(, cpu);
> > - return state.time[RUNSTATE_runnable] +
> state.time[RUNSTATE_offline];
> > + xen_new_steal = state.time[RUNSTATE_runnable]
> > + + state.time[RUNSTATE_offline];
> > + steal_delta = xen_new_steal - per_cpu(xen_old_steal, cpu);
> > +
> > + if (steal_delta < 0)
> > + xen_new_steal = per_cpu(xen_old_steal, cpu);
> > + else
> > + per_cpu(xen_old_steal, cpu) = xen_new_steal;
> > +
> > + return xen_new_steal;
> > }
> >
> > void xen_setup_runstate_info(int cpu)
>
> Can we stash state.time[] during suspend and then add stashed values
> inside xen_get_runstate_snapshot_cpu()?
Would you like to stash state.time[] during do_suspend() (or xen_suspend()) or
code below is expected:
-
--- a/drivers/xen/time.c
+++ b/drivers/xen/time.c
@@ -19,6 +19,8 @@
/* runstate info updated by Xen */
static DEFINE_PER_CPU(struct vcpu_runstate_info, xen_runstate);
+static DEFINE_PER_CPU(u64[4], old_runstate_time);
+
/* return an consistent snapshot of 64-bit time/counter value */
static u64 get64(const u64 *p)
{
@@ -52,6 +54,8 @@ static void xen_get_runstate_snapshot_cpu(struct
vcpu_runstate_info *res,
{
u64 state_time;
struct vcpu_runstate_info *state;
+ int i;
+ s64 time_delta;
BUG_ON(preemptible());
@@ -64,6 +68,17 @@ static void xen_get_runstate_snapshot_cpu(struct
vcpu_runstate_info *res,
rmb(); /* Hypervisor might update data. */
} while (get64(>state_entry_time) != state_time ||
(state_time & XEN_RUNSTATE_UPDATE));
+
+ for (i = 0; i < 4; i++) {
+ if (i == RUNSTATE_runnable || i == RUNSTATE_offline) {
+ time_delta = res->time[i] - per_cpu(old_runstate_time,
cpu)[i];
+
+ if (unlikely(time_delta < 0))
+ res->time[i] = per_cpu(old_runstate_time,
cpu)[i];
+ else
+ per_cpu(old_runstate_time, cpu)[i] =
res->time[i];
+ }
+ }
}
-
Thank you very much!
Dongli Zhang
>
> This will make xen_steal_clock() simpler.
>
> -boris
>
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
>
Re: [Xen-devel] [PATCH 1/1] xen/time: do not decrease steal time after live migration on xen
On 10/19/2017 04:02 AM, Dongli Zhang wrote:
> After guest live migration on xen, steal time in /proc/stat
> (cpustat[CPUTIME_STEAL]) might decrease because steal returned by
> xen_steal_lock() might be less than this_rq()->prev_steal_time which is
> derived from previous return value of xen_steal_clock().
>
> For instance, steal time of each vcpu is 335 before live migration.
>
> cpu 198 0 368 200064 1962 0 0 1340 0 0
> cpu0 38 0 81 50063 492 0 0 335 0 0
> cpu1 65 0 97 49763 634 0 0 335 0 0
> cpu2 38 0 81 50098 462 0 0 335 0 0
> cpu3 56 0 107 50138 374 0 0 335 0 0
>
> After live migration, steal time is reduced to 312.
>
> cpu 200 0 370 200330 1971 0 0 1248 0 0
> cpu0 38 0 82 50123 500 0 0 312 0 0
> cpu1 65 0 97 49832 634 0 0 312 0 0
> cpu2 39 0 82 50167 462 0 0 312 0 0
> cpu3 56 0 107 50207 374 0 0 312 0 0
>
> The code in this patch is borrowed from do_stolen_accounting() which has
> already been removed from linux source code since commit ecb23dc6f2ef
> ("xen: add steal_clock support on x86"). The core idea of both
> do_stolen_accounting() and this patch is to avoid accounting new steal
> clock if it is smaller than previous old steal clock.
>
> Similar and more severe issue would impact prior linux 4.8-4.10 as
> discussed by Michael Las at
> https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest,
> which would overflow steal time and lead to 100% st usage in top command
> for linux 4.8-4.10. A backport of this patch would fix that issue.
>
> References:
> https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest
> Signed-off-by: Dongli Zhang
> ---
> drivers/xen/time.c | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/xen/time.c b/drivers/xen/time.c
> index ac5f23f..2b3a996 100644
> --- a/drivers/xen/time.c
> +++ b/drivers/xen/time.c
> @@ -19,6 +19,8 @@
> /* runstate info updated by Xen */
> static DEFINE_PER_CPU(struct vcpu_runstate_info, xen_runstate);
>
> +static DEFINE_PER_CPU(u64, xen_old_steal);
> +
> /* return an consistent snapshot of 64-bit time/counter value */
> static u64 get64(const u64 *p)
> {
> @@ -83,9 +85,20 @@ bool xen_vcpu_stolen(int vcpu)
> u64 xen_steal_clock(int cpu)
> {
> struct vcpu_runstate_info state;
> + u64 xen_new_steal;
> + s64 steal_delta;
>
> xen_get_runstate_snapshot_cpu(, cpu);
> - return state.time[RUNSTATE_runnable] + state.time[RUNSTATE_offline];
> + xen_new_steal = state.time[RUNSTATE_runnable]
> + + state.time[RUNSTATE_offline];
> + steal_delta = xen_new_steal - per_cpu(xen_old_steal, cpu);
> +
> + if (steal_delta < 0)
> + xen_new_steal = per_cpu(xen_old_steal, cpu);
> + else
> + per_cpu(xen_old_steal, cpu) = xen_new_steal;
> +
> + return xen_new_steal;
> }
>
> void xen_setup_runstate_info(int cpu)
Can we stash state.time[] during suspend and then add stashed values
inside xen_get_runstate_snapshot_cpu()?
This will make xen_steal_clock() simpler.
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/1] xen/time: do not decrease steal time after live migration on xen
After guest live migration on xen, steal time in /proc/stat
(cpustat[CPUTIME_STEAL]) might decrease because steal returned by
xen_steal_lock() might be less than this_rq()->prev_steal_time which is
derived from previous return value of xen_steal_clock().
For instance, steal time of each vcpu is 335 before live migration.
cpu 198 0 368 200064 1962 0 0 1340 0 0
cpu0 38 0 81 50063 492 0 0 335 0 0
cpu1 65 0 97 49763 634 0 0 335 0 0
cpu2 38 0 81 50098 462 0 0 335 0 0
cpu3 56 0 107 50138 374 0 0 335 0 0
After live migration, steal time is reduced to 312.
cpu 200 0 370 200330 1971 0 0 1248 0 0
cpu0 38 0 82 50123 500 0 0 312 0 0
cpu1 65 0 97 49832 634 0 0 312 0 0
cpu2 39 0 82 50167 462 0 0 312 0 0
cpu3 56 0 107 50207 374 0 0 312 0 0
The code in this patch is borrowed from do_stolen_accounting() which has
already been removed from linux source code since commit ecb23dc6f2ef
("xen: add steal_clock support on x86"). The core idea of both
do_stolen_accounting() and this patch is to avoid accounting new steal
clock if it is smaller than previous old steal clock.
Similar and more severe issue would impact prior linux 4.8-4.10 as
discussed by Michael Las at
https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest,
which would overflow steal time and lead to 100% st usage in top command
for linux 4.8-4.10. A backport of this patch would fix that issue.
References:
https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest
Signed-off-by: Dongli Zhang
---
drivers/xen/time.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/xen/time.c b/drivers/xen/time.c
index ac5f23f..2b3a996 100644
--- a/drivers/xen/time.c
+++ b/drivers/xen/time.c
@@ -19,6 +19,8 @@
/* runstate info updated by Xen */
static DEFINE_PER_CPU(struct vcpu_runstate_info, xen_runstate);
+static DEFINE_PER_CPU(u64, xen_old_steal);
+
/* return an consistent snapshot of 64-bit time/counter value */
static u64 get64(const u64 *p)
{
@@ -83,9 +85,20 @@ bool xen_vcpu_stolen(int vcpu)
u64 xen_steal_clock(int cpu)
{
struct vcpu_runstate_info state;
+ u64 xen_new_steal;
+ s64 steal_delta;
xen_get_runstate_snapshot_cpu(, cpu);
- return state.time[RUNSTATE_runnable] + state.time[RUNSTATE_offline];
+ xen_new_steal = state.time[RUNSTATE_runnable]
+ + state.time[RUNSTATE_offline];
+ steal_delta = xen_new_steal - per_cpu(xen_old_steal, cpu);
+
+ if (steal_delta < 0)
+ xen_new_steal = per_cpu(xen_old_steal, cpu);
+ else
+ per_cpu(xen_old_steal, cpu) = xen_new_steal;
+
+ return xen_new_steal;
}
void xen_setup_runstate_info(int cpu)
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
