* Re: [PATCH resend4 2/3] itimers: fix periodic tics precision [not found] ` <alpine.LFD.2.00.0905221611380.3570@localhost.localdomain> @ 2009-05-25 11:28 ` Stanislaw Gruszka 2009-05-25 12:32 ` Thomas Gleixner 0 siblings, 1 reply; 5+ messages in thread From: Stanislaw Gruszka @ 2009-05-25 11:28 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo, Peter Zijlstra, linuxppc-dev, linux-kernel@vger.kernel.org, Nesterov, Oleg, Molnar, Andrew Morton (linuxppc-dev CC added) On Fri, 22 May 2009 16:27:40 +0200 (CEST) Thomas Gleixner <tglx@linutronix.de> wrote: > > if (cputime_ge(cur_time, it->expires)) { > > - it->expires = it->incr; > > - if (!cputime_eq(it->expires, cputime_zero)) > > - it->expires = cputime_add(it->expires, cur_time); > > + if (!cputime_eq(it->incr, cputime_zero)) { > > + it->expires = cputime_add(it->expires, it->incr); > > + it->error += it->incr_error; > > + if (it->error >= onecputick) { > > + it->expires = cputime_sub(it->expires, > > + jiffies_to_cputime(1)); > > + it->error -= onecputick; > > + } > > Yep, that's a sane solution except for jiffies_to_cputime(), which > can be precomputed as well. I think precomputed is only needed for PPC where jiffies_to_cputime(1) is not compile time constant. To not affect other architectures, I wrote a patch with cputime_one value, it is global variable for PPC and preprocessor definition for others. This patch is against current Linus' tree. I send it as RFC, it was only compile tested for x86. diff --git a/arch/ia64/include/asm/cputime.h b/arch/ia64/include/asm/cputime.h index d20b998..df88b07 100644 --- a/arch/ia64/include/asm/cputime.h +++ b/arch/ia64/include/asm/cputime.h @@ -30,6 +30,7 @@ typedef u64 cputime_t; typedef u64 cputime64_t; #define cputime_zero ((cputime_t)0) +#define cputime_one jiffies_to_cputime(1) #define cputime_max ((~((cputime_t)0) >> 1) - 1) #define cputime_add(__a, __b) ((__a) + (__b)) #define cputime_sub(__a, __b) ((__a) - (__b)) diff --git a/arch/powerpc/include/asm/cputime.h b/arch/powerpc/include/asm/cputime.h index f42e623..e57f951 100644 --- a/arch/powerpc/include/asm/cputime.h +++ b/arch/powerpc/include/asm/cputime.h @@ -48,6 +48,8 @@ typedef u64 cputime64_t; #ifdef __KERNEL__ +extern cputime_t cputime_one; + /* * Convert cputime <-> jiffies */ diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index 48571ac..e46e210 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -498,6 +498,7 @@ static int __init iSeries_tb_recal(void) tb_to_xs = divres.result_low; vdso_data->tb_ticks_per_sec = tb_ticks_per_sec; vdso_data->tb_to_xs = tb_to_xs; + cputime_one = jiffies_to_cputime(1); } else { printk( "Titan recalibrate: FAILED (difference > 4 percent)\n" @@ -904,6 +905,7 @@ void __init time_init(void) tb_ticks_per_usec = ppc_tb_freq / 1000000; tb_to_us = mulhwu_scale_factor(ppc_tb_freq, 1000000); calc_cputime_factors(); + cputime_one = jiffies_to_cputime(1); /* * Calculate the length of each tick in ns. It will not be diff --git a/arch/s390/include/asm/cputime.h b/arch/s390/include/asm/cputime.h index 941384f..f7bbdc9 100644 --- a/arch/s390/include/asm/cputime.h +++ b/arch/s390/include/asm/cputime.h @@ -39,6 +39,7 @@ __div(unsigned long long n, unsigned int base) #endif /* __s390x__ */ #define cputime_zero (0ULL) +#define cputime_one jiffies_to_cputime(1) #define cputime_max ((~0UL >> 1) - 1) #define cputime_add(__a, __b) ((__a) + (__b)) #define cputime_sub(__a, __b) ((__a) - (__b)) diff --git a/include/asm-generic/cputime.h b/include/asm-generic/cputime.h index 1c1fa42..f2b18be 100644 --- a/include/asm-generic/cputime.h +++ b/include/asm-generic/cputime.h @@ -7,6 +7,7 @@ typedef unsigned long cputime_t; #define cputime_zero (0UL) +#define cputime_one (1UL) #define cputime_max ((~0UL >> 1) - 1) #define cputime_add(__a, __b) ((__a) + (__b)) #define cputime_sub(__a, __b) ((__a) - (__b)) diff --git a/kernel/itimer.c b/kernel/itimer.c index 58762f7..ba378c6 100644 --- a/kernel/itimer.c +++ b/kernel/itimer.c @@ -65,7 +65,7 @@ int do_getitimer(int which, struct itimerval *value) thread_group_cputimer(tsk, &cputime); utime = cputime.utime; if (cputime_le(cval, utime)) { /* about to fire */ - cval = jiffies_to_cputime(1); + cval = cputime_one; } else { cval = cputime_sub(cval, utime); } @@ -85,7 +85,7 @@ int do_getitimer(int which, struct itimerval *value) thread_group_cputimer(tsk, ×); ptime = cputime_add(times.utime, times.stime); if (cputime_le(cval, ptime)) { /* about to fire */ - cval = jiffies_to_cputime(1); + cval = cputime_one; } else { cval = cputime_sub(cval, ptime); } @@ -182,8 +182,7 @@ again: if (!cputime_eq(cval, cputime_zero) || !cputime_eq(nval, cputime_zero)) { if (cputime_gt(nval, cputime_zero)) - nval = cputime_add(nval, - jiffies_to_cputime(1)); + nval = cputime_add(nval, cputime_one); set_process_cpu_timer(tsk, CPUCLOCK_VIRT, &nval, &cval); } @@ -204,8 +203,7 @@ again: if (!cputime_eq(cval, cputime_zero) || !cputime_eq(nval, cputime_zero)) { if (cputime_gt(nval, cputime_zero)) - nval = cputime_add(nval, - jiffies_to_cputime(1)); + nval = cputime_add(nval, cputime_one); set_process_cpu_timer(tsk, CPUCLOCK_PROF, &nval, &cval); } diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c index bece7c0..a86333c 100644 --- a/kernel/posix-cpu-timers.c +++ b/kernel/posix-cpu-timers.c @@ -1456,7 +1456,7 @@ void set_process_cpu_timer(struct task_struct *tsk, unsigned int clock_idx, if (!cputime_eq(*oldval, cputime_zero)) { if (cputime_le(*oldval, now.cpu)) { /* Just about to fire. */ - *oldval = jiffies_to_cputime(1); + *oldval = cputime_one; } else { *oldval = cputime_sub(*oldval, now.cpu); } diff --git a/kernel/sched.c b/kernel/sched.c index 26efa47..b17fe3c 100644 --- a/kernel/sched.c +++ b/kernel/sched.c @@ -4726,17 +4726,16 @@ void account_idle_time(cputime_t cputime) */ void account_process_tick(struct task_struct *p, int user_tick) { - cputime_t one_jiffy = jiffies_to_cputime(1); - cputime_t one_jiffy_scaled = cputime_to_scaled(one_jiffy); + cputime_t one_jiffy_scaled = cputime_to_scaled(cputime_one); struct rq *rq = this_rq(); if (user_tick) - account_user_time(p, one_jiffy, one_jiffy_scaled); + account_user_time(p, cputime_one, one_jiffy_scaled); else if ((p != rq->idle) || (irq_count() != HARDIRQ_OFFSET)) - account_system_time(p, HARDIRQ_OFFSET, one_jiffy, + account_system_time(p, HARDIRQ_OFFSET, cputime_one, one_jiffy_scaled); else - account_idle_time(one_jiffy); + account_idle_time(cputime_one); } /* ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH resend4 2/3] itimers: fix periodic tics precision 2009-05-25 11:28 ` [PATCH resend4 2/3] itimers: fix periodic tics precision Stanislaw Gruszka @ 2009-05-25 12:32 ` Thomas Gleixner 2009-05-25 12:51 ` Stanislaw Gruszka 0 siblings, 1 reply; 5+ messages in thread From: Thomas Gleixner @ 2009-05-25 12:32 UTC (permalink / raw) To: Stanislaw Gruszka Cc: Peter Zijlstra, linux-kernel@vger.kernel.org, Oleg Nesterov, linuxppc-dev, Ingo Molnar, Andrew Morton On Mon, 25 May 2009, Stanislaw Gruszka wrote: > @@ -904,6 +905,7 @@ void __init time_init(void) > tb_ticks_per_usec = ppc_tb_freq / 1000000; > tb_to_us = mulhwu_scale_factor(ppc_tb_freq, 1000000); > calc_cputime_factors(); > + cputime_one = jiffies_to_cputime(1); 1) The variable name is misleading. 2) The patch breaks all powerpc platforms which have CONFIG_VIRT_CPU_ACCOUNTING=n and ia64 with CONFIG_VIRT_CPU_ACCOUNTING=y Thanks, tglx ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH resend4 2/3] itimers: fix periodic tics precision 2009-05-25 12:32 ` Thomas Gleixner @ 2009-05-25 12:51 ` Stanislaw Gruszka 2009-05-26 6:44 ` Stanislaw Gruszka 0 siblings, 1 reply; 5+ messages in thread From: Stanislaw Gruszka @ 2009-05-25 12:51 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo, Peter Zijlstra, linuxppc-dev, linux-kernel@vger.kernel.org, Nesterov, Oleg, Molnar, Andrew Morton On Mon, 25 May 2009 14:32:14 +0200 (CEST) Thomas Gleixner <tglx@linutronix.de> wrote: > On Mon, 25 May 2009, Stanislaw Gruszka wrote: > > @@ -904,6 +905,7 @@ void __init time_init(void) > > tb_ticks_per_usec = ppc_tb_freq / 1000000; > > tb_to_us = mulhwu_scale_factor(ppc_tb_freq, 1000000); > > calc_cputime_factors(); > > + cputime_one = jiffies_to_cputime(1); > > 1) The variable name is misleading. What about cputime_one_jiffy ? > 2) The patch breaks all powerpc platforms which have > CONFIG_VIRT_CPU_ACCOUNTING=n and ia64 with > CONFIG_VIRT_CPU_ACCOUNTING=y Stupid me, in asm-generic/cputime.h should be #define cputime_one jiffies_to_cputime(1) Thanks Stanislaw ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH resend4 2/3] itimers: fix periodic tics precision 2009-05-25 12:51 ` Stanislaw Gruszka @ 2009-05-26 6:44 ` Stanislaw Gruszka 2009-05-26 12:04 ` Thomas Gleixner 0 siblings, 1 reply; 5+ messages in thread From: Stanislaw Gruszka @ 2009-05-26 6:44 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo, Peter Zijlstra, linuxppc-dev, linux-kernel@vger.kernel.org, Nesterov, Oleg, Molnar, Andrew Morton On Mon, 25 May 2009 14:51:32 +0200 Stanislaw Gruszka <sgruszka@redhat.com> wrote: > On Mon, 25 May 2009 14:32:14 +0200 (CEST) > Thomas Gleixner <tglx@linutronix.de> wrote: > > > On Mon, 25 May 2009, Stanislaw Gruszka wrote: > > > @@ -904,6 +905,7 @@ void __init time_init(void) > > > tb_ticks_per_usec = ppc_tb_freq / 1000000; > > > tb_to_us = mulhwu_scale_factor(ppc_tb_freq, 1000000); > > > calc_cputime_factors(); > > > + cputime_one = jiffies_to_cputime(1); > > > > 1) The variable name is misleading. > > What about cputime_one_jiffy ? > > > 2) The patch breaks all powerpc platforms which have > > CONFIG_VIRT_CPU_ACCOUNTING=n and ia64 with > > CONFIG_VIRT_CPU_ACCOUNTING=y > > Stupid me, in asm-generic/cputime.h should be > #define cputime_one jiffies_to_cputime(1) Hmmm, I'm confused. Perhaps I missed something, but I think patch was ok. For powerpc and ia64 and CONFIG_VIRT_CPU_ACCOUNTING=n definitions from asm-generic/cputime.h where used. In this file was: #define cputime_one (1UL) and that correct as jiffies_to_cputime(x) is just (x) For CONFIG_VIRT_CPU_ACCOUTING=y: - For powerpc additional variable was declared and computed in initialization time. Declaration of was in __KERENEL__ scope. - For ia64: cputime_one was defined as jiffies_to_cputime(1) Anyway I didn't try to even compile the patch on other architectures than x86. Of cource I will test my patch, but first I would like to know what You think? Does we really need such optimization (because before usage of jiffies_to_cputime(1) was just fine) ? Cheers Stanislaw ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH resend4 2/3] itimers: fix periodic tics precision 2009-05-26 6:44 ` Stanislaw Gruszka @ 2009-05-26 12:04 ` Thomas Gleixner 0 siblings, 0 replies; 5+ messages in thread From: Thomas Gleixner @ 2009-05-26 12:04 UTC (permalink / raw) To: Stanislaw Gruszka Cc: Peter Zijlstra, linux-kernel@vger.kernel.org, Oleg Nesterov, linuxppc-dev, Ingo Molnar, Andrew Morton On Tue, 26 May 2009, Stanislaw Gruszka wrote: > On Mon, 25 May 2009 14:51:32 +0200 > Stanislaw Gruszka <sgruszka@redhat.com> wrote: > > > On Mon, 25 May 2009 14:32:14 +0200 (CEST) > > Thomas Gleixner <tglx@linutronix.de> wrote: > > > > > On Mon, 25 May 2009, Stanislaw Gruszka wrote: > > > > @@ -904,6 +905,7 @@ void __init time_init(void) > > > > tb_ticks_per_usec = ppc_tb_freq / 1000000; > > > > tb_to_us = mulhwu_scale_factor(ppc_tb_freq, 1000000); > > > > calc_cputime_factors(); > > > > + cputime_one = jiffies_to_cputime(1); > > > > > > 1) The variable name is misleading. > > > > What about cputime_one_jiffy ? > > > > > 2) The patch breaks all powerpc platforms which have > > > CONFIG_VIRT_CPU_ACCOUNTING=n and ia64 with > > > CONFIG_VIRT_CPU_ACCOUNTING=y > > > > Stupid me, in asm-generic/cputime.h should be > > #define cputime_one jiffies_to_cputime(1) > > Hmmm, I'm confused. Perhaps I missed something, but I think patch was ok. > For powerpc and ia64 and CONFIG_VIRT_CPU_ACCOUNTING=n definitions from > asm-generic/cputime.h where used. In this file was: > > #define cputime_one (1UL) In time_init you add unconditionally: > > > > @@ -904,6 +905,7 @@ void __init time_init(void) > > > > tb_ticks_per_usec = ppc_tb_freq / 1000000; > > > > tb_to_us = mulhwu_scale_factor(ppc_tb_freq, 1000000); > > > > calc_cputime_factors(); > > > > + cputime_one = jiffies_to_cputime(1); I doubt that the compiler will happy about that as it expands to (1UL) = jiffies_to_cputime(1) or jiffies_to_cputime(1) = jiffies_to_cputime(1) in the CONFIG_VIRT_CPU_ACCOUTING=n case. > and that correct as jiffies_to_cputime(x) is just (x) > > For CONFIG_VIRT_CPU_ACCOUTING=y: > - For powerpc additional variable was declared and computed in > initialization time. Declaration of was in __KERENEL__ scope. > - For ia64: cputime_one was defined as jiffies_to_cputime(1) My bad, I missed the ia64 hunk. > Does we really need such optimization (because before usage of > jiffies_to_cputime(1) was just fine) ? Well, there is a subtle difference between working and nobody noticing the overhead. If we notice such heavy instructions in a code path we change then ignoring the optimization with the argument that it worked before is just plain wrong. Thanks, tglx ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-05-26 12:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20090522154339.06d30f0a@dhcp-lab-109.englab.brq.redhat.com>
[not found] ` <alpine.LFD.2.00.0905221611380.3570@localhost.localdomain>
2009-05-25 11:28 ` [PATCH resend4 2/3] itimers: fix periodic tics precision Stanislaw Gruszka
2009-05-25 12:32 ` Thomas Gleixner
2009-05-25 12:51 ` Stanislaw Gruszka
2009-05-26 6:44 ` Stanislaw Gruszka
2009-05-26 12:04 ` Thomas Gleixner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).