[PATCH] clocksource: em_sti: Adjust clock event rating to fix SMP broadcast

[Simon Horman]

On Wed, Jul 31, 2013 at 01:45:47PM -0700, Stephen Boyd wrote:
> On 07/31/13 12:17, Magnus Damm wrote:
> > Hi Stephen,
> >
> > On Thu, Aug 1, 2013 at 2:32 AM, Stephen Boyd <sboyd at codeaurora.org> wrote:
> >> On 07/30/13 23:25, Simon Horman wrote:
> >>> From: Magnus Damm <damm at opensource.se>
> >>>
> >>> Update the STI rating from 200 to 80 to fix SMP operation with
> >>> the ARM broadcast timer. This breakage was introduced by:
> >>>
> >>> f7db706 ARM: 7674/1: smp: Avoid dummy clockevent being preferred over real hardware clock-event
> >>>
> >>> Without this fix SMP operation is broken on EMEV2 since no
> >>> broadcast timer interrupts trigger on the secondary CPU cores.
> >>>
> >>> Signed-off-by: Magnus Damm <damm at opensource.se>
> >>> Signed-off-by: Simon Horman <horms+renesas at verge.net.au>
> >>> ---
> >> This looks suspicious. Are you're purposefully deflating the rating so
> >> that the STI timer fills in the broadcast position? Why not make the STI
> >> cpumask be all possible CPUs? Presumably the interrupt can target all
> >> CPUs since it isn't a per-cpu interrupt and doing this would cause the
> >> STI to fill in the broadcast slot, leaving the per-cpu dummys in the
> >> tick position.
> > While letting the timer broadcast to all CPUs sounds interesting the
> > STI driver has so far only been used to drive a single CPU core. This
> > used to work well for us but has since some time unfortunately been
> > broken. I agree that it may be suboptimal with a single timer like STI
> > and using IPI for broadcast, but for more efficient SMP we already
> > have TWD or arch timer.
> 
> I think there is some confusion. The mask field says what CPUs the timer
> can possibly interrupt and for non-percpu interrupts this should be all
> possible CPUs (unless we're talking clusters, etc. but I don't think we
> are). Can you please give the output of /proc/timer_list or confirm that
> the STI is your broadcast source? If so you should probably be marking
> the cpumask for all possible CPUs so that the clockevent core knows to
> prefer this clockevent for the broadcast source and not a per-cpu
> source. Then you can leave the rating as is.

>From my point of view this seems to be a case of fixing a regression
by adjusting the code so it has its previous working behaviour.
I think that any work on altering the behaviour of the code
through masks should be considered as future work.