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

[Sudeep Holla]

On 26/06/15 13:34, Preeti U Murthy wrote:
> On 06/26/2015 01:17 PM, Thomas Gleixner wrote:
> > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> > index d39f32cdd1b5..281ce29d295e 100644
> > --- a/kernel/time/tick-broadcast.c
> > +++ b/kernel/time/tick-broadcast.c
> > @@ -662,46 +662,39 @@ static void broadcast_shutdown_local(struct clock_event_device *bc,
> >   	clockevents_switch_state(dev, CLOCK_EVT_STATE_SHUTDOWN);
> >   }
> >
> > -/**
> > - * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode
> > - * @state:	The target state (enter/exit)
> > - *
> > - * The system enters/leaves a state, where affected devices might stop
> > - * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups.
> > - *
> > - * Called with interrupts disabled, so clockevents_lock is not
> > - * required here because the local clock event device cannot go away
> > - * under us.
> > - */
> > -int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
> > +int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
> >   {
> >   	struct clock_event_device *bc, *dev;
> > -	struct tick_device *td;
> >   	int cpu, ret = 0;
> >   	ktime_t now;
> >
> > -	/*
> > -	 * Periodic mode does not care about the enter/exit of power
> > -	 * states
> > -	 */
> > -	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
> > -		return 0;
> > +	if (!tick_broadcast_device.evtdev)
> > +		return -EBUSY;
> >
> > -	/*
> > -	 * We are called with preemtion disabled from the depth of the
> > -	 * idle code, so we can't be moved away.
> > -	 */
> > -	td = this_cpu_ptr(&tick_cpu_device);
> > -	dev = td->evtdev;
> > -
> > -	if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
> > -		return 0;
> > +	dev = this_cpu_ptr(&tick_cpu_device)->evtdev;
> >
> >   	raw_spin_lock(&tick_broadcast_lock);
> >   	bc = tick_broadcast_device.evtdev;
> >   	cpu = smp_processor_id();
> >
> >   	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).

I tested the updated patch and found issues. I am seeing some random
behaviour(with GENERIC_CLOCKEVENTS_BROADCAST=y TICK_ONESHOT=y):
1. sometimes all the CPUs have entered deeper idle states(though very
   rare, finding it difficult to hit this scenario)
2. few other times I see one CPU in shallow state which matches the
   above explanation of missing hrtimer programming.

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