Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755955AbbFOPew (ORCPT <rfc822;w@1wt.eu>);
	Mon, 15 Jun 2015 11:34:52 -0400
Received: from mail-la0-f50.google.com ([209.85.215.50]:35000 "EHLO
	mail-la0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754336AbbFOPeo (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 15 Jun 2015 11:34:44 -0400
MIME-Version: 1.0
In-Reply-To: <20150612110158.GA18673@twins.programming.kicks-ass.net>
References: <1434099316-29749-1-git-send-email-fredrik.markstrom@gmail.com>
 <1434099316-29749-2-git-send-email-fredrik.markstrom@gmail.com>
 <1434104217.1495.74.camel@twins> <20150612110158.GA18673@twins.programming.kicks-ass.net>
From: =?UTF-8?Q?Fredrik_Markstr=C3=B6m?= <fredrik.markstrom@gmail.com>
Date: Mon, 15 Jun 2015 17:34:11 +0200
Message-ID: <CAKdL+dQWGtzURUe+4cB4XtsoaEPh0p0O1ejzbgxN0AUsbvUi0g@mail.gmail.com>
Subject: Re: [PATCH 1/1] cputime: Make the reported utime+stime correspond to
 the actual runtime.
To: Peter Zijlstra <peterz@infradead.org>
Cc: mingo@redhat.com, linux-kernel@vger.kernel.org,
        Rik van Riel <riel@redhat.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4070
Lines: 117

Hello Peter, your patch helps with some of the cases but not all:

(the "called with.." below means cputime_adjust() is called with the
values specified in it's struct task_cputime argument.)

It helps when called with:

sum_exec_runtime=1000000000 utime=0 stime=1
... followed by...
sum_exec_runtime=1010000000 utime=100 stime=1

It doesn't help when called with:

sum_exec_runtime=1000000000 utime=1 stime=0
... followed by...
sum_exec_runtime=1010000000 utime=1 stime=100

Also if we get a call with:

sum_exec_runtime=1000000000 utime=1 stime=1

...  then get preempted after your proposed fix and before we are done
with the calls to cpu_advance(), then gets called again (from a
different thread) with:

sum_exec_runtime=1010000000 utime=100 stime=1

... it still breaks.

I think there might be additional concurrency problems before, between
and/or possibly after the calls to cputime_advance(), at least if we
want to guarantee that sys+user should stay sane. I believe my
proposed patch eliminates those potential problems in a pretty
straight forward way.

I tried to come up with a lock free solution but didn't find a simple
solution. Since, from what I understand, the likelihood of scalability
issues here are unlikely I felt that simplicity was preferred. Also
the current implementation has two cmpxchg:s, and my proposal a single
spinlock, so on some setups I bet it's more efficient (like mine with
a lousy interconnect and preempt-rt (but I'm on thin ice here)).

Below is the output from my test application (it's to much of a hack
to post publicly), but I'd be happy to clean it up and post it if
necessary.

/Fredrik


#<test>.<step> <input> =>  <test>.<step> <output> [=====> FAILED]

0.0 sum_exec=100000000000 utime=0     stime=1     =>    0.0 tot=10000
    user=0     sys=10000
0.1 sum_exec=101000000000 utime=100 stime=1     =>    0.1 tot=10100
  user=100   sys=10000

1.0 sum_exec=100000000000 utime=1     stime=0     =>    1.0 tot=10000
    user=10000 sys=0
1.1 sum_exec=101000000000 utime=1     stime=100 =>    1.1 tot=20000
  user=10000 sys=10000 =====> FAILED

2.0 sum_exec=100000000000 utime=1     stime=1     =>    2.0 tot=10000
    user=5000  sys=5000
2.1 sum_exec=101000000000 utime=100 stime=1     =>    2.1 tot=10100
  user=5100  sys=5000

3.0 sum_exec=100000000000 utime=1     stime=1     => <<PREEMPT>>
    3.1 sum_exec=101000000000 utime=100 stime=1     =>    3.1
tot=10100      user=10000 sys=100
<<SWITCH BACK>>   3.0 tot=15000      user=10000 sys=5000  =====> FAILED


On Fri, Jun 12, 2015 at 1:01 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Jun 12, 2015 at 12:16:57PM +0200, Peter Zijlstra wrote:
>> On Fri, 2015-06-12 at 10:55 +0200, Fredrik Markstrom wrote:
>> > The scaling mechanism might sometimes cause top to report >100%
>> > (sometimes > 1000%) cpu usage for a single thread. This patch makes
>> > sure that stime+utime corresponds to the actual runtime of the thread.
>>
>> This Changelog is inadequate, it does not explain the actual problem.
>>
>> > +static DEFINE_SPINLOCK(prev_time_lock);
>>
>> global (spin)locks are bad.
>
> Since you have a proglet handy to test this; does something like the
> below help anything?
>
> ---
>  kernel/sched/cputime.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index f5a64ffad176..3d3f60a555a0 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -613,6 +613,10 @@ static void cputime_adjust(struct task_cputime *curr,
>
>                 stime = scale_stime((__force u64)stime,
>                                     (__force u64)rtime, (__force u64)total);
> +
> +               if (stime < prev->stime)
> +                       stime = prev->stime;
> +
>                 utime = rtime - stime;
>         }
>



-- 
/Fredrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/