Date: Wed, 1 Jul 2015 11:08:49 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Preeti U Murthy <preeti@linux.vnet.ibm.com>
cc: Sudeep Holla <sudeep.holla@arm.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Suzuki Poulose <Suzuki.Poulose@arm.com>,
        Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
        Catalin Marinas <Catalin.Marinas@arm.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Subject: Re: [PATCH] clockevents: return error from tick_broadcast_oneshot_control
 if !GENERIC_CLOCKEVENTS_BROADCAST
In-Reply-To: <alpine.DEB.2.11.1506261437280.4037@nanos>
Message-ID: <alpine.DEB.2.11.1507011046420.3916@nanos>
References: <1435228065-2428-1-git-send-email-sudeep.holla@arm.com> <alpine.DEB.2.11.1506251359360.4037@nanos> <558C1E97.4020206@arm.com> <558CDE57.3050008@linux.vnet.ibm.com> <alpine.DEB.2.11.1506260924200.4037@nanos> <558D46C5.8080307@linux.vnet.ibm.com>
 <alpine.DEB.2.11.1506261437280.4037@nanos>
User-Agent: Alpine 2.11 (DEB 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2238
Lines: 54

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/