Date:	Mon, 14 Jul 2014 10:11:35 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Frederic Weisbecker <fweisbec@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>
Cc:	Lists linaro-kernel <linaro-kernel@...ts.linaro.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Arvind Chauhan <arvind.chauhan@....com>,
	Preeti U Murthy <preeti@...ux.vnet.ibm.com>,
	Kevin Hilman <khilman@...aro.org>,
	Darren Hart <dvhart@...ux.intel.com>,
	"David S. Miller" <davem@...emloft.net>,
	Ingo Molnar <mingo@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [RFC 0/7] hrtimer: drop active hrtimer checks after adding it

Hi Thomas,

On 10 July 2014 07:04, Frederic Weisbecker <fweisbec@...il.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists