Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1172593 > unrolled thread

Re: [PATCH] clockevents: return error from tick_broadcast_oneshot_control if !GENERIC_CLOCKEVENTS_BROADCAST

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] clockevents: return error from tick_broadcast_oneshot_control  if !GENERIC_CLOCKEVENTS_BROADCAST Preeti U Murthy <preeti@linux.vnet.ibm.com> - 2015-06-26 07:10 +0200
    Re: [PATCH] clockevents: return error from tick_broadcast_oneshot_control  if !GENERIC_CLOCKEVENTS_BROADCAST Thomas Gleixner <tglx@linutronix.de> - 2015-07-01 11:10 +0200

#1172593 — Re: [PATCH] clockevents: return error from tick_broadcast_oneshot_control if !GENERIC_CLOCKEVENTS_BROADCAST

On 06/25/2015 09:00 PM, Sudeep Holla wrote:
> 
> 
> On 25/06/15 14:55, Thomas Gleixner wrote:
>> On Thu, 25 Jun 2015, Sudeep Holla wrote:
>>
>>> tick_broadcast_enter returns 0 when CPU can switch to broadcast
>>> timer and non-zero otherwise. However when GENERIC_CLOCKEVENTS_BROADCAST
>>> and TICK_ONESHOT are disabled, tick_broadcast_oneshot_control returns 0
>>> which indicates to the CPUIdle framework that the CPU can enter deeper
>>> idle states even when the CPU local timer will be shutdown. If the
>>> target state needs broadcast but not broadcast timer is available, then
>>> the CPU can not resume back from that idle state.
>>>
>>> This patch returns error when there's no broadcast timer support
>>> available so that CPUIdle framework prevents the CPU from entering any
>>> idle states losing the local timer.
>>
>> That's wrong and breaks stuff which does not require the broadcast
>> nonsense.
>>
> 
> OK, sorry for not considering that case.
> 
>> If TICK_ONESHOT is disabled, then everything is in periodic mode and
>> tick_broadcast_enter() rightfully returns 0. Ditto for 'highres=off'
>> on the command line.
>>
>> But there is a case which is not correctly handled right now. That's
>> what you are trying to solve in the wrong way.
>>
> 
> Correct I was trying to solve exactly the case mentioned below.
> 
>> If
>>     GENERIC_CLOCKEVENTS_BROADCAST=n
>>
>> or
>>
>>     GENERIC_CLOCKEVENTS_BROADCAST=y and no broadcast device is available,
>>
>> AND cpu local tick device has the C3STOP flag set,
>>
>> then we have no way to tell the idle code that going deep is not
>> allowed.
>>
>> So we need to be smarter than blindly changing a return
>> value. Completely untested patch below.
>>
> 
> Agreed, thanks for the quick patch, I have tested it and it works fine.
> You can add
> 
> Tested-by: Sudeep Holla <sudeep.holla@arm.com>

What about the case where GENERIC_CLOCKEVENTS_BROADCAST=y and
TICK_ONESHOT=n (HZ_PERIODIC=y) ? Have you tested this ?

This will hang the kernel at boot if you are using the hrtimer mode of
broadcast. This is because the local timers of all cpus are shutdown
when the cpuidle driver registers itself, on finding out that there are
idle states where local tick devices stop. The broadcast tick device is
then in charge of waking up the cpus at every period. In hrtimer mode of
broadcast, there is no such real device and we hang.

There was a patch sent out recently to fix this on powerpc.
https://lkml.org/lkml/2015/6/24/42

Regards
Preeti U Murthy
> 
> Regards,
> Sudeep
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1175105

On Fri, 26 Jun 2015, Thomas Gleixner wrote:
> On Fri, 26 Jun 2015, Preeti U Murthy wrote:
> > On 06/26/2015 01:17 PM, Thomas Gleixner wrote:
> > >  	if (state == TICK_BROADCAST_ENTER) {
> > > +		/*
> > > +		 * If the current CPU owns the hrtimer broadcast
> > > +		 * mechanism, it cannot go deep idle and we do not add
> > > +		 * the CPU to the broadcast mask. We don't have to go
> > > +		 * through the EXIT path as the local timer is not
> > > +		 * shutdown.
> > > +		 */
> > > +		ret = broadcast_needs_cpu(bc, cpu);
> > > +
> > > +		/*
> > > +		 * If the hrtimer broadcast check tells us that the
> > > +		 * cpu cannot go deep idle, or if the broadcast device
> > > +		 * is in periodic mode, we just return.
> > > +		 */
> > > +		if (ret || tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
> > > +			goto out;
> > 
> > The check on PERIODIC mode is good, but I don't see the point of moving
> > broadcast_needs_cpu() up above. broadcast_shutdown_local() calls
> > broadcast_needs_cpu() internally.
> > 
> > Besides, by jumping to 'out', we will miss programming the broadcast
> > hrtimer in tick_broadcast_set_event() below, if the cpu happen to be the
> > broadcast cpu(which is why it was not allowed to go to deep idle).
> 
> Hmm. Need to think a bit more about this convoluted maze ...

Actually, that does not matter at all because the CPU which runs the
broadcast does not need to program its own next event.

That's just redundant because the next event on this cpu is already
programmed in the cpu local timer device and the associated hrtimer is
in the tree. The cpu does not go into deep idle so it just works and
we spare the set/clear bit dance along with the hrtimer update.

Now, there is another caveat. If the cpu is not holding the broadcast
device and has the first expiring timer then the broadcast device
might migrate over and we should clear the cpu in the
broadcast_oneshot_mask.

I so wish we had never invented that broadcast crap at all.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web