Re: [BUG nohz]: wrong user and system time accounting

Wanpeng Li
2017-03-30T11:52:58Z

2017-03-30 14:47 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> Cc Peterz, Thomas,
> 2017-03-30 12:27 GMT+08:00 Mike Galbraith <efault@gmx.de>:
>> On Wed, 2017-03-29 at 16:08 -0400, Rik van Riel wrote:
>>
>>> In other words, the tick on cpu0 is aligned
>>> with the tick on the nohz_full cpus, and
>>> jiffies is advanced while the nohz_full cpus
>>> with an active tick happen to be in kernel
>>> mode?
>>
>> You really want skew_tick=1, especially on big boxen.
>>
>>> Frederic, can you think of any reason why
>>> the tick on nohz_full CPUs would end up aligned
>>> with the tick on cpu0, instead of running at some
>>> random offset?
>>
>> (I or low rq->clock bits as crude NOHZ collision avoidance)
>>
>>> A random offset, or better yet a somewhat randomized
>>> tick length to make sure that simultaneous ticks are
>>> fairly rare and the vtime sampling does not end up
>>> "in phase" with the jiffies incrementing, could make
>>> the accounting work right again.
>>
>> That improves jitter, especially on big boxen. I have an 8 socket box
>> that thinks it's an extra large PC, there, collision avoidance matters
>> hugely. I couldn't reproduce bean counting woes, no idea if collision
>> avoidance will help that.
>
> So I implement two methods, one is from Rik's random offset proposal

If we should just add random offset to the cpu in the nohz_full mode?

> through skew tick, the other one is from Frederic's proposal and it is
> the same as my original idea through use nanosecond granularity to
> check deltas but only perform an actual cputime update when that delta
>>= TICK_NSEC. Both methods can solve the bug which Luiz reported.

This can just solves two cpu hogs running on the cpu in nohz_full
mode. However, Luiz's testcase w/ ./acct-bug 1 995 shows idle 100%.

Regards,
Wanpeng Li

> Peterz, Thomas, any ideas?
>
> --------------------------->8-------------------------------------------------------------
>
> skew tick:
>
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 7fe53be..9981437 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -1198,7 +1198,11 @@ void tick_setup_sched_timer(void)
> hrtimer_set_expires(&ts->sched_timer, tick_init_jiffy_update());
>
> /* Offset the tick to avert jiffies_lock contention. */
> +#ifdef CONFIG_NO_HZ_FULL
> + if (sched_skew_tick || tick_nohz_full_running) {
> +#else
> if (sched_skew_tick) {
> +#endif
> u64 offset = ktime_to_ns(tick_period) >> 1;
> do_div(offset, num_possible_cpus());
> offset *= smp_processor_id();
>
> -------------------------------------->8-----------------------------------------------------
>
> use nanosecond granularity to check deltas but only perform an actual
> cputime update when that delta >= TICK_NSEC.
>
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index f3778e2b..f1ee393 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -676,18 +676,21 @@ void thread_group_cputime_adjusted(struct
> task_struct *p, u64 *ut, u64 *st)
> #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
> static u64 vtime_delta(struct task_struct *tsk)
> {
> - unsigned long now = READ_ONCE(jiffies);
> + u64 now = local_clock();
> + u64 delta;
> +
> + delta = now - tsk->vtime_snap;
>
> - if (time_before(now, (unsigned long)tsk->vtime_snap))
> + if (delta < TICK_NSEC)
> return 0;
>
> - return jiffies_to_nsecs(now - tsk->vtime_snap);
> + return jiffies_to_nsecs(delta / TICK_NSEC);
> }
>
> static u64 get_vtime_delta(struct task_struct *tsk)
> {
> - unsigned long now = READ_ONCE(jiffies);
> - u64 delta, other;
> + u64 delta = vtime_delta(tsk);
> + u64 other;
>
> /*
> * Unlike tick based timing, vtime based timing never has lost
> @@ -696,10 +699,9 @@ static u64 get_vtime_delta(struct task_struct *tsk)
> * elapsed time. Limit account_other_time to prevent rounding
> * errors from causing elapsed vtime to go negative.
> */
> - delta = jiffies_to_nsecs(now - tsk->vtime_snap);
> other = account_other_time(delta);
> WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE);
> - tsk->vtime_snap = now;
> + tsk->vtime_snap += delta;
>
> return delta - other;
> }
> @@ -776,7 +778,7 @@ void arch_vtime_task_switch(struct task_struct *prev)
>
> write_seqcount_begin(&current->vtime_seqcount);
> current->vtime_snap_whence = VTIME_SYS;
> - current->vtime_snap = jiffies;
> + current->vtime_snap = sched_clock_cpu(smp_processor_id());
> write_seqcount_end(&current->vtime_seqcount);
> }
>
> @@ -787,7 +789,7 @@ void vtime_init_idle(struct task_struct *t, int cpu)
> local_irq_save(flags);
> write_seqcount_begin(&t->vtime_seqcount);
> t->vtime_snap_whence = VTIME_SYS;
> - t->vtime_snap = jiffies;
> + t->vtime_snap = sched_clock_cpu(cpu);
> write_seqcount_end(&t->vtime_seqcount);
> local_irq_restore(flags);
> }
>
> Regards,
> Wanpeng Li