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

Groups > linux.kernel > #1322783 > unrolled thread

[PATCH 1/4] sched,time: remove non-power-of-two divides from __acct_update_integrals

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

  [PATCH 1/4] sched,time: remove non-power-of-two divides from __acct_update_integrals riel@redhat.com - 2016-02-01 03:20 +0100
    Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from  __acct_update_integrals kbuild test robot <lkp@intel.com> - 2016-02-01 05:50 +0100
    Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from  __acct_update_integrals Thomas Gleixner <tglx@linutronix.de> - 2016-02-01 09:40 +0100
      Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from  __acct_update_integrals Peter Zijlstra <peterz@infradead.org> - 2016-02-01 10:30 +0100
        Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from  __acct_update_integrals Thomas Gleixner <tglx@linutronix.de> - 2016-02-01 10:40 +0100
        Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from  __acct_update_integrals Rik van Riel <riel@redhat.com> - 2016-02-01 14:50 +0100
          Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from  __acct_update_integrals Peter Zijlstra <peterz@infradead.org> - 2016-02-01 15:00 +0100

#1322783 — [PATCH 1/4] sched,time: remove non-power-of-two divides from __acct_update_integrals

From: Rik van Riel <riel@redhat.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 12%, with 10 million calls to an
invalid syscall number dropping from 3.7 to 3.25 seconds.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 kernel/tsacct.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/kernel/tsacct.c b/kernel/tsacct.c
index 975cb49e32bf..1b121a2f1c55 100644
--- a/kernel/tsacct.c
+++ b/kernel/tsacct.c
@@ -93,9 +93,9 @@ void xacct_add_tsk(struct taskstats *stats, struct task_struct *p)
 {
 	struct mm_struct *mm;
 
-	/* convert pages-usec to Mbyte-usec */
-	stats->coremem = p->acct_rss_mem1 * PAGE_SIZE / MB;
-	stats->virtmem = p->acct_vm_mem1 * PAGE_SIZE / MB;
+	/* convert pages-nsec/1024 to Mbyte-usec, see __acct_update_integrals */
+	stats->coremem = p->acct_rss_mem1 * PAGE_SIZE / (1000 * KB);
+	stats->virtmem = p->acct_vm_mem1 * PAGE_SIZE / (1000 * KB);
 	mm = get_task_mm(p);
 	if (mm) {
 		/* adjust to KB unit */
@@ -125,22 +125,26 @@ static void __acct_update_integrals(struct task_struct *tsk,
 {
 	if (likely(tsk->mm)) {
 		cputime_t time, dtime;
-		struct timeval value;
 		unsigned long flags;
 		u64 delta;
 
 		local_irq_save(flags);
 		time = stime + utime;
 		dtime = time - tsk->acct_timexpd;
-		jiffies_to_timeval(cputime_to_jiffies(dtime), &value);
-		delta = value.tv_sec;
-		delta = delta * USEC_PER_SEC + value.tv_usec;
+		/* Avoid division: cputime_t is often in nanoseconds already. */
+		delta = cputime_to_nsecs(dtime);
 
-		if (delta == 0)
+		if (delta < TICK_NSEC)
 			goto out;
+
 		tsk->acct_timexpd = time;
-		tsk->acct_rss_mem1 += delta * get_mm_rss(tsk->mm);
-		tsk->acct_vm_mem1 += delta * tsk->mm->total_vm;
+		/*
+		 * Divide by 1024 to avoid overflow, and to avoid division.
+		 * The final unit reported to userspace is Mbyte-usecs,
+		 * the rest of the math is done in xacct_add_tsk.
+		 */
+		tsk->acct_rss_mem1 += delta * get_mm_rss(tsk->mm) >> 10;
+		tsk->acct_vm_mem1 += delta * tsk->mm->total_vm >> 10;
 	out:
 		local_irq_restore(flags);
 	}
-- 
2.5.0

[toc] | [next] | [standalone]

#1322824 — Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from __acct_update_integrals

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

Hi Rik,

[auto build test ERROR on tip/sched/core]
[also build test ERROR on v4.5-rc2 next-20160129]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/riel-redhat-com/sched-time-reduce-nohz_full-syscall-overhead-40/20160201-101609
config: sh-sh7757lcr_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   kernel/built-in.o: In function `get_mm_hiwater_vm':
>> include/linux/mm.h:1377: undefined reference to `__udivdi3'

vim +1377 include/linux/mm.h

172703b0 Matt Fleming      2011-05-24  1361  	atomic_long_dec(&mm->rss_stat.count[member]);
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1362  }
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1363  
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1364  static inline unsigned long get_mm_rss(struct mm_struct *mm)
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1365  {
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1366  	return get_mm_counter(mm, MM_FILEPAGES) +
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1367  		get_mm_counter(mm, MM_ANONPAGES);
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1368  }
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1369  
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1370  static inline unsigned long get_mm_hiwater_rss(struct mm_struct *mm)
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1371  {
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1372  	return max(mm->hiwater_rss, get_mm_rss(mm));
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1373  }
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1374  
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1375  static inline unsigned long get_mm_hiwater_vm(struct mm_struct *mm)
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1376  {
d559db08 KAMEZAWA Hiroyuki 2010-03-05 @1377  	return max(mm->hiwater_vm, mm->total_vm);
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1378  }
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1379  
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1380  static inline void update_hiwater_rss(struct mm_struct *mm)
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1381  {
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1382  	unsigned long _rss = get_mm_rss(mm);
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1383  
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1384  	if ((mm)->hiwater_rss < _rss)
d559db08 KAMEZAWA Hiroyuki 2010-03-05  1385  		(mm)->hiwater_rss = _rss;

:::::: The code at line 1377 was first introduced by commit
:::::: d559db086ff5be9bcc259e5aa50bf3d881eaf1d1 mm: clean up mm_counter

:::::: TO: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

#1322905 — Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from __acct_update_integrals

On Sun, 31 Jan 2016, riel@redhat.com wrote:
> @@ -93,9 +93,9 @@ void xacct_add_tsk(struct taskstats *stats, struct task_struct *p)
>  {
>  	struct mm_struct *mm;
>  
> -	/* convert pages-usec to Mbyte-usec */
> -	stats->coremem = p->acct_rss_mem1 * PAGE_SIZE / MB;
> -	stats->virtmem = p->acct_vm_mem1 * PAGE_SIZE / MB;
> +	/* convert pages-nsec/1024 to Mbyte-usec, see __acct_update_integrals */
> +	stats->coremem = p->acct_rss_mem1 * PAGE_SIZE / (1000 * KB);
> +	stats->virtmem = p->acct_vm_mem1 * PAGE_SIZE / (1000 * KB);

You replace "/ (1024 * 1024)" by "/ (1000 * 1024). So that's introducing a non
power of 2 division instead of removing one and wont compile on systems which
do not have a 64/32 division in hardware.

Thanks,

	tglx

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

#1322937 — Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from __acct_update_integrals

On Mon, Feb 01, 2016 at 09:37:00AM +0100, Thomas Gleixner wrote:
> On Sun, 31 Jan 2016, riel@redhat.com wrote:
> > @@ -93,9 +93,9 @@ void xacct_add_tsk(struct taskstats *stats, struct task_struct *p)
> >  {
> >  	struct mm_struct *mm;
> >  
> > -	/* convert pages-usec to Mbyte-usec */
> > -	stats->coremem = p->acct_rss_mem1 * PAGE_SIZE / MB;
> > -	stats->virtmem = p->acct_vm_mem1 * PAGE_SIZE / MB;
> > +	/* convert pages-nsec/1024 to Mbyte-usec, see __acct_update_integrals */
> > +	stats->coremem = p->acct_rss_mem1 * PAGE_SIZE / (1000 * KB);
> > +	stats->virtmem = p->acct_vm_mem1 * PAGE_SIZE / (1000 * KB);
> 
> You replace "/ (1024 * 1024)" by "/ (1000 * 1024). So that's introducing a non
> power of 2 division instead of removing one and wont compile on systems which
> do not have a 64/32 division in hardware.

Yep, so that needs to be fixed to use do_div(). But the reason for this
is that this is the consumer side of these stats and therefore rarely
executed.

This patch effectively moves a div out of the fast path into the slow
path.

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

#1322945 — Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from __acct_update_integrals

On Mon, 1 Feb 2016, Peter Zijlstra wrote:

> On Mon, Feb 01, 2016 at 09:37:00AM +0100, Thomas Gleixner wrote:
> > On Sun, 31 Jan 2016, riel@redhat.com wrote:
> > > @@ -93,9 +93,9 @@ void xacct_add_tsk(struct taskstats *stats, struct task_struct *p)
> > >  {
> > >  	struct mm_struct *mm;
> > >  
> > > -	/* convert pages-usec to Mbyte-usec */
> > > -	stats->coremem = p->acct_rss_mem1 * PAGE_SIZE / MB;
> > > -	stats->virtmem = p->acct_vm_mem1 * PAGE_SIZE / MB;
> > > +	/* convert pages-nsec/1024 to Mbyte-usec, see __acct_update_integrals */
> > > +	stats->coremem = p->acct_rss_mem1 * PAGE_SIZE / (1000 * KB);
> > > +	stats->virtmem = p->acct_vm_mem1 * PAGE_SIZE / (1000 * KB);
> > 
> > You replace "/ (1024 * 1024)" by "/ (1000 * 1024). So that's introducing a non
> > power of 2 division instead of removing one and wont compile on systems which
> > do not have a 64/32 division in hardware.
> 
> Yep, so that needs to be fixed to use do_div(). But the reason for this
> is that this is the consumer side of these stats and therefore rarely
> executed.
> 
> This patch effectively moves a div out of the fast path into the slow
> path.

Yeah, noticed after hitting Send :)
 

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

#1323127 — Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from __acct_update_integrals

On 02/01/2016 04:22 AM, Peter Zijlstra wrote:
> On Mon, Feb 01, 2016 at 09:37:00AM +0100, Thomas Gleixner wrote:
>> On Sun, 31 Jan 2016, riel@redhat.com wrote:
>>> @@ -93,9 +93,9 @@ void xacct_add_tsk(struct taskstats *stats, struct task_struct *p)
>>>  {
>>>  	struct mm_struct *mm;
>>>  
>>> -	/* convert pages-usec to Mbyte-usec */
>>> -	stats->coremem = p->acct_rss_mem1 * PAGE_SIZE / MB;
>>> -	stats->virtmem = p->acct_vm_mem1 * PAGE_SIZE / MB;
>>> +	/* convert pages-nsec/1024 to Mbyte-usec, see __acct_update_integrals */
>>> +	stats->coremem = p->acct_rss_mem1 * PAGE_SIZE / (1000 * KB);
>>> +	stats->virtmem = p->acct_vm_mem1 * PAGE_SIZE / (1000 * KB);
>>
>> You replace "/ (1024 * 1024)" by "/ (1000 * 1024). So that's introducing a non
>> power of 2 division instead of removing one and wont compile on systems which
>> do not have a 64/32 division in hardware.
> 
> Yep, so that needs to be fixed to use do_div(). But the reason for this
> is that this is the consumer side of these stats and therefore rarely
> executed.

Before I send in a v4 with do_div in that location, are there
any other changes you would like me to make to the code?

-- 
All rights reversed

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

#1323137 — Re: [PATCH 1/4] sched,time: remove non-power-of-two divides from __acct_update_integrals

On Mon, Feb 01, 2016 at 08:44:44AM -0500, Rik van Riel wrote:
> Before I send in a v4 with do_div in that location, are there
> any other changes you would like me to make to the code?

The WARN_ON(!irqs_disabled()) run and adding irq_save/restore to
acct_update_integrals() would be good :-)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web