Re: [PATCH V3]hrtimer: Fix a performance regression by disable reprogramming in remove_hrtimer

Re: [PATCH V3]hrtimer: Fix a performance regression by disable reprogramming in remove_hrtimer

[Peter Zijlstra]

On Mon, Sep 09, 2013 at 03:30:44PM +0200, Peter Zijlstra wrote:
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index fa6964d..486c0ba 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -359,6 +359,14 @@ static int intel_idle(struct cpuidle_device *dev,
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
>  
> +	current_thread_into()->status |= TS_POLLING;
> +
> +	/*
> +	 * Order against setting of TS_POLLING against the reading of
> +	 * NEED_RESCHED, matched by resched_task().
> +	 */
> +	smp_mb();
> +
>  	if (!need_resched()) {
>  
>  		__monitor((void *)&current_thread_info()->flags, 0, 0);
> @@ -367,6 +375,8 @@ static int intel_idle(struct cpuidle_device *dev,
>  			__mwait(eax, ecx);
>  	}
>  
> +	current_thread_into()->status &= ~TS_POLLING;
> +
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);

Hmm, arguably it would be better to set this from
intel_idle_cpuidle_driver_init() and clear it whenever this goes away.
Not sure how all that works, the cpuidle driver framework seems 'weird'.

Re: [PATCH V3]hrtimer: Fix a performance regression by disable reprogramming in remove_hrtimer

[Peter Zijlstra]

On Mon, Sep 09, 2013 at 03:46:35PM +0200, Peter Zijlstra wrote:
> On Mon, Sep 09, 2013 at 03:30:44PM +0200, Peter Zijlstra wrote:
> > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> > index fa6964d..486c0ba 100644
> > --- a/drivers/idle/intel_idle.c
> > +++ b/drivers/idle/intel_idle.c
> > @@ -359,6 +359,14 @@ static int intel_idle(struct cpuidle_device *dev,
> >  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
> >  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
> >  
> > +	current_thread_into()->status |= TS_POLLING;
> > +
> > +	/*
> > +	 * Order against setting of TS_POLLING against the reading of
> > +	 * NEED_RESCHED, matched by resched_task().
> > +	 */
> > +	smp_mb();
> > +
> >  	if (!need_resched()) {
> >  
> >  		__monitor((void *)&current_thread_info()->flags, 0, 0);
> > @@ -367,6 +375,8 @@ static int intel_idle(struct cpuidle_device *dev,
> >  			__mwait(eax, ecx);
> >  	}
> >  
> > +	current_thread_into()->status &= ~TS_POLLING;
> > +
> >  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
> >  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
> 
> Hmm, arguably it would be better to set this from
> intel_idle_cpuidle_driver_init() and clear it whenever this goes away.
> Not sure how all that works, the cpuidle driver framework seems 'weird'.

OK, so I went over the idle stuff again, and we do set TS_POLLING like
stuff, it got hidden in current_{clr,set}_polling().

Still if that patch causes extra IPIs its bound to be broken in some
creative way.. I'll prod.

Re: [PATCH V3]hrtimer: Fix a performance regression by disable reprogramming in remove_hrtimer

[Mike Galbraith]

On Wed, 2013-09-11 at 10:56 +0200, Peter Zijlstra wrote: 
> On Mon, Sep 09, 2013 at 03:46:35PM +0200, Peter Zijlstra wrote:
> > On Mon, Sep 09, 2013 at 03:30:44PM +0200, Peter Zijlstra wrote:
> > > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> > > index fa6964d..486c0ba 100644
> > > --- a/drivers/idle/intel_idle.c
> > > +++ b/drivers/idle/intel_idle.c
> > > @@ -359,6 +359,14 @@ static int intel_idle(struct cpuidle_device *dev,
> > >  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
> > >  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
> > >  
> > > +	current_thread_into()->status |= TS_POLLING;
> > > +
> > > +	/*
> > > +	 * Order against setting of TS_POLLING against the reading of
> > > +	 * NEED_RESCHED, matched by resched_task().
> > > +	 */
> > > +	smp_mb();
> > > +
> > >  	if (!need_resched()) {
> > >  
> > >  		__monitor((void *)&current_thread_info()->flags, 0, 0);
> > > @@ -367,6 +375,8 @@ static int intel_idle(struct cpuidle_device *dev,
> > >  			__mwait(eax, ecx);
> > >  	}
> > >  
> > > +	current_thread_into()->status &= ~TS_POLLING;
> > > +
> > >  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
> > >  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
> > 
> > Hmm, arguably it would be better to set this from
> > intel_idle_cpuidle_driver_init() and clear it whenever this goes away.
> > Not sure how all that works, the cpuidle driver framework seems 'weird'.
> 
> OK, so I went over the idle stuff again, and we do set TS_POLLING like
> stuff, it got hidden in current_{clr,set}_polling().
> 
> Still if that patch causes extra IPIs its bound to be broken in some
> creative way.. I'll prod.

(thanks, when I get a break from testing/poking this'n'that, I'll take a
peek too...  well, good_intentions++ anyway;)

Re: [PATCH V3]hrtimer: Fix a performance regression by disable reprogramming in remove_hrtimer

[Ethan Zhao]

Mike, Peter,
   Seems lots of work has been done these days, studious guys. those
patches merged in last stable/dev branch (fix performance regression
caused by extra rtimer programming and rescheduling IPI,confusing
idle... etc) ? So I could just do a lazy pull for test with my
environment.  I need catch up with other mail loops with my vacation
again.

Thanks,
Ethan

On Wed, Sep 11, 2013 at 6:25 PM, Mike Galbraith <bitbucket@online.de> wrote:
> On Wed, 2013-09-11 at 10:56 +0200, Peter Zijlstra wrote:
>> On Mon, Sep 09, 2013 at 03:46:35PM +0200, Peter Zijlstra wrote:
>> > On Mon, Sep 09, 2013 at 03:30:44PM +0200, Peter Zijlstra wrote:
>> > > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> > > index fa6964d..486c0ba 100644
>> > > --- a/drivers/idle/intel_idle.c
>> > > +++ b/drivers/idle/intel_idle.c
>> > > @@ -359,6 +359,14 @@ static int intel_idle(struct cpuidle_device *dev,
>> > >   if (!(lapic_timer_reliable_states & (1 << (cstate))))
>> > >           clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
>> > >
>> > > + current_thread_into()->status |= TS_POLLING;
>> > > +
>> > > + /*
>> > > +  * Order against setting of TS_POLLING against the reading of
>> > > +  * NEED_RESCHED, matched by resched_task().
>> > > +  */
>> > > + smp_mb();
>> > > +
>> > >   if (!need_resched()) {
>> > >
>> > >           __monitor((void *)&current_thread_info()->flags, 0, 0);
>> > > @@ -367,6 +375,8 @@ static int intel_idle(struct cpuidle_device *dev,
>> > >                   __mwait(eax, ecx);
>> > >   }
>> > >
>> > > + current_thread_into()->status &= ~TS_POLLING;
>> > > +
>> > >   if (!(lapic_timer_reliable_states & (1 << (cstate))))
>> > >           clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
>> >
>> > Hmm, arguably it would be better to set this from
>> > intel_idle_cpuidle_driver_init() and clear it whenever this goes away.
>> > Not sure how all that works, the cpuidle driver framework seems 'weird'.
>>
>> OK, so I went over the idle stuff again, and we do set TS_POLLING like
>> stuff, it got hidden in current_{clr,set}_polling().
>>
>> Still if that patch causes extra IPIs its bound to be broken in some
>> creative way.. I'll prod.
>
> (thanks, when I get a break from testing/poking this'n'that, I'll take a
> peek too...  well, good_intentions++ anyway;)
>

Re: [PATCH V3]hrtimer: Fix a performance regression by disable reprogramming in remove_hrtimer

[Mike Galbraith]

On Fri, 2013-10-04 at 20:06 +0800, Ethan Zhao wrote: 
> Mike, Peter,
>    Seems lots of work has been done these days, studious guys. those
> patches merged in last stable/dev branch (fix performance regression
> caused by extra rtimer programming and rescheduling IPI,confusing
> idle... etc) ? So I could just do a lazy pull for test with my
> environment.  I need catch up with other mail loops with my vacation
> again.

Massive timer overhead seems to have crawled off and died while I wasn't
looking.  Peter's fix for IPI woes..

tip commit ea811747 sched, idle: Fix the idle polling state logic

..hasn't yet swum upstream.

-Mike

Re: [PATCH V3]hrtimer: Fix a performance regression by disable reprogramming in remove_hrtimer

[Ethan Zhao]

Got it.

On Mon, Oct 7, 2013 at 12:41 PM, Mike Galbraith <bitbucket@online.de> wrote:
> On Fri, 2013-10-04 at 20:06 +0800, Ethan Zhao wrote:
>> Mike, Peter,
>>    Seems lots of work has been done these days, studious guys. those
>> patches merged in last stable/dev branch (fix performance regression
>> caused by extra rtimer programming and rescheduling IPI,confusing
>> idle... etc) ? So I could just do a lazy pull for test with my
>> environment.  I need catch up with other mail loops with my vacation
>> again.
>
> Massive timer overhead seems to have crawled off and died while I wasn't
> looking.  Peter's fix for IPI woes..
>
> tip commit ea811747 sched, idle: Fix the idle polling state logic
>
> ..hasn't yet swum upstream.
>
> -Mike
>

Re: [PATCH V3]hrtimer: Fix a performance regression by disable reprogramming in remove_hrtimer

[Ethan Zhao]

Mike,
    it seems the IPI issue got root cause (cpu hardware errata ) now ?
I only catch some pieces of the mails, is that to say the crazy horse
made by Intel will wake up unexpected, and eat too much reschedule
IPI, then the horse got hotter, performance down, and so the fix would
be a 'CFLUSH' to all the buggy CPU, is that right ?

Thanks,
Ethan





On Mon, Oct 7, 2013 at 12:57 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
> Got it.
>
> On Mon, Oct 7, 2013 at 12:41 PM, Mike Galbraith <bitbucket@online.de> wrote:
>> On Fri, 2013-10-04 at 20:06 +0800, Ethan Zhao wrote:
>>> Mike, Peter,
>>>    Seems lots of work has been done these days, studious guys. those
>>> patches merged in last stable/dev branch (fix performance regression
>>> caused by extra rtimer programming and rescheduling IPI,confusing
>>> idle... etc) ? So I could just do a lazy pull for test with my
>>> environment.  I need catch up with other mail loops with my vacation
>>> again.
>>
>> Massive timer overhead seems to have crawled off and died while I wasn't
>> looking.  Peter's fix for IPI woes..
>>
>> tip commit ea811747 sched, idle: Fix the idle polling state logic
>>
>> ..hasn't yet swum upstream.
>>
>> -Mike
>>

Re: [PATCH V3]hrtimer: Fix a performance regression by disable reprogramming in remove_hrtimer

[Mike Galbraith]

On Thu, 2013-12-12 at 22:14 +0800, Ethan Zhao wrote: 
> Mike,
>     it seems the IPI issue got root cause (cpu hardware errata ) now ?
> I only catch some pieces of the mails, is that to say the crazy horse
> made by Intel will wake up unexpected, and eat too much reschedule
> IPI, then the horse got hotter, performance down, and so the fix would
> be a 'CFLUSH' to all the buggy CPU, is that right ?

You're mixing a couple different issues together, but yeah, close
enough.  The barking amazonian tree guppy is being saved as we speak.

-Mike