linux-kernel - Re: [patch] sched: accurate user accounting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 25 Mar 2007 19:19:21 +0400 (MSD)
From:	malc <av1474@...tv.ru>
To:	Con Kolivas <kernel@...ivas.org>
cc:	Ingo Molnar <mingo@...e.hu>,
	linux list <linux-kernel@...r.kernel.org>, zwane@...radead.org,
	ck list <ck@....kolivas.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [patch] sched: accurate user accounting

On Mon, 26 Mar 2007, Con Kolivas wrote:

> On Monday 26 March 2007 00:57, malc wrote:
>> On Mon, 26 Mar 2007, Con Kolivas wrote:
>>> On Sunday 25 March 2007 23:06, malc wrote:
>>>> On Sun, 25 Mar 2007, Con Kolivas wrote:
>>>>> On Sunday 25 March 2007 21:46, Con Kolivas wrote:
>>>>>> On Sunday 25 March 2007 21:34, malc wrote:
>>>>>>> On Sun, 25 Mar 2007, Ingo Molnar wrote:
>>>>>>>> * Con Kolivas <kernel@...ivas.org> wrote:
>>>>>>>>> For an rsdl 0.33 patched kernel. Comments? Overhead worth it?
>>>>
>>>> [..snip..]
>>>>
>>>>> ---
>>>>> Currently we only do cpu accounting to userspace based on what is
>>>>> actually happening precisely on each tick. The accuracy of that
>>>>> accounting gets progressively worse the lower HZ is. As we already keep
>>>>> accounting of nanosecond resolution we can accurately track user cpu,
>>>>> nice cpu and idle cpu if we move the accounting to update_cpu_clock
>>>>> with a nanosecond cpu_usage_stat entry. This increases overhead
>>>>> slightly but avoids the problem of tick aliasing errors making
>>>>> accounting unreliable.
>>>>>
>>>>> Signed-off-by: Con Kolivas <kernel@...ivas.org>
>>>>> Signed-off-by: Ingo Molnar <mingo@...e.hu>
>>>>
>>>> [..snip..]
>>>>
>>>> Forgot to mention. Given that this goes into the kernel, shouldn't
>>>> Documentation/cpu-load.txt be amended/removed?
>>>
>>> Yes that's a good idea. Also there should be a sanity check because
>>> sometimes for some reason noone's been able to explain to me sched_clock
>>> gives a value which doesn't make sense (time appears to have gone
>>> backwards) and that will completely ruin the accounting from then on.
>>
>> After running this new kernel for a while i guess i have hit this issue:
>> http://www.boblycat.org/~malc/apc/bad-load.png
>>
>> Top and icewm's monitor do show incredibly huge load while in reality
>> nothing like that is really happening. Both ad-hoc and `/proc/stat' (idle)
>> show normal CPU utilization (7% since i'm doing some A/V stuff in the
>> background)
>
> Yes I'd say you hit the problem I described earlier. When playing with
> sched_clock() I found it gave some "interesting" results fairly infrequently.
> They could lead to ridiculous accounting mistakes.
>
> So before we go any further with this patch, can you try the following one and
> see if this simple sanity check is enough?

Sure (compiling the kernel now), too bad old axiom that testing can not
confirm absence of bugs holds.

I have one nit and one request from clarification. Question first (i
admit i haven't looked at the surroundings of the patch maybe things
would have been are self evident if i did):

What this patch amounts to is that the accounting logic is moved from
timer interrupt to the place where scheduler switches task (or something
to that effect)?

[..snip..]

>  * These are the 'tuning knobs' of the scheduler:
> @@ -3017,8 +3018,53 @@ EXPORT_PER_CPU_SYMBOL(kstat);
> static inline void
> update_cpu_clock(struct task_struct *p, struct rq *rq, unsigned long long now)
> {
> -	p->sched_time += now - p->last_ran;
> +	struct cpu_usage_stat *cpustat = &kstat_this_cpu.cpustat;
> +	cputime64_t time_diff;
> +
> 	p->last_ran = rq->most_recent_timestamp = now;
> +	/* Sanity check. It should never go backwards or ruin accounting */
> +	if (unlikely(now < p->last_ran))
> +		return;
> +	time_diff = now - p->last_ran;

A nit. Anything wrong with:

time_diff = now - p->last_ran;
if (unlikeley (LESS_THAN_ZERO (time_diff))
         return;

[..snip..]

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