[PATCH] ARM: sched_clock: Load cycle count after epoch stabilizes

[PATCH] ARM: sched_clock: Load cycle count after epoch stabilizes

[Stephen Boyd]

There is a small race between when the cycle count is read from
the hardware and when the epoch stabilizes. Consider this
scenario:

 CPU0                           CPU1
 ----                           ----
 cyc = read_sched_clock()
 cyc_to_sched_clock()
                                 update_sched_clock()
                                  ...
                                  cd.epoch_cyc = cyc;
  epoch_cyc = cd.epoch_cyc;
  ...
  epoch_ns + cyc_to_ns((cyc - epoch_cyc)

The cyc on cpu0 was read before the epoch changed. But we
calculate the nanoseconds based on the new epoch by subtracting
the new epoch from the old cycle count. Since epoch is most likely
larger than the old cycle count we calculate a large number that
will be converted to nanoseconds and added to epoch_ns, causing
time to jump forward too much.

Fix this problem by reading the hardware after the epoch has
stabilized.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

Found this while reading through the code. I haven't actually
seen it in practice but I think it's real.

 arch/arm/kernel/sched_clock.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
index e8edcaa..a57cc5d 100644
--- a/arch/arm/kernel/sched_clock.c
+++ b/arch/arm/kernel/sched_clock.c
@@ -51,10 +51,11 @@ static inline u64 notrace cyc_to_ns(u64 cyc, u32 mult, u32 shift)
 	return (cyc * mult) >> shift;
 }
 
-static unsigned long long notrace cyc_to_sched_clock(u32 cyc, u32 mask)
+static unsigned long long notrace sched_clock_32(void)
 {
 	u64 epoch_ns;
 	u32 epoch_cyc;
+	u32 cyc;
 
 	if (cd.suspended)
 		return cd.epoch_ns;
@@ -73,7 +74,9 @@ static unsigned long long notrace cyc_to_sched_clock(u32 cyc, u32 mask)
 		smp_rmb();
 	} while (epoch_cyc != cd.epoch_cyc_copy);
 
-	return epoch_ns + cyc_to_ns((cyc - epoch_cyc) & mask, cd.mult, cd.shift);
+	cyc = read_sched_clock();
+	cyc = (cyc - epoch_cyc) & sched_clock_mask;
+	return epoch_ns + cyc_to_ns(cyc, cd.mult, cd.shift);
 }
 
 /*
@@ -165,12 +168,6 @@ void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
 	pr_debug("Registered %pF as sched_clock source\n", read);
 }
 
-static unsigned long long notrace sched_clock_32(void)
-{
-	u32 cyc = read_sched_clock();
-	return cyc_to_sched_clock(cyc, sched_clock_mask);
-}
-
 unsigned long long __read_mostly (*sched_clock_func)(void) = sched_clock_32;
 
 unsigned long long notrace sched_clock(void)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] ARM: sched_clock: Load cycle count after epoch stabilizes

[Stephen Boyd]

John,

I just saw your pull request for making this code generic. I believe
this patch fixes a bug that nobody has seen in practice so it's probably
fine to delay this until 3.11.

Also, I've just noticed that "ARM: sched_clock: Return suspended count
earlier" that I sent in that series is going to break the arm
architected timer path because they're circumventing all this epoch_ns
code. It would be better if you could replace that patch with this patch
because this optimizes it in the same way and also fixes a bug at the
same time.

Thanks,
Stephen

On 06/12/13 17:10, Stephen Boyd wrote:
> There is a small race between when the cycle count is read from
> the hardware and when the epoch stabilizes. Consider this
> scenario:
>
>  CPU0                           CPU1
>  ----                           ----
>  cyc = read_sched_clock()
>  cyc_to_sched_clock()
>                                  update_sched_clock()
>                                   ...
>                                   cd.epoch_cyc = cyc;
>   epoch_cyc = cd.epoch_cyc;
>   ...
>   epoch_ns + cyc_to_ns((cyc - epoch_cyc)
>
> The cyc on cpu0 was read before the epoch changed. But we
> calculate the nanoseconds based on the new epoch by subtracting
> the new epoch from the old cycle count. Since epoch is most likely
> larger than the old cycle count we calculate a large number that
> will be converted to nanoseconds and added to epoch_ns, causing
> time to jump forward too much.
>
> Fix this problem by reading the hardware after the epoch has
> stabilized.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>
> Found this while reading through the code. I haven't actually
> seen it in practice but I think it's real.
>
>  arch/arm/kernel/sched_clock.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
> index e8edcaa..a57cc5d 100644
> --- a/arch/arm/kernel/sched_clock.c
> +++ b/arch/arm/kernel/sched_clock.c
> @@ -51,10 +51,11 @@ static inline u64 notrace cyc_to_ns(u64 cyc, u32 mult, u32 shift)
>  	return (cyc * mult) >> shift;
>  }
>  
> -static unsigned long long notrace cyc_to_sched_clock(u32 cyc, u32 mask)
> +static unsigned long long notrace sched_clock_32(void)
>  {
>  	u64 epoch_ns;
>  	u32 epoch_cyc;
> +	u32 cyc;
>  
>  	if (cd.suspended)
>  		return cd.epoch_ns;
> @@ -73,7 +74,9 @@ static unsigned long long notrace cyc_to_sched_clock(u32 cyc, u32 mask)
>  		smp_rmb();
>  	} while (epoch_cyc != cd.epoch_cyc_copy);
>  
> -	return epoch_ns + cyc_to_ns((cyc - epoch_cyc) & mask, cd.mult, cd.shift);
> +	cyc = read_sched_clock();
> +	cyc = (cyc - epoch_cyc) & sched_clock_mask;
> +	return epoch_ns + cyc_to_ns(cyc, cd.mult, cd.shift);
>  }
>  
>  /*
> @@ -165,12 +168,6 @@ void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
>  	pr_debug("Registered %pF as sched_clock source\n", read);
>  }
>  
> -static unsigned long long notrace sched_clock_32(void)
> -{
> -	u32 cyc = read_sched_clock();
> -	return cyc_to_sched_clock(cyc, sched_clock_mask);
> -}
> -
>  unsigned long long __read_mostly (*sched_clock_func)(void) = sched_clock_32;
>  
>  unsigned long long notrace sched_clock(void)


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] ARM: sched_clock: Load cycle count after epoch stabilizes

[John Stultz]

On 06/17/2013 12:51 PM, Stephen Boyd wrote:
> John,
>
> I just saw your pull request for making this code generic. I believe
> this patch fixes a bug that nobody has seen in practice so it's probably
> fine to delay this until 3.11.
>
> Also, I've just noticed that "ARM: sched_clock: Return suspended count
> earlier" that I sent in that series is going to break the arm
> architected timer path because they're circumventing all this epoch_ns
> code. It would be better if you could replace that patch with this patch
> because this optimizes it in the same way and also fixes a bug at the
> same time.

Sorry, could you clarify a bit more? The above sounds like there are two 
issues, but you only sent one patch.

I'm also not sure how to proceed with the patch you sent, since it 
collides with the patch that moves sched_clock to be generic.

Could you refactor the change on-top of git branch I sent to Thomas? 
Otherwise I'll have to withdraw the pull request, and we'll probably 
miss 3.11 for the generic sched_clock change.

thanks
-john

[PATCH] ARM: sched_clock: Load cycle count after epoch stabilizes

[Stephen Boyd]

On 06/17/13 14:50, John Stultz wrote:
> On 06/17/2013 12:51 PM, Stephen Boyd wrote:
>> John,
>>
>> I just saw your pull request for making this code generic. I believe
>> this patch fixes a bug that nobody has seen in practice so it's probably
>> fine to delay this until 3.11.
>>
>> Also, I've just noticed that "ARM: sched_clock: Return suspended count
>> earlier" that I sent in that series is going to break the arm
>> architected timer path because they're circumventing all this epoch_ns
>> code. It would be better if you could replace that patch with this patch
>> because this optimizes it in the same way and also fixes a bug at the
>> same time.
>
> Sorry, could you clarify a bit more? The above sounds like there are
> two issues, but you only sent one patch.

I'm saying that the "return the suspended count earlier" patch is bad.
But this patch does what that patch is doing plus it fixes a race at the
same time. So it would be better to just take this patch and drop the other.

The problem is that when the cd.suspended flag is true we're going to
return the jiffies based sched_clock value for the ARM architected timer
driver (the only driver that replaces the sched_clock_func function
pointer with its own function). Jiffies should be pretty close to the
actual time anyway, but since the ARM architected timer driver is
already broken with respect to suspend (because it doesn't stop the
sched_clock value from incrementing during suspend) it doesn't really
matter. Every other driver using the sched_clock code will not be
affected either way.

>
> I'm also not sure how to proceed with the patch you sent, since it
> collides with the patch that moves sched_clock to be generic.

Ah I thought that the git rename detection would just make it work.

>
> Could you refactor the change on-top of git branch I sent to Thomas?
> Otherwise I'll have to withdraw the pull request, and we'll probably
> miss 3.11 for the generic sched_clock change.

Sure. I'll send it right now.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation