[RFC 0/7] hrtimer: drop active hrtimer checks after adding it

[RFC 0/7] hrtimer: drop active hrtimer checks after adding it

[Viresh Kumar]

hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The
only special case is when the hrtimer was in past. If it is getting enqueued to
local CPUs's clock-base, we raise a softirq and exit, else we handle that on
next interrupt on remote CPU.

At several places in the kernel, we try to make sure if hrtimer was added
properly or not by calling hrtimer_active(), like:

	hrtimer_start(timer, expires, mode);
	if (hrtimer_active(timer)) {
		/* Added successfully */
	} else {
		/* Was added in the past */
	}

As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'.
So, there is no point calling hrtimer_active().

First patch adds a WARN_ON_ONCE() to __hrtimer_start_range_ns() to make sure
hrtimers are always enqueued from it. Next 6 patches update several parts of
kernel to drop calls to hrtimer_active() after starting a hrtimer.

Rebased over 3.16-rc4 and pushed here:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git hrtimer/drop-hrtimer-active-calls

Cc: Darren Hart <dvhart@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: netdev@vger.kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>

Viresh Kumar (7):
  hrtimer: Warn if hrtimer_start*() failed to enqueue hrtimer
  hrtimer: don't check for active hrtimer after adding it
  tick: don't check for active hrtimer after adding it
  sched: don't check for active hrtimer after adding it
  futex: don't check for active hrtimer after adding it
  rtmutex: don't check for active hrtimer after adding it
  net: don't check for active hrtimer after adding it

 kernel/futex.c           |  5 +----
 kernel/hrtimer.c         |  6 ++----
 kernel/locking/rtmutex.c |  5 +----
 kernel/sched/core.c      | 20 +++++++++-----------
 kernel/sched/deadline.c  |  2 +-
 kernel/time/tick-sched.c | 45 ++++++++++++++++++---------------------------
 net/core/pktgen.c        |  2 --
 7 files changed, 32 insertions(+), 53 deletions(-)

-- 
2.0.0.rc2

[RFC 7/7] net: don't check for active hrtimer after adding it

[Viresh Kumar]

hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The
only special case is when the hrtimer was in past. If it is getting enqueued to
local CPUs's clock-base, we raise a softirq and exit, else we handle that on
next interrupt on remote CPU.

At several places in the kernel, we try to make sure if hrtimer was added
properly or not by calling hrtimer_active(), like:

	hrtimer_start(timer, expires, mode);
	if (hrtimer_active(timer)) {
		/* Added successfully */
	} else {
		/* Was added in the past */
	}

As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'.
So, there is no point calling hrtimer_active().

This patch updates net core to get this fixed.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 net/core/pktgen.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index fc17a9d..f911acd 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -2186,8 +2186,6 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
 		do {
 			set_current_state(TASK_INTERRUPTIBLE);
 			hrtimer_start_expires(&t.timer, HRTIMER_MODE_ABS);
-			if (!hrtimer_active(&t.timer))
-				t.task = NULL;
 
 			if (likely(t.task))
 				schedule();
-- 
2.0.0.rc2

Re: [RFC 7/7] net: don't check for active hrtimer after adding it

[Chris Redpath]

Hi Viresh,

On 09/07/14 07:55, Viresh Kumar wrote:
> hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The
> only special case is when the hrtimer was in past. If it is getting enqueued to
> local CPUs's clock-base, we raise a softirq and exit, else we handle that on
> next interrupt on remote CPU.
>
> At several places in the kernel, we try to make sure if hrtimer was added
> properly or not by calling hrtimer_active(), like:
>
> 	hrtimer_start(timer, expires, mode);
> 	if (hrtimer_active(timer)) {
> 		/* Added successfully */
> 	} else {
> 		/* Was added in the past */
> 	}
>
> As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'.
> So, there is no point calling hrtimer_active().
>
> This patch updates net core to get this fixed.
>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   net/core/pktgen.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index fc17a9d..f911acd 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -2186,8 +2186,6 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t spin_until)
>   		do {
>   			set_current_state(TASK_INTERRUPTIBLE);
>   			hrtimer_start_expires(&t.timer, HRTIMER_MODE_ABS);
> -			if (!hrtimer_active(&t.timer))
> -				t.task = NULL;
>
>   			if (likely(t.task))
>   				schedule();

I think this if condition can also be removed. hrtimer_init_sleeper 
copies the supplied task_struct * to the timer, which in this case is 
'current'. The check is likely to be there in case of !active case you 
removed.

>

--Chris

Re: [RFC 7/7] net: don't check for active hrtimer after adding it

[Viresh Kumar]

Hi Chris,

On 9 July 2014 16:02, Chris Redpath <Chris.Redpath@arm.com> wrote:

>> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
>> index fc17a9d..f911acd 100644
>> --- a/net/core/pktgen.c
>> +++ b/net/core/pktgen.c
>> @@ -2186,8 +2186,6 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t
>> spin_until)
>>                 do {
>>                         set_current_state(TASK_INTERRUPTIBLE);
>>                         hrtimer_start_expires(&t.timer, HRTIMER_MODE_ABS);
>> -                       if (!hrtimer_active(&t.timer))
>> -                               t.task = NULL;
>>
>>                         if (likely(t.task))
>>                                 schedule();
>
>
> I think this if condition can also be removed. hrtimer_init_sleeper copies
> the supplied task_struct * to the timer, which in this case is 'current'.
> The check is likely to be there in case of !active case you removed.

Yeah, it looks like we can get rid of this. Also,

        } while (t.task && pkt_dev->running && !signal_pending(current));

is present in the closing "}" of do-while loop and probably we
don't need to check t.task here as well.

And this review comment applies to patch 2/7 as well:
hrtimer: don't check for active hrtimer after adding it

I would still wait for somebody to prove us wrong :), and will resend
it next week only.

Thanks.

Re: [RFC 7/7] net: don't check for active hrtimer after adding it

[Chris Redpath]

On 09/07/14 11:44, Viresh Kumar wrote:
> Hi Chris,
>
> On 9 July 2014 16:02, Chris Redpath <Chris.Redpath@arm.com> wrote:
>
>>> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
>>> index fc17a9d..f911acd 100644
>>> --- a/net/core/pktgen.c
>>> +++ b/net/core/pktgen.c
>>> @@ -2186,8 +2186,6 @@ static void spin(struct pktgen_dev *pkt_dev, ktime_t
>>> spin_until)
>>>                  do {
>>>                          set_current_state(TASK_INTERRUPTIBLE);
>>>                          hrtimer_start_expires(&t.timer, HRTIMER_MODE_ABS);
>>> -                       if (!hrtimer_active(&t.timer))
>>> -                               t.task = NULL;
>>>
>>>                          if (likely(t.task))
>>>                                  schedule();
>>
>>
>> I think this if condition can also be removed. hrtimer_init_sleeper copies
>> the supplied task_struct * to the timer, which in this case is 'current'.
>> The check is likely to be there in case of !active case you removed.
>
> Yeah, it looks like we can get rid of this. Also,
>
>          } while (t.task && pkt_dev->running && !signal_pending(current));
>
> is present in the closing "}" of do-while loop and probably we
> don't need to check t.task here as well.
>
> And this review comment applies to patch 2/7 as well:
> hrtimer: don't check for active hrtimer after adding it
>
> I would still wait for somebody to prove us wrong :), and will resend
> it next week only.
>
> Thanks.
>

Yeah, no worries. I just happened to read it and not knowing any of the 
APIs had to look up what is going on.

BTW, I *will* get back to you about that broadcast stuff when I get back 
to it myself. Other priorities at the moment again.

--Chris

Re: [RFC 7/7] net: don't check for active hrtimer after adding it

[Viresh Kumar]

On 9 July 2014 16:14, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Yeah, it looks like we can get rid of this. Also,
>
>         } while (t.task && pkt_dev->running && !signal_pending(current));
>
> is present in the closing "}" of do-while loop and probably we
> don't need to check t.task here as well.

Actually No. t.task is modified from hrtimer handler and so this check
would stay:

Diff I have added to this patch:

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f911acd..cc2694e 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -2187,8 +2187,7 @@ static void spin(struct pktgen_dev *pkt_dev,
ktime_t spin_until)
                        set_current_state(TASK_INTERRUPTIBLE);
                        hrtimer_start_expires(&t.timer, HRTIMER_MODE_ABS);

-                       if (likely(t.task))
-                               schedule();
+                       schedule();

                        hrtimer_cancel(&t.timer);
                } while (t.task && pkt_dev->running &&
!signal_pending(current));

Re: [RFC 0/7] hrtimer: drop active hrtimer checks after adding it

[Thomas Gleixner]

On Wed, 9 Jul 2014, Viresh Kumar wrote:

So your patch series drops active hrtimer checks after adding it,
according to your subject line.

Quite useeul to drop something after adding it, right?

> hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The
> only special case is when the hrtimer was in past. If it is getting enqueued to
> local CPUs's clock-base, we raise a softirq and exit, else we handle that on
> next interrupt on remote CPU.
> 
> At several places in the kernel, we try to make sure if hrtimer was added
> properly or not by calling hrtimer_active(), like:
> 
> 	hrtimer_start(timer, expires, mode);
> 	if (hrtimer_active(timer)) {
> 		/* Added successfully */
> 	} else {
> 		/* Was added in the past */
> 	}
> 
> As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'.
> So, there is no point calling hrtimer_active().

Wrong as usual.

It's a common pattern that short timeouts are given which lead to
immediate expiry so the extra round through schedule is even more
pointless than the extra check.

Aside of that it's a long discussed issue that we really should tell
the caller right away that the timer was setup in the past and not
enqueued at all.

That requires to fixup a few call sites, but that'd far more valuable
than removing a few assumed to be pointless checks.

Thnaks,

	tglx

Re: [RFC 0/7] hrtimer: drop active hrtimer checks after adding it

[Frederic Weisbecker]

On Wed, Jul 09, 2014 at 11:30:41PM +0200, Thomas Gleixner wrote:
> On Wed, 9 Jul 2014, Viresh Kumar wrote:
> 
> So your patch series drops active hrtimer checks after adding it,
> according to your subject line.
> 
> Quite useeul to drop something after adding it, right?
> 
> > hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The
> > only special case is when the hrtimer was in past. If it is getting enqueued to
> > local CPUs's clock-base, we raise a softirq and exit, else we handle that on
> > next interrupt on remote CPU.
> > 
> > At several places in the kernel, we try to make sure if hrtimer was added
> > properly or not by calling hrtimer_active(), like:
> > 
> > 	hrtimer_start(timer, expires, mode);
> > 	if (hrtimer_active(timer)) {
> > 		/* Added successfully */
> > 	} else {
> > 		/* Was added in the past */
> > 	}
> > 
> > As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'.
> > So, there is no point calling hrtimer_active().
> 
> Wrong as usual.
> 
> It's a common pattern that short timeouts are given which lead to
> immediate expiry so the extra round through schedule is even more
> pointless than the extra check.

It may be a common pattern but it's not obvious at all as is in the code
except for timers gurus.

It looks like error handling while it's actually an optimization.

Also what about this pattern when it's used in interrupt or interrupt-disabled code?
In this case the handler is not going to fire right away, unless it's enqueued
on another CPU for unpinned timers.

For example this code in tick_nohz_stop_sched_tick():

          hrtimer_start(&ts->sched_timer, expires, HRTIMER_MODE_ABS_PINNED);
          /* Check, if the timer was already in the past */
          if (hrtimer_active(&ts->sched_timer))
                 goto out;

It's not clear what this is handling. Concurrent immediate callback expiration from another CPU?
But the timer is pinned local so it can't execute right away between hrtimer_start() and hrtimer_active()
check...

Re: [RFC 0/7] hrtimer: drop active hrtimer checks after adding it

[Viresh Kumar]

Hi Thomas,

On 10 July 2014 07:04, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Wed, Jul 09, 2014 at 11:30:41PM +0200, Thomas Gleixner wrote:
>> On Wed, 9 Jul 2014, Viresh Kumar wrote:
>>
>> So your patch series drops active hrtimer checks after adding it,
>> according to your subject line.
>>
>> Quite useeul to drop something after adding it, right?

I meant "hrtimer" by "it". Will fix it in case this patchset is still required.

>> > As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'.
>> > So, there is no point calling hrtimer_active().
>>
>> Wrong as usual.

I cross-checked this with Frederic and Preeti before reaching out to
you, to make sure its not 'obviously stupid'. And still couldn't get it
right. :(

>> It's a common pattern that short timeouts are given which lead to
>> immediate expiry so the extra round through schedule is even more
>> pointless than the extra check.

Just wanted to confirm it again, you are talking about CPU being
interrupted by clockevent device's interrupt right after hrtimer_start*()
returns and before calling hrtimer_active()?

> It may be a common pattern but it's not obvious at all as is in the code
> except for timers gurus.
>
> It looks like error handling while it's actually an optimization.
>
> Also what about this pattern when it's used in interrupt or interrupt-disabled code?
> In this case the handler is not going to fire right away, unless it's enqueued
> on another CPU for unpinned timers.
>
> For example this code in tick_nohz_stop_sched_tick():
>
>           hrtimer_start(&ts->sched_timer, expires, HRTIMER_MODE_ABS_PINNED);
>           /* Check, if the timer was already in the past */
>           if (hrtimer_active(&ts->sched_timer))
>                  goto out;
>
> It's not clear what this is handling. Concurrent immediate callback expiration from another CPU?
> But the timer is pinned local so it can't execute right away between hrtimer_start() and hrtimer_active()
> check...

Actually I was concerned about other cases as well.

- Timeouts

I do agree that an extra check is better than an extra round of schedule().
But this is already achieved without calling hrtimer_active(), isn't it?

All these timeout hrtimers have hrtimer_wakeup() as there handler (as
these are initialized with: hrtimer_init_sleeper()).

And on expiration hrtimer_wakeup() does this:  t->task = NULL;

So would this extra call to hrtimer_active() make any difference?

- Process-context: sched changes

I am not sure if scheduler routines: start_bandwidth_timer() and
start_dl_timer() would get called *only* with interrupts disabled.

But, it doesn't look obvious that the optimization Thomas mentioned
earlier is relevant here as well. These might be added here for error
checking.

I might be wrong here as I don't have any understanding of this code
and so sorry in advance.


Note: My tree is monitored by kbuild-bot and these changes are under
testing for over a week now. And I haven't received any reports of the
WARN() firing in __hrtimer_start_range_ns().. Probably these short
timeouts aren't getting hit at all by bot's tests.

--
viresh