Re: [Xen-devel] [PATCH v4 1/1] xen/time: do not decrease steal time after live migration on xen

2017-11-01 Thread kbuild test robot 
Hi Dongli,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on xen-tip/linux-next]
[also build test ERROR on v4.14-rc7 next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Dongli-Zhang/xen-time-do-not-decrease-steal-time-after-live-migration-on-xen/20171102-025719
base:   https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git linux-next
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   drivers/xen/time.c: In function 'xen_accumulate_runstate_time':
   drivers/xen/time.c:92:25: error: implicit declaration of function 'kcalloc' 
[-Werror=implicit-function-declaration]
  runstate_time_delta = kcalloc(num_possible_cpus(),
^~~
   drivers/xen/time.c:92:23: warning: assignment makes pointer from integer 
without a cast [-Wint-conversion]
  runstate_time_delta = kcalloc(num_possible_cpus(),
  ^
>> drivers/xen/time.c:102:31: error: implicit declaration of function 
>> 'kmalloc_array' [-Werror=implicit-function-declaration]
   runstate_time_delta[cpu] = kmalloc_array(RUNSTATE_max,
  ^
   drivers/xen/time.c:102:29: warning: assignment makes pointer from integer 
without a cast [-Wint-conversion]
   runstate_time_delta[cpu] = kmalloc_array(RUNSTATE_max,
^
   drivers/xen/time.c:141:5: error: implicit declaration of function 'kfree' 
[-Werror=implicit-function-declaration]
kfree(runstate_time_delta[cpu]);
^
   cc1: some warnings being treated as errors

vim +/kmalloc_array +102 drivers/xen/time.c

82  
83  void xen_accumulate_runstate_time(int action)
84  {
85  struct vcpu_runstate_info state;
86  int cpu, i;
87  
88  switch (action) {
89  case -1: /* backup runstate time before suspend */
90  WARN_ON_ONCE(unlikely(runstate_time_delta));
91  
  > 92  runstate_time_delta = kcalloc(num_possible_cpus(),
93
sizeof(*runstate_time_delta),
94GFP_KERNEL);
95  if (unlikely(!runstate_time_delta)) {
96  pr_alert("%s: failed to allocate 
runstate_time_delta\n",
97  __func__);
98  return;
99  }
   100  
   101  for_each_possible_cpu(cpu) {
 > 102  runstate_time_delta[cpu] = 
 > kmalloc_array(RUNSTATE_max,
   103
sizeof(**runstate_time_delta),
   104GFP_KERNEL);
   105  if (unlikely(!runstate_time_delta[cpu])) {
   106  pr_alert("%s: failed to allocate 
runstate_time_delta[%d]\n",
   107  __func__, cpu);
   108  action = 0;
   109  goto reclaim_mem;
   110  }
   111  
   112  xen_get_runstate_snapshot_cpu_delta(, 
cpu);
   113  memcpy(runstate_time_delta[cpu],
   114  state.time,
   115  RUNSTATE_max * 
sizeof(**runstate_time_delta));
   116  }
   117  break;
   118  
   119  case 0: /* backup runstate time after resume */
   120  if (unlikely(!runstate_time_delta)) {
   121  pr_alert("%s: cannot accumulate runstate time 
as runstate_time_delta is NULL\n",
   122  __func__);
   123  return;
   124  }
   125  
   126  for_each_possible_cpu(cpu) {
   127  for (i = 0; i < RUNSTATE_max; i++)
   128  per_cpu(old_runstate_time, cpu)[i] +=
   129  
runstate_time_delta[cpu][i];
   130  }
   131  break;
   132  
   133  default: /* do not accumulate runstate time for checkpointing */
   134  break;
   135  }
   136  
   137  reclaim_mem:
   138  if (action != -1 && runstate_time_delta) {
   139  for_each_possible_cpu(cpu) {
   140

Re: [Xen-devel] [PATCH v4 1/1] xen/time: do not decrease steal time after live migration on xen

2017-10-30 Thread Juergen Gross 
On 30/10/17 06:26, 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
> 
> Since runstate times are cumulative and cleared during xen live migration
> by xen hypervisor, the idea of this patch is to accumulate runstate times
> to global percpu variables before live migration suspend. Once guest VM is
> resumed, xen_get_runstate_snapshot_cpu() would always return the sum of new
> runstate times and previously accumulated times stored in global percpu
> variables.
> 
> 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 
> 
> ---
> Changed since v1:
>   * relocate modification to xen_get_runstate_snapshot_cpu
> 
> Changed since v2:
>   * accumulate runstate times before live migration
> 
> Changed since v3:
>   * do not accumulate times in the case of guest checkpointing
> 
> ---
>  drivers/xen/manage.c |  2 ++
>  drivers/xen/time.c   | 83 
> ++--
>  include/xen/interface/vcpu.h |  2 ++
>  include/xen/xen-ops.h|  1 +
>  4 files changed, 86 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index c425d03..3dc085d 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -72,6 +72,7 @@ static int xen_suspend(void *data)
>   }
>  
>   gnttab_suspend();
> + xen_accumulate_runstate_time(-1);
>   xen_arch_pre_suspend();
>  
>   /*
> @@ -84,6 +85,7 @@ static int xen_suspend(void *data)
> : 0);
>  
>   xen_arch_post_suspend(si->cancelled);
> + xen_accumulate_runstate_time(si->cancelled);
>   gnttab_resume();
>  
>   if (!si->cancelled) {
> diff --git a/drivers/xen/time.c b/drivers/xen/time.c
> index ac5f23f..18e2b76 100644
> --- a/drivers/xen/time.c
> +++ b/drivers/xen/time.c
> @@ -19,6 +19,9 @@
>  /* runstate info updated by Xen */
>  static DEFINE_PER_CPU(struct vcpu_runstate_info, xen_runstate);
>  
> +static DEFINE_PER_CPU(u64[RUNSTATE_max], old_runstate_time);
> +static u64 **runstate_time_delta;
> +
>  /* return an consistent snapshot of 64-bit time/counter value */
>  static u64 get64(const u64 *p)
>  {
> @@ -47,8 +50,8 @@ static u64 get64(const u64 *p)
>   return ret;
>  }
>  
> -static void xen_get_runstate_snapshot_cpu(struct vcpu_runstate_info *res,
> -   unsigned int cpu)
> +static void xen_get_runstate_snapshot_cpu_delta(
> + struct vcpu_runstate_info *res, unsigned int cpu)
>  {
>   u64 state_time;
>   struct vcpu_runstate_info *state;
> @@ -66,6 +69,82 @@ static void xen_get_runstate_snapshot_cpu(struct 
> vcpu_runstate_info *res,
>(state_time & XEN_RUNSTATE_UPDATE));
>  }
>  
> +static void xen_get_runstate_snapshot_cpu(struct vcpu_runstate_info *res,
> +   unsigned int cpu)
> +{
> + int i;
> +
> + xen_get_runstate_snapshot_cpu_delta(res, cpu);
> +
> + for (i = 0; i < RUNSTATE_max; i++)
> + res->time[i] += per_cpu(old_runstate_time, cpu)[i];
> +}
> +
> +void xen_accumulate_runstate_time(int action)
> +{
> + struct vcpu_runstate_info state;
> + int cpu, i;
> +
> + switch (action) {
> + case -1: /* backup runstate time before suspend */
> + WARN_ON_ONCE(unlikely(runstate_time_delta));
> +
> + runstate_time_delta = kcalloc(num_possible_cpus(),
> +   sizeof(*runstate_time_delta),
> +   GFP_KERNEL);

You know the number of cpus, so you can just allocate an array of
struct vcpu_runstate_info:

struct vcpu_runstate_info *runstate_time_delta;

kcalloc(num_possible_cpus(), sizeof(*runstate_time_delta), GFP_KERNEL);

then ...

> + if