It seems that run_hrtimer_queue() is calling get_time() much more often
than it needs to.
With this patch, it only calls get_time() if there's a pending timer.
Following is from a profile done without the patch:
kernel ticks: 30841 1.02 %
13572 44.01 44.01 time_interpolator_get_offset
155 0.50 96.91 hrtimer_run_queues
And with the patch:
kernel ticks: 18334 0.58 %
74 0.40 97.81 hrtimer_run_queues
43 0.23 98.63 time_interpolator_get_offset
Signed-off-by: Dimitri Sivanich <[email protected]>
Index: linux-2.6.15/kernel/hrtimer.c
===================================================================
--- linux-2.6.15.orig/kernel/hrtimer.c 2006-03-23 14:37:49.032686221 -0600
+++ linux-2.6.15/kernel/hrtimer.c 2006-03-23 14:39:10.655542086 -0600
@@ -586,12 +586,17 @@ int hrtimer_get_res(const clockid_t whic
*/
static inline void run_hrtimer_queue(struct hrtimer_base *base)
{
- ktime_t now = base->get_time();
- struct rb_node *node;
+ ktime_t now;
+ struct rb_node *node = base->first;
+
+ if (!node)
+ return;
+
+ now = base->get_time();
spin_lock_irq(&base->lock);
- while ((node = base->first)) {
+ while (node) {
struct hrtimer *timer;
int (*fn)(void *);
int restart;
@@ -620,6 +625,7 @@ static inline void run_hrtimer_queue(str
spin_lock_irq(&base->lock);
+ node = base->first;
/* Another CPU has added back the timer */
if (timer->state != HRTIMER_RUNNING)
continue;
Dimitri Sivanich <[email protected]> wrote:
>
> It seems that run_hrtimer_queue() is calling get_time() much more often
> than it needs to.
>
> With this patch, it only calls get_time() if there's a pending timer.
>
> Following is from a profile done without the patch:
> kernel ticks: 30841 1.02 %
> 13572 44.01 44.01 time_interpolator_get_offset
> 155 0.50 96.91 hrtimer_run_queues
>
> And with the patch:
> kernel ticks: 18334 0.58 %
> 74 0.40 97.81 hrtimer_run_queues
> 43 0.23 98.63 time_interpolator_get_offset
>
>
This code has been extensively redone in -mm and I am planning on sending
all that to Linus within a week.
The hrtimer rework in -mm might fix this performance problem, although from
a quick peek, perhaps not.
So could you please verify that the problem still needs fixing in
2.6.16-mm1 and if so, raise a patch against that?
Thanks.
On Fri, 2006-03-24 at 14:28 -0800, Andrew Morton wrote:
> This code has been extensively redone in -mm and I am planning on sending
> all that to Linus within a week.
>
> The hrtimer rework in -mm might fix this performance problem, although from
> a quick peek, perhaps not.
>
> So could you please verify that the problem still needs fixing in
> 2.6.16-mm1 and if so, raise a patch against that?
I have a look into it.
tglx
On Fri, Mar 24, 2006 at 02:28:49PM -0800, Andrew Morton wrote:
> This code has been extensively redone in -mm and I am planning on sending
> all that to Linus within a week.
>
> The hrtimer rework in -mm might fix this performance problem, although from
> a quick peek, perhaps not.
>
> So could you please verify that the problem still needs fixing in
> 2.6.16-mm1 and if so, raise a patch against that?
>
The hrtimer work in -mm does improve on the situation, although there
appears to be some occasional cache line contention for xtime. The
following patch (which is similiar to my previously submitted patch)
is applicable to 2.6.16-mm1 and does take care of at least a good
portion of that.
Signed-off-by: Dimitri Sivanich <[email protected]>
Index: linux/kernel/hrtimer.c
===================================================================
--- linux.orig/kernel/hrtimer.c 2006-03-27 09:43:40.000000000 -0600
+++ linux/kernel/hrtimer.c 2006-03-27 12:35:47.416054373 -0600
@@ -604,14 +604,17 @@ int hrtimer_get_res(const clockid_t whic
*/
static inline void run_hrtimer_queue(struct hrtimer_base *base)
{
- struct rb_node *node;
+ struct rb_node *node = base->first;
+
+ if (!node)
+ return;
if (base->get_softirq_time)
base->softirq_time = base->get_softirq_time();
spin_lock_irq(&base->lock);
- while ((node = base->first)) {
+ while (node) {
struct hrtimer *timer;
int (*fn)(struct hrtimer *);
int restart;
@@ -633,6 +636,7 @@ static inline void run_hrtimer_queue(str
BUG_ON(hrtimer_active(timer));
enqueue_hrtimer(timer, base);
}
+ node = base->first;
}
set_curr_timer(base, NULL);
spin_unlock_irq(&base->lock);
Dimitri Sivanich a ?crit :
>
> The hrtimer work in -mm does improve on the situation, although there
> appears to be some occasional cache line contention for xtime. The
> following patch (which is similiar to my previously submitted patch)
> is applicable to 2.6.16-mm1 and does take care of at least a good
> portion of that.
I am not sure your patch is correct.
>
> Index: linux/kernel/hrtimer.c
> ===================================================================
> --- linux.orig/kernel/hrtimer.c 2006-03-27 09:43:40.000000000 -0600
> +++ linux/kernel/hrtimer.c 2006-03-27 12:35:47.416054373 -0600
> @@ -604,14 +604,17 @@ int hrtimer_get_res(const clockid_t whic
> */
> static inline void run_hrtimer_queue(struct hrtimer_base *base)
> {
> - struct rb_node *node;
> + struct rb_node *node = base->first;
> +
> + if (!node)
> + return;
>
> if (base->get_softirq_time)
> base->softirq_time = base->get_softirq_time();
>
> spin_lock_irq(&base->lock);
>
> - while ((node = base->first)) {
> + while (node) {
Are you sure of this change ?
base->first may have changed just before you locked the base->lock
(spin_lock_irq(&base->lock);)
Eric
On Tue, Mar 28, 2006 at 07:39:08PM +0200, Eric Dumazet wrote:
> >
> >Index: linux/kernel/hrtimer.c
> >===================================================================
> >--- linux.orig/kernel/hrtimer.c 2006-03-27 09:43:40.000000000 -0600
> >+++ linux/kernel/hrtimer.c 2006-03-27 12:35:47.416054373 -0600
> >@@ -604,14 +604,17 @@ int hrtimer_get_res(const clockid_t whic
> > */
> > static inline void run_hrtimer_queue(struct hrtimer_base *base)
> > {
> >- struct rb_node *node;
> >+ struct rb_node *node = base->first;
> >+
> >+ if (!node)
> >+ return;
> >
> > if (base->get_softirq_time)
> > base->softirq_time = base->get_softirq_time();
> >
> > spin_lock_irq(&base->lock);
> >
> >- while ((node = base->first)) {
> >+ while (node) {
>
> Are you sure of this change ?
>
> base->first may have changed just before you locked the base->lock
> (spin_lock_irq(&base->lock);)
>
We could the following simpler change instead:
Signed-off-by: Dimitri Sivanich <[email protected]>
Index: linux/kernel/hrtimer.c
===================================================================
--- linux.orig/kernel/hrtimer.c 2006-03-28 11:46:45.279722496 -0600
+++ linux/kernel/hrtimer.c 2006-03-28 11:51:36.722469752 -0600
@@ -606,6 +606,9 @@ static inline void run_hrtimer_queue(str
{
struct rb_node *node;
+ if (!base->first)
+ return;
+
if (base->get_softirq_time)
base->softirq_time = base->get_softirq_time();
On Tue, 2006-03-28 at 11:55 -0600, Dimitri Sivanich wrote:
> We could the following simpler change instead:
>
> Signed-off-by: Dimitri Sivanich <[email protected]>
>
> Index: linux/kernel/hrtimer.c
> ===================================================================
> --- linux.orig/kernel/hrtimer.c 2006-03-28 11:46:45.279722496 -0600
> +++ linux/kernel/hrtimer.c 2006-03-28 11:51:36.722469752 -0600
> @@ -606,6 +606,9 @@ static inline void run_hrtimer_queue(str
> {
> struct rb_node *node;
>
> + if (!base->first)
> + return;
> +
> if (base->get_softirq_time)
> base->softirq_time = base->get_softirq_time();
>
Looks much better. Can you please resubmit with a proper patch
description ?
Please add: Signed-off-by: Thomas Gleixner <[email protected]>
tglx
It seems that run_hrtimer_queue() is calling get_softirq_time() more
often than it needs to.
With this patch, it only calls get_softirq_time() if there's a
pending timer.
Signed-off-by: Dimitri Sivanich <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Index: linux/kernel/hrtimer.c
===================================================================
--- linux.orig/kernel/hrtimer.c 2006-03-28 11:46:45.279722496 -0600
+++ linux/kernel/hrtimer.c 2006-03-28 11:51:36.722469752 -0600
@@ -606,6 +606,9 @@ static inline void run_hrtimer_queue(str
{
struct rb_node *node;
+ if (!base->first)
+ return;
+
if (base->get_softirq_time)
base->softirq_time = base->get_softirq_time();