LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	linaro-kernel@lists.linaro.org, linux-kernel@vger.kernel.org,
	Steven Miao <realmz6@gmail.com>
Subject: Re: [PATCH 1/2] timer: Avoid waking up an idle-core by migrate running timer
Date: Wed, 15 Apr 2015 01:13:28 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.11.1504150008470.3845@nanos> (raw)
In-Reply-To: <80182e47a7103608d2ddab7f62c0c3dffc99fdcc.1427782893.git.viresh.kumar@linaro.org>

On Tue, 31 Mar 2015, Viresh Kumar wrote:
> @@ -1189,12 +1195,41 @@ static inline void __run_timers(struct tvec_base *base)
>  			cascade(base, &base->tv5, INDEX(3));
>  		++base->timer_jiffies;
>  		list_replace_init(base->tv1.vec + index, head);
> +
> +again:
>  		while (!list_empty(head)) {
>  			void (*fn)(unsigned long);
>  			unsigned long data;
>  			bool irqsafe;
>  
> -			timer = list_first_entry(head, struct timer_list,entry);
> +			timer = list_first_entry(head, struct timer_list, entry);
> +
> +			if (unlikely(timer_running(timer))) {
> +				/*
> +				 * The only way to get here is if the handler,
> +				 * running on another base, re-queued itself on
> +				 * this base, and the handler hasn't finished
> +				 * yet.
> +				 */
> +
> +				if (list_is_last(&timer->entry, head)) {
> +					/*
> +					 * Drop lock, so that TIMER_RUNNING can
> +					 * be cleared on another base.
> +					 */
> +					spin_unlock(&base->lock);
> +
> +					while (timer_running(timer))
> +						cpu_relax();
> +
> +					spin_lock(&base->lock);
> +				} else {
> +					/* Rotate the list and try someone else */
> +					list_move_tail(&timer->entry, head);
> +				}

Can we please stick that timer into the next bucket and be done with it?

> +				goto again;

"continue;" is old school, right?

> +			}
> +
>  			fn = timer->function;
>  			data = timer->data;
>  			irqsafe = tbase_get_irqsafe(timer->base);
> @@ -1202,6 +1237,7 @@ static inline void __run_timers(struct tvec_base *base)
>  			timer_stats_account_timer(timer);
>  
>  			base->running_timer = timer;
> +			timer_set_running(timer);
>  			detach_expired_timer(timer, base);
>  
>  			if (irqsafe) {
> @@ -1213,6 +1249,25 @@ static inline void __run_timers(struct tvec_base *base)
>  				call_timer_fn(timer, fn, data);
>  				spin_lock_irq(&base->lock);
>  			}
> +
> +			/*
> +			 * Handler running on this base, re-queued itself on
> +			 * another base ?
> +			 */
> +			if (unlikely(timer->base != base)) {
> +				unsigned long flags;
> +				struct tvec_base *tbase;
> +
> +				spin_unlock(&base->lock);
> +
> +				tbase = lock_timer_base(timer, &flags);
> +				timer_clear_running(timer);
> +				spin_unlock(&tbase->lock);
> +
> +				spin_lock(&base->lock);
> +			} else {
> +				timer_clear_running(timer);
> +			}

Aside of the above this is really horrible. Why not doing the obvious:

__mod_timer()

	if (base != newbase) {
	   	if (timer_running()) {
		   list_add(&base->migration_list);
		   goto out_unlock;
		}
		.....

__run_timers()

	while(!list_empty(head)) {
		....
	}

	if (unlikely(!list_empty(&base->migration_list)) {
		/* dequeue and requeue again */
	}

Simple, isn't it?

No new flags in the timer base, no concurrent expiry, no extra
conditionals in the expiry path. Just a single extra check at the end
of the softirq handler for this rare condition instead of imposing all
that nonsense to the hotpath.

We might even optimize that:

	  	if (timer_running()) {
		   list_add(&base->migration_list);
		   base->preferred_target = newbase;
		   goto out_unlock;
		}

	if (unlikely(!list_empty(&base->migration_list)) {
		/* dequeue and requeue again */
		while (!list_empty(&base->migration_list)) {
			dequeue_timer();
			newbase = base->preferred_target;
			unlock(base);
			lock(newbase);
			enqueue(newbase);
			unlock(newbase);
			lock(base);
		}
	}

Thanks,

	tglx

  parent reply	other threads:[~2015-04-14 23:13 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-31  6:55 [PATCH 0/2] timer: Migrate running timers Viresh Kumar
2015-03-31  6:55 ` [PATCH 1/2] timer: Avoid waking up an idle-core by migrate running timer Viresh Kumar
2015-03-31 14:53   ` Peter Zijlstra
2015-04-14 23:13   ` Thomas Gleixner [this message]
2015-04-17  8:12     ` viresh kumar
2015-04-17  8:32       ` Ingo Molnar
2015-04-21 21:32       ` Thomas Gleixner
2015-04-21 21:54         ` Eric Dumazet
2015-04-22 15:29           ` Peter Zijlstra
2015-04-22 16:02             ` Eric Dumazet
2015-04-22 18:56               ` Thomas Gleixner
2015-04-22 19:59                 ` Eric Dumazet
2015-04-22 21:56                   ` Thomas Gleixner
2015-04-23  6:57                     ` Eric Dumazet
2015-04-23 12:45                       ` Thomas Gleixner
2015-04-25 18:37                         ` Eric Dumazet
2015-05-05 13:00                           ` Thomas Gleixner
2015-05-06 16:33                             ` Eric Dumazet
2015-04-15 15:54   ` Thomas Gleixner
2015-03-31  6:55 ` [PATCH 2/2] timer: Replace base-> 'running_timer' with 'busy' Viresh Kumar
2015-03-31 15:01 ` [PATCH 0/2] timer: Migrate running timers Viresh Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.11.1504150008470.3845@nanos \
    --to=tglx@linutronix.de \
    --cc=linaro-kernel@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=realmz6@gmail.com \
    --cc=viresh.kumar@linaro.org \
    --subject='Re: [PATCH 1/2] timer: Avoid waking up an idle-core by migrate running timer' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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