linux-kernel - Re: [PATCH 1/2] sched,time: remove pointless divides from __acct_update

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]

Message-ID: <20160129231018.GK6357@twins.programming.kicks-ass.net>
Date:	Sat, 30 Jan 2016 00:10:18 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	riel@...hat.com
Cc:	linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...nel.org,
	luto@...capital.net, fweisbec@...il.com
Subject: Re: [PATCH 1/2] sched,time: remove pointless divides from
 __acct_update_integrals

On Fri, Jan 29, 2016 at 05:22:59PM -0500, riel@...hat.com wrote:
> From: Rik van Riel <riel@...hat.com>
> 
> When running a microbenchmark calling an invalid syscall number
> in a loop, on a nohz_full CPU, we spend a full 9% of our CPU
> time in __acct_update_integrals.
> 
> This function converts cputime_t to jiffies, to a timeval, only to
> convert the timeval back to microseconds before discarding it.
> 
> This patch leaves __acct_update_integrals functionally equivalent,
> but speeds things up by about 11%, with 10 million calls to an
> invalid syscall number dropping from 3.7 to 3.3 seconds.

WTH is this taskstat crap anyway? Who uses it and can't we kill it?

There seems to be an endless reserve of accounting crap.

> +++ b/kernel/tsacct.c
> @@ -125,22 +125,22 @@ static void __acct_update_integrals(struct task_struct *tsk,
>  {
>  	if (likely(tsk->mm)) {
>  		cputime_t time, dtime;
>  		unsigned long flags;
> +		u64 delta, usecs;
>  
>  		local_irq_save(flags);
>  		time = stime + utime;
>  		dtime = time - tsk->acct_timexpd;
> +		delta = cputime_to_jiffies(dtime);
>  
>  		if (delta == 0)
>  			goto out;

> +
> +		usecs = jiffies_to_usecs(delta);
> +
>  		tsk->acct_timexpd = time;
> +		tsk->acct_rss_mem1 += usecs * get_mm_rss(tsk->mm);
> +		tsk->acct_vm_mem1 += usecs * tsk->mm->total_vm;
>  	out:
>  		local_irq_restore(flags);
>  	}

Hurch, what horrible code.

Can't you at least drop the pointless indent level while you're there
anyway?

Also, it looks like the callpath through acct_account_cputime() should
already guarantee IRQs are disabled, so if you move the irq_save /
irq_restore business into acct_update_integrals() you can remove them
too from the fast path.

And I think you can kill that last divide too, if you do something like:


	nsecs = cputime_to_nsecs(dtime);
	if (nsecs < TICK_NSEC)
		goto out;

	usecs = nsecs >> 10;

	...

And if people really care, you can correct the 1024 != 1000 thing in
xacct_add_tsk() which seems to be the consumer side and thus should be
exceedingly rare.