[PATCH] Call get_time() only when necessary in run_hrtimer_queue

[PATCH] Call get_time() only when necessary in run_hrtimer_queue

[Dimitri Sivanich]

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 <sivanich@sgi.com>

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;

Re: [PATCH] Call get_time() only when necessary in run_hrtimer_queue

[Andrew Morton]

Dimitri Sivanich <sivanich@sgi.com> 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.

Re: [PATCH] Call get_time() only when necessary in run_hrtimer_queue

[Thomas Gleixner]

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

Re: [PATCH] Call get_time() only when necessary in run_hrtimer_queue

[Dimitri Sivanich]

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 <sivanich@sgi.com>

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);

Re: [PATCH] Call get_time() only when necessary in run_hrtimer_queue

[Eric Dumazet]

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

Re: [PATCH] Call get_time() only when necessary in run_hrtimer_queue

[Dimitri Sivanich]

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 <sivanich@sgi.com>

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();
 

Re: [PATCH] Call get_time() only when necessary in run_hrtimer_queue

[Thomas Gleixner]

On Tue, 2006-03-28 at 11:55 -0600, Dimitri Sivanich wrote:
> We could the following simpler change instead:
> 
> Signed-off-by: Dimitri Sivanich <sivanich@sgi.com>
> 
> 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 <tglx@linutronix.de>

	tglx

[PATCH] Call get_softirq_time() only when necessary in run_hrtimer_queue()

[Dimitri Sivanich]

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 <sivanich@sgi.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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();