Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1458389 > unrolled thread

Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time

Started byWanpeng Li <kernellwp@gmail.com>
First post2016-08-09 06:00 +0200
Last post2016-08-10 23:30 +0200
Articles 10 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time Wanpeng Li <kernellwp@gmail.com> - 2016-08-09 06:00 +0200
    Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq  time Rik van Riel <riel@redhat.com> - 2016-08-09 16:10 +0200
      Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time Wanpeng Li <kernellwp@gmail.com> - 2016-08-10 01:10 +0200
        Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time Wanpeng Li <kernellwp@gmail.com> - 2016-08-10 21:30 +0200
      Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time Wanpeng Li <kernellwp@gmail.com> - 2016-08-10 01:30 +0200
        Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time Wanpeng Li <kernellwp@gmail.com> - 2016-08-10 01:40 +0200
        Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time Wanpeng Li <kernellwp@gmail.com> - 2016-08-10 01:40 +0200
          [PATCH] time,virt: resync steal time when guest & host lose sync Rik van Riel <riel@redhat.com> - 2016-08-10 20:10 +0200
          Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time Wanpeng Li <kernellwp@gmail.com> - 2016-08-10 22:20 +0200
          Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq  time Rik van Riel <riel@redhat.com> - 2016-08-10 23:30 +0200

#1458389 — Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time

FromWanpeng Li <kernellwp@gmail.com>
Date2016-08-09 06:00 +0200
SubjectRe: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time
Message-ID<s46mR-2aM-1@gated-at.bofh.it>
Hi Rik,
2016-07-13 22:50 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.com>:
> From: Rik van Riel <riel@redhat.com>
>
> Currently, if there was any irq or softirq time during 'ticks'
> jiffies, the entire period will be accounted as irq or softirq
> time.
>
> This is inaccurate if only a subset of the time was actually spent
> handling irqs, and could conceivably mis-count all of the ticks during
> a period as irq time, when there was some irq and some softirq time.
>
> This can actually happen when irqtime_account_process_tick is called
> from account_idle_ticks, which can pass a larger number of ticks down
> all at once.
>
> Fix this by changing irqtime_account_hi_update, irqtime_account_si_update,
> and steal_account_process_ticks to work with cputime_t time units, and
> return the amount of time spent in each mode.

Do we need to minus st cputime from idle cputime in
account_idle_ticks() when noirqtime is true? I try to add this logic
w/ noirqtime and idle=poll boot parameter for a full dynticks guest,
however, there is no difference, where I miss?

Regards,
Wanpeng Li

[toc] | [next] | [standalone]


#1458784 — Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time

FromRik van Riel <riel@redhat.com>
Date2016-08-09 16:10 +0200
SubjectRe: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time
Message-ID<s4fTc-bN-29@gated-at.bofh.it>
In reply to#1458389

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2016-08-09 at 11:59 +0800, Wanpeng Li wrote:
> Hi Rik,
> 2016-07-13 22:50 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.com>:
> > From: Rik van Riel <riel@redhat.com>
> > 
> > Currently, if there was any irq or softirq time during 'ticks'
> > jiffies, the entire period will be accounted as irq or softirq
> > time.
> > 
> > This is inaccurate if only a subset of the time was actually spent
> > handling irqs, and could conceivably mis-count all of the ticks
> > during
> > a period as irq time, when there was some irq and some softirq
> > time.
> > 
> > This can actually happen when irqtime_account_process_tick is
> > called
> > from account_idle_ticks, which can pass a larger number of ticks
> > down
> > all at once.
> > 
> > Fix this by changing irqtime_account_hi_update,
> > irqtime_account_si_update,
> > and steal_account_process_ticks to work with cputime_t time units,
> > and
> > return the amount of time spent in each mode.
> 
> Do we need to minus st cputime from idle cputime in
> account_idle_ticks() when noirqtime is true? I try to add this logic
> w/ noirqtime and idle=poll boot parameter for a full dynticks guest,
> however, there is no difference, where I miss?

Yes, you are right. The code in account_idle_ticks()
could use the same treatment.

I am not sure why it would not work, though...

-- 

All Rights Reversed.

[toc] | [prev] | [next] | [standalone]


#1459170

FromWanpeng Li <kernellwp@gmail.com>
Date2016-08-10 01:10 +0200
Message-ID<s4ojM-5xv-9@gated-at.bofh.it>
In reply to#1458784
2016-08-09 22:06 GMT+08:00 Rik van Riel <riel@redhat.com>:
> On Tue, 2016-08-09 at 11:59 +0800, Wanpeng Li wrote:
>> Hi Rik,
>> 2016-07-13 22:50 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.com>:
>> > From: Rik van Riel <riel@redhat.com>
>> >
>> > Currently, if there was any irq or softirq time during 'ticks'
>> > jiffies, the entire period will be accounted as irq or softirq
>> > time.
>> >
>> > This is inaccurate if only a subset of the time was actually spent
>> > handling irqs, and could conceivably mis-count all of the ticks
>> > during
>> > a period as irq time, when there was some irq and some softirq
>> > time.
>> >
>> > This can actually happen when irqtime_account_process_tick is
>> > called
>> > from account_idle_ticks, which can pass a larger number of ticks
>> > down
>> > all at once.
>> >
>> > Fix this by changing irqtime_account_hi_update,
>> > irqtime_account_si_update,
>> > and steal_account_process_ticks to work with cputime_t time units,
>> > and
>> > return the amount of time spent in each mode.
>>
>> Do we need to minus st cputime from idle cputime in
>> account_idle_ticks() when noirqtime is true? I try to add this logic
>> w/ noirqtime and idle=poll boot parameter for a full dynticks guest,
>> however, there is no difference, where I miss?
>
> Yes, you are right. The code in account_idle_ticks()
> could use the same treatment.
>
> I am not sure why it would not work, though...

I will try nohz idle kvm guest and other more tests, a patch will be
sent out once successful.

Regards,
Wanpeng Li

[toc] | [prev] | [next] | [standalone]


#1459662

FromWanpeng Li <kernellwp@gmail.com>
Date2016-08-10 21:30 +0200
Message-ID<s4Hmq-Ul-33@gated-at.bofh.it>
In reply to#1459170
2016-08-10 7:07 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> 2016-08-09 22:06 GMT+08:00 Rik van Riel <riel@redhat.com>:
>> On Tue, 2016-08-09 at 11:59 +0800, Wanpeng Li wrote:
>>> Hi Rik,
>>> 2016-07-13 22:50 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.com>:
>>> > From: Rik van Riel <riel@redhat.com>
>>> >
>>> > Currently, if there was any irq or softirq time during 'ticks'
>>> > jiffies, the entire period will be accounted as irq or softirq
>>> > time.
>>> >
>>> > This is inaccurate if only a subset of the time was actually spent
>>> > handling irqs, and could conceivably mis-count all of the ticks
>>> > during
>>> > a period as irq time, when there was some irq and some softirq
>>> > time.
>>> >
>>> > This can actually happen when irqtime_account_process_tick is
>>> > called
>>> > from account_idle_ticks, which can pass a larger number of ticks
>>> > down
>>> > all at once.
>>> >
>>> > Fix this by changing irqtime_account_hi_update,
>>> > irqtime_account_si_update,
>>> > and steal_account_process_ticks to work with cputime_t time units,
>>> > and
>>> > return the amount of time spent in each mode.
>>>
>>> Do we need to minus st cputime from idle cputime in
>>> account_idle_ticks() when noirqtime is true? I try to add this logic
>>> w/ noirqtime and idle=poll boot parameter for a full dynticks guest,
>>> however, there is no difference, where I miss?
>>
>> Yes, you are right. The code in account_idle_ticks()
>> could use the same treatment.
>>
>> I am not sure why it would not work, though...
>
> I will try nohz idle kvm guest and other more tests, a patch will be
> sent out once successful.

After apply the same logic to account_idle_ticks() for nohz idle kvm
guest(noirqtime, idle=poll, one pCPU and four vCPUs), the average idle
drop from 56.8% to 54.75%, I think it makes sense to make a formal
patch.

Regards,
Wanpeng Li

[toc] | [prev] | [next] | [standalone]


#1459186

FromWanpeng Li <kernellwp@gmail.com>
Date2016-08-10 01:30 +0200
Message-ID<s4oD8-5EB-49@gated-at.bofh.it>
In reply to#1458784
2016-08-09 22:06 GMT+08:00 Rik van Riel <riel@redhat.com>:
> On Tue, 2016-08-09 at 11:59 +0800, Wanpeng Li wrote:
>> Hi Rik,
>> 2016-07-13 22:50 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.com>:
>> > From: Rik van Riel <riel@redhat.com>
>> >
>> > Currently, if there was any irq or softirq time during 'ticks'
>> > jiffies, the entire period will be accounted as irq or softirq
>> > time.
>> >
>> > This is inaccurate if only a subset of the time was actually spent
>> > handling irqs, and could conceivably mis-count all of the ticks
>> > during
>> > a period as irq time, when there was some irq and some softirq
>> > time.
>> >
>> > This can actually happen when irqtime_account_process_tick is
>> > called
>> > from account_idle_ticks, which can pass a larger number of ticks
>> > down
>> > all at once.
>> >
>> > Fix this by changing irqtime_account_hi_update,
>> > irqtime_account_si_update,
>> > and steal_account_process_ticks to work with cputime_t time units,
>> > and
>> > return the amount of time spent in each mode.
>>
>> Do we need to minus st cputime from idle cputime in
>> account_idle_ticks() when noirqtime is true? I try to add this logic
>> w/ noirqtime and idle=poll boot parameter for a full dynticks guest,
>> however, there is no difference, where I miss?
>
> Yes, you are right. The code in account_idle_ticks()
> could use the same treatment.
>
> I am not sure why it would not work, though...

Actually I observed a regression caused by this patch. I use a i5
laptop, 4 pCPUs, 4vCPUs for one full dynticks guest, there are four
cpu hog processes(for loop) running in the guest, I hot-unplug the
pCPUs on host one by one until there is only one left, then observe
the top in guest, there are 100% st for cpu0(housekeeping), and 75% st
for other cpus(nohz full). However, w/o this patch, 75% for all the
four cpus.

I try to figure out this recently, any tip is a great appreciated. :)

Regards,
Wapeng Li

[toc] | [prev] | [next] | [standalone]


#1459197

FromWanpeng Li <kernellwp@gmail.com>
Date2016-08-10 01:40 +0200
Message-ID<s4oMO-5I4-15@gated-at.bofh.it>
In reply to#1459186
2016-08-10 7:25 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> 2016-08-09 22:06 GMT+08:00 Rik van Riel <riel@redhat.com>:
>> On Tue, 2016-08-09 at 11:59 +0800, Wanpeng Li wrote:
>>> Hi Rik,
>>> 2016-07-13 22:50 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.com>:
>>> > From: Rik van Riel <riel@redhat.com>
>>> >
>>> > Currently, if there was any irq or softirq time during 'ticks'
>>> > jiffies, the entire period will be accounted as irq or softirq
>>> > time.
>>> >
>>> > This is inaccurate if only a subset of the time was actually spent
>>> > handling irqs, and could conceivably mis-count all of the ticks
>>> > during
>>> > a period as irq time, when there was some irq and some softirq
>>> > time.
>>> >
>>> > This can actually happen when irqtime_account_process_tick is
>>> > called
>>> > from account_idle_ticks, which can pass a larger number of ticks
>>> > down
>>> > all at once.
>>> >
>>> > Fix this by changing irqtime_account_hi_update,
>>> > irqtime_account_si_update,
>>> > and steal_account_process_ticks to work with cputime_t time units,
>>> > and
>>> > return the amount of time spent in each mode.
>>>
>>> Do we need to minus st cputime from idle cputime in
>>> account_idle_ticks() when noirqtime is true? I try to add this logic
>>> w/ noirqtime and idle=poll boot parameter for a full dynticks guest,
>>> however, there is no difference, where I miss?
>>
>> Yes, you are right. The code in account_idle_ticks()
>> could use the same treatment.
>>
>> I am not sure why it would not work, though...
>
> Actually I observed a regression caused by this patch. I use a i5

The regression is caused by your commit "sched,time: Count actually
elapsed irq & softirq time".

> laptop, 4 pCPUs, 4vCPUs for one full dynticks guest, there are four
> cpu hog processes(for loop) running in the guest, I hot-unplug the
> pCPUs on host one by one until there is only one left, then observe
> the top in guest, there are 100% st for cpu0(housekeeping), and 75% st
> for other cpus(nohz full). However, w/o this patch, 75% for all the
> four cpus.
>
> I try to figure out this recently, any tip is a great appreciated. :)
>
> Regards,
> Wapeng Li

[toc] | [prev] | [next] | [standalone]


#1459202

FromWanpeng Li <kernellwp@gmail.com>
Date2016-08-10 01:40 +0200
Message-ID<s4oMO-5I4-21@gated-at.bofh.it>
In reply to#1459186
2016-08-10 7:25 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> 2016-08-09 22:06 GMT+08:00 Rik van Riel <riel@redhat.com>:
>> On Tue, 2016-08-09 at 11:59 +0800, Wanpeng Li wrote:
>>> Hi Rik,
>>> 2016-07-13 22:50 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.com>:
>>> > From: Rik van Riel <riel@redhat.com>
>>> >
>>> > Currently, if there was any irq or softirq time during 'ticks'
>>> > jiffies, the entire period will be accounted as irq or softirq
>>> > time.
>>> >
>>> > This is inaccurate if only a subset of the time was actually spent
>>> > handling irqs, and could conceivably mis-count all of the ticks
>>> > during
>>> > a period as irq time, when there was some irq and some softirq
>>> > time.
>>> >
>>> > This can actually happen when irqtime_account_process_tick is
>>> > called
>>> > from account_idle_ticks, which can pass a larger number of ticks
>>> > down
>>> > all at once.
>>> >
>>> > Fix this by changing irqtime_account_hi_update,
>>> > irqtime_account_si_update,
>>> > and steal_account_process_ticks to work with cputime_t time units,
>>> > and
>>> > return the amount of time spent in each mode.
>>>
>>> Do we need to minus st cputime from idle cputime in
>>> account_idle_ticks() when noirqtime is true? I try to add this logic
>>> w/ noirqtime and idle=poll boot parameter for a full dynticks guest,
>>> however, there is no difference, where I miss?
>>
>> Yes, you are right. The code in account_idle_ticks()
>> could use the same treatment.
>>
>> I am not sure why it would not work, though...
>
> Actually I observed a regression caused by this patch. I use a i5

The regression is caused by your commit "sched,time: Count actually
elapsed irq & softirq time".

> laptop, 4 pCPUs, 4vCPUs for one full dynticks guest, there are four
> cpu hog processes(for loop) running in the guest, I hot-unplug the
> pCPUs on host one by one until there is only one left, then observe
> the top in guest, there are 100% st for cpu0(housekeeping), and 75% st
> for other cpus(nohz full). However, w/o this patch, 75% for all the
> four cpus.
>
> I try to figure out this recently, any tip is a great appreciated. :)
>
> Regards,
> Wapeng Li

[toc] | [prev] | [next] | [standalone]


#1459317 — [PATCH] time,virt: resync steal time when guest & host lose sync

FromRik van Riel <riel@redhat.com>
Date2016-08-10 20:10 +0200
Subject[PATCH] time,virt: resync steal time when guest & host lose sync
Message-ID<s4G70-8uN-67@gated-at.bofh.it>
In reply to#1459202
On Wed, 10 Aug 2016 07:39:08 +0800
Wanpeng Li <kernellwp@gmail.com> wrote:

> The regression is caused by your commit "sched,time: Count actually
> elapsed irq & softirq time".

Wanpeng, does this patch fix your issue?

Paolo, what is your opinion on this issue?

I can think of all kinds of ways in which guest and host might lose
sync with steal time, from uninitialized values at boot, to guest
pause, followed by save to disk, and reload, to live migration, to...

---8<---

Subject: time,virt: resync steal time when guest & host lose sync

When guest and host wildly disagree on steal time, a guest can
do several things:
1) Quickly account all the steal time at once (the kernel did this before
   57430218317e ("sched/cputime: Count actually elapsed irq & softirq time"),
   when steal_account_process_ticks got ULONG_MAX as its maximum value.
2) Stay out of sync for an indeterminate amount of time. This is what the
   system does today.
3) Sync up the guest value to the host-provided value, without accounting
   an absurdly large value in the cpu time statistics.

This patch makes the kernel do (3), which seems like the right thing
to do.

The exact value of the threshold use probably does not matter too much,
as long as it is long enough to cover all the timer ticks that passed
during an idle period, because (irqtime_)account_idle_ticks can process
a large amount of time all at once.

Signed-off-by: Rik van Riel <riel@redhat.com>
Reported-by: Wanpeng Li <kernellwp@gmail.com>
---
 kernel/sched/cputime.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 1934f658c036..c18f9e717af6 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -273,7 +273,17 @@ static __always_inline cputime_t steal_account_process_time(cputime_t maxtime)
 		steal = paravirt_steal_clock(smp_processor_id());
 		steal -= this_rq()->prev_steal_time;
 
-		steal_cputime = min(nsecs_to_cputime(steal), maxtime);
+		steal_cputime = nsecs_to_cputime(steal);
+		if (steal_cputime > 32 * maxtime) {
+			/*
+			 * Guest and host steal time values are way out of
+			 * sync. Sync up the guest steal time with the host.
+			 */
+			this_rq()->prev_steal_time +=
+					cputime_to_nsecs(steal_cputime);
+			return 0;
+		}
+		steal_cputime = min(steal_cputime, maxtime);
 		account_steal_time(steal_cputime);
 		this_rq()->prev_steal_time += cputime_to_nsecs(steal_cputime);
 

[toc] | [prev] | [next] | [standalone]


#1459823

FromWanpeng Li <kernellwp@gmail.com>
Date2016-08-10 22:20 +0200
Message-ID<s4I8P-1sK-41@gated-at.bofh.it>
In reply to#1459202
2016-08-10 13:07 GMT+08:00 Rik van Riel <riel@redhat.com>:
> On Wed, 2016-08-10 at 07:39 +0800, Wanpeng Li wrote:
>> 2016-08-10 7:25 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
>> > 2016-08-09 22:06 GMT+08:00 Rik van Riel <riel@redhat.com>:
>> > > On Tue, 2016-08-09 at 11:59 +0800, Wanpeng Li wrote:
>> > > > Hi Rik,
>> > > > 2016-07-13 22:50 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.
>> > > > com>:
>> > > > > From: Rik van Riel <riel@redhat.com>
>> > > > >
>> > > > > Currently, if there was any irq or softirq time during
>> > > > > 'ticks'
>> > > > > jiffies, the entire period will be accounted as irq or
>> > > > > softirq
>> > > > > time.
>> > > > >
>> > > > > This is inaccurate if only a subset of the time was actually
>> > > > > spent
>> > > > > handling irqs, and could conceivably mis-count all of the
>> > > > > ticks
>> > > > > during
>> > > > > a period as irq time, when there was some irq and some
>> > > > > softirq
>> > > > > time.
>> > > > >
>> > > > > This can actually happen when irqtime_account_process_tick is
>> > > > > called
>> > > > > from account_idle_ticks, which can pass a larger number of
>> > > > > ticks
>> > > > > down
>> > > > > all at once.
>> > > > >
>> > > > > Fix this by changing irqtime_account_hi_update,
>> > > > > irqtime_account_si_update,
>> > > > > and steal_account_process_ticks to work with cputime_t time
>> > > > > units,
>> > > > > and
>> > > > > return the amount of time spent in each mode.
>> > > >
>> > > > Do we need to minus st cputime from idle cputime in
>> > > > account_idle_ticks() when noirqtime is true? I try to add this
>> > > > logic
>> > > > w/ noirqtime and idle=poll boot parameter for a full dynticks
>> > > > guest,
>> > > > however, there is no difference, where I miss?
>> > >
>> > > Yes, you are right. The code in account_idle_ticks()
>> > > could use the same treatment.
>> > >
>> > > I am not sure why it would not work, though...
>> >
>> > Actually I observed a regression caused by this patch. I use a i5
>>
>> The regression is caused by your commit "sched,time: Count actually
>> elapsed irq & softirq time".
>
> Wanpeng and I discussed this issue, and discovered
> that this bug is triggered by my patch, specifically
> this bit:
>
> -       if (steal_account_process_tick(ULONG_MAX))
> +       other = account_other_time(cputime);
> +       if (other >= cputime)
>                 return;
>
> Replacing "cputime" with "ULONG_MAX" as the argument
> to account_other_time makes the bug disappear.
>
> However, this is not the cause of the bug.
>
> The cause of the bug appears to be that the values
> used to figure out how much steal time has passed
> are never initialized.
>
>                 steal = paravirt_steal_clock(smp_processor_id());
>                 steal -= this_rq()->prev_steal_time;
>
> The first of the two may be initialized by the host
> (I did not verify that), but the second one does not
> have any explicit initializers anywhere in the kernel
> tree.
>
> This can lead to an arbitrarily large difference between
> paravirt_steal_clock(smp_processor_id()) and
> this_rq()->prev_steal_time, which results in nothing but
> steal time getting accounted for a potentially a very
> long amount of time.
>
> Previously we carried this patch to initialize the
> various rq->prev_* values at CPU hotplug time:
>
> https://patchwork.codeaurora.org/patch/27699/
>
> Which got reverted by Paolo here:
>
> https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=sche
> d/core&id=3d89e5478bf550a50c99e93adf659369798263b0
>
> Which leads me to this question:
>
> Paulo, how would you like us to fix this bug?
>
> It seems like the host and guest steal time CAN get out
> of sync, sometimes by a ridiculous amount, and we need
> some way to get the excessive difference out of the way,
> without it getting accounted as steal time (not immediately,
> and not over the next 17 hours, or months).

I can be the volunteer to fix it if you guys have an idea. :)

Regards,
Wanpeng Li

[toc] | [prev] | [next] | [standalone]


#1459992 — Re: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time

FromRik van Riel <riel@redhat.com>
Date2016-08-10 23:30 +0200
SubjectRe: [PATCH 1/5] sched,time: Count actually elapsed irq & softirq time
Message-ID<s4I8P-1sK-43@gated-at.bofh.it>
In reply to#1459202

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2016-08-10 at 07:39 +0800, Wanpeng Li wrote:
> 2016-08-10 7:25 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> > 2016-08-09 22:06 GMT+08:00 Rik van Riel <riel@redhat.com>:
> > > On Tue, 2016-08-09 at 11:59 +0800, Wanpeng Li wrote:
> > > > Hi Rik,
> > > > 2016-07-13 22:50 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.
> > > > com>:
> > > > > From: Rik van Riel <riel@redhat.com>
> > > > > 
> > > > > Currently, if there was any irq or softirq time during
> > > > > 'ticks'
> > > > > jiffies, the entire period will be accounted as irq or
> > > > > softirq
> > > > > time.
> > > > > 
> > > > > This is inaccurate if only a subset of the time was actually
> > > > > spent
> > > > > handling irqs, and could conceivably mis-count all of the
> > > > > ticks
> > > > > during
> > > > > a period as irq time, when there was some irq and some
> > > > > softirq
> > > > > time.
> > > > > 
> > > > > This can actually happen when irqtime_account_process_tick is
> > > > > called
> > > > > from account_idle_ticks, which can pass a larger number of
> > > > > ticks
> > > > > down
> > > > > all at once.
> > > > > 
> > > > > Fix this by changing irqtime_account_hi_update,
> > > > > irqtime_account_si_update,
> > > > > and steal_account_process_ticks to work with cputime_t time
> > > > > units,
> > > > > and
> > > > > return the amount of time spent in each mode.
> > > > 
> > > > Do we need to minus st cputime from idle cputime in
> > > > account_idle_ticks() when noirqtime is true? I try to add this
> > > > logic
> > > > w/ noirqtime and idle=poll boot parameter for a full dynticks
> > > > guest,
> > > > however, there is no difference, where I miss?
> > > 
> > > Yes, you are right. The code in account_idle_ticks()
> > > could use the same treatment.
> > > 
> > > I am not sure why it would not work, though...
> > 
> > Actually I observed a regression caused by this patch. I use a i5
> 
> The regression is caused by your commit "sched,time: Count actually
> elapsed irq & softirq time".

Wanpeng and I discussed this issue, and discovered
that this bug is triggered by my patch, specifically
this bit:

-       if (steal_account_process_tick(ULONG_MAX))
+       other = account_other_time(cputime);
+       if (other >= cputime)
                return;

Replacing "cputime" with "ULONG_MAX" as the argument
to account_other_time makes the bug disappear.

However, this is not the cause of the bug.

The cause of the bug appears to be that the values
used to figure out how much steal time has passed
are never initialized.

                steal = paravirt_steal_clock(smp_processor_id());
                steal -= this_rq()->prev_steal_time;

The first of the two may be initialized by the host
(I did not verify that), but the second one does not
have any explicit initializers anywhere in the kernel
tree.

This can lead to an arbitrarily large difference between
paravirt_steal_clock(smp_processor_id()) and
this_rq()->prev_steal_time, which results in nothing but
steal time getting accounted for a potentially a very
long amount of time.

Previously we carried this patch to initialize the
various rq->prev_* values at CPU hotplug time:

https://patchwork.codeaurora.org/patch/27699/

Which got reverted by Paolo here:

https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=sche
d/core&id=3d89e5478bf550a50c99e93adf659369798263b0

Which leads me to this question:

Paulo, how would you like us to fix this bug?

It seems like the host and guest steal time CAN get out
of sync, sometimes by a ridiculous amount, and we need
some way to get the excessive difference out of the way,
without it getting accounted as steal time (not immediately,
and not over the next 17 hours, or months).

-- 

All Rights Reversed.

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web