[PATCH] sys_time-speedup-small-cleanup

[PATCH] sys_time-speedup-small-cleanup

[Oleg Nesterov]

on top of sys_time-speedup.patch

Ingo Molnar wrote:
>
>  asmlinkage long sys_time(time_t __user * tloc)
>  {
> -	time_t i;
> -	struct timeval tv;
> +	/*
> +	 * We read xtime.tv_sec atomically - it's updated
> +	 * atomically by update_wall_time(), so no need to
> +	 * even read-lock the xtime seqlock:
> +	 */
> +	time_t i = xtime.tv_sec;
>  
> -	do_gettimeofday(&tv);
> -	i = tv.tv_sec;
> +	smp_rmb(); /* sys_time() results are coherent */

Why do we need this barrier? My guess it is needed to prevent
the reading of xtime.tv_sec twice, yes? In that case a simple
barrier() should be enough.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- t/kernel/time.c~	2007-06-26 16:28:59.000000000 +0400
+++ t/kernel/time.c	2007-06-26 16:32:09.000000000 +0400
@@ -64,7 +64,7 @@ asmlinkage long sys_time(time_t __user *
 	 */
 	time_t i = xtime.tv_sec;
 
-	smp_rmb(); /* sys_time() results are coherent */
+	barrier(); /* sys_time() results are coherent */
 
 	if (tloc) {
 		if (put_user(i, tloc))

Re: [PATCH] sys_time-speedup-small-cleanup

[Chris Snook]

Oleg Nesterov wrote:
> on top of sys_time-speedup.patch
> 
> Ingo Molnar wrote:
>>  asmlinkage long sys_time(time_t __user * tloc)
>>  {
>> -	time_t i;
>> -	struct timeval tv;
>> +	/*
>> +	 * We read xtime.tv_sec atomically - it's updated
>> +	 * atomically by update_wall_time(), so no need to
>> +	 * even read-lock the xtime seqlock:
>> +	 */
>> +	time_t i = xtime.tv_sec;
>>  
>> -	do_gettimeofday(&tv);
>> -	i = tv.tv_sec;
>> +	smp_rmb(); /* sys_time() results are coherent */
> 
> Why do we need this barrier? My guess it is needed to prevent
> the reading of xtime.tv_sec twice, yes? In that case a simple
> barrier() should be enough.

Without the smp_rmb, you can potentially have a situation where one CPU is still 
reading an old value from cache while another has the new value.  It's generally 
a rather small race window on most architectures, but very bad things can happen 
if time ever goes backwards, so it's worth the overhead of maintaining coherence 
on smp.

	-- Chris

> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> --- t/kernel/time.c~	2007-06-26 16:28:59.000000000 +0400
> +++ t/kernel/time.c	2007-06-26 16:32:09.000000000 +0400
> @@ -64,7 +64,7 @@ asmlinkage long sys_time(time_t __user *
>  	 */
>  	time_t i = xtime.tv_sec;
>  
> -	smp_rmb(); /* sys_time() results are coherent */
> +	barrier(); /* sys_time() results are coherent */
>  
>  	if (tloc) {
>  		if (put_user(i, tloc))
> 
> -
> 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/

Re: [PATCH] sys_time-speedup-small-cleanup

[Oleg Nesterov]

On 06/26, Chris Snook wrote:
>
> Oleg Nesterov wrote:
> >on top of sys_time-speedup.patch
> >
> >Ingo Molnar wrote:
> >> asmlinkage long sys_time(time_t __user * tloc)
> >> {
> >>-	time_t i;
> >>-	struct timeval tv;
> >>+	/*
> >>+	 * We read xtime.tv_sec atomically - it's updated
> >>+	 * atomically by update_wall_time(), so no need to
> >>+	 * even read-lock the xtime seqlock:
> >>+	 */
> >>+	time_t i = xtime.tv_sec;
> >> 
> >>-	do_gettimeofday(&tv);
> >>-	i = tv.tv_sec;
> >>+	smp_rmb(); /* sys_time() results are coherent */
> >
> >Why do we need this barrier? My guess it is needed to prevent
> >the reading of xtime.tv_sec twice, yes? In that case a simple
> >barrier() should be enough.
> 
> Without the smp_rmb, you can potentially have a situation where one CPU is 
> still reading an old value from cache while another has the new value.  

I can't understand this.

Fisrt, smp_rmb() can't help in this case. It can't influence the preceeding
LOAD if it was from cache.

Even if it could, another CPU can alter the value just after the reading
completes, and we have the same situation.

Could you please clarify if I am wrong?

Oleg.

Re: [PATCH] sys_time-speedup-small-cleanup

[Chris Snook]

Oleg Nesterov wrote:
> On 06/26, Chris Snook wrote:
>> Oleg Nesterov wrote:
>>> on top of sys_time-speedup.patch
>>>
>>> Ingo Molnar wrote:
>>>> asmlinkage long sys_time(time_t __user * tloc)
>>>> {
>>>> -	time_t i;
>>>> -	struct timeval tv;
>>>> +	/*
>>>> +	 * We read xtime.tv_sec atomically - it's updated
>>>> +	 * atomically by update_wall_time(), so no need to
>>>> +	 * even read-lock the xtime seqlock:
>>>> +	 */
>>>> +	time_t i = xtime.tv_sec;
>>>>
>>>> -	do_gettimeofday(&tv);
>>>> -	i = tv.tv_sec;
>>>> +	smp_rmb(); /* sys_time() results are coherent */
>>> Why do we need this barrier? My guess it is needed to prevent
>>> the reading of xtime.tv_sec twice, yes? In that case a simple
>>> barrier() should be enough.
>> Without the smp_rmb, you can potentially have a situation where one CPU is 
>> still reading an old value from cache while another has the new value.  
> 
> I can't understand this.
> 
> Fisrt, smp_rmb() can't help in this case. It can't influence the preceeding
> LOAD if it was from cache.
> 
> Even if it could, another CPU can alter the value just after the reading
> completes, and we have the same situation.
> 
> Could you please clarify if I am wrong?
> 
> Oleg.
> 

You're right, but so is Ingo's patch.  We're not trying to enforce some notion 
of absolute time, just  make it possible for userspace to guarantee that time 
cannot be *observed* to travel backwards.  It's still the responsibility of the 
user to use proper synchronization in multithreaded apps.  Without the smp_rmb() 
it would be possible on some architectures for the results of the race you 
describe to leak across other lock-prefixed instructions used to ensure 
monotonicity in userspace.  Relativity applies to SMP timekeeping, not just 
space travelers, so if there's no way to prove a race occurred, it doesn't 
matter whether or not it occurred in some frame of reference.

	-- Chris

Re: [PATCH] sys_time-speedup-small-cleanup

[Oleg Nesterov]

On 06/27, Chris Snook wrote:
>
> Oleg Nesterov wrote:
> >On 06/26, Chris Snook wrote:
> >>Oleg Nesterov wrote:
> >>>on top of sys_time-speedup.patch
> >>>
> >>>Ingo Molnar wrote:
> >>>>asmlinkage long sys_time(time_t __user * tloc)
> >>>>{
> >>>>-	time_t i;
> >>>>-	struct timeval tv;
> >>>>+	/*
> >>>>+	 * We read xtime.tv_sec atomically - it's updated
> >>>>+	 * atomically by update_wall_time(), so no need to
> >>>>+	 * even read-lock the xtime seqlock:
> >>>>+	 */
> >>>>+	time_t i = xtime.tv_sec;
> >>>>
> >>>>-	do_gettimeofday(&tv);
> >>>>-	i = tv.tv_sec;
> >>>>+	smp_rmb(); /* sys_time() results are coherent */
> >>>Why do we need this barrier? My guess it is needed to prevent
> >>>the reading of xtime.tv_sec twice, yes? In that case a simple
> >>>barrier() should be enough.
> >>Without the smp_rmb, you can potentially have a situation where one CPU 
> >>is still reading an old value from cache while another has the new value. 
> >
> >I can't understand this.
> >
> >Fisrt, smp_rmb() can't help in this case. It can't influence the preceeding
> >LOAD if it was from cache.
> >
> >Even if it could, another CPU can alter the value just after the reading
> >completes, and we have the same situation.
> >
> >Could you please clarify if I am wrong?
> >
> >Oleg.
> >
> 
> You're right, but so is Ingo's patch.  We're not trying to enforce some 
> notion of absolute time, just  make it possible for userspace to guarantee 
> that time cannot be *observed* to travel backwards.  It's still the 
> responsibility of the user to use proper synchronization in multithreaded 
> apps.  Without the smp_rmb() it would be possible on some architectures for 
> the results of the race you describe to leak across other lock-prefixed 
> instructions used to ensure monotonicity in userspace.  Relativity applies 
> to SMP timekeeping, not just space travelers, so if there's no way to prove 
> a race occurred, it doesn't matter whether or not it occurred in some frame 
> of reference.

This doesn't make sense to me, sorry. Could provide more details to explain
the race? Some ascii diagram?

I believe smp_rmb() can't make _any_ difference in this case.

Oleg.