Re: [PATCH 1/2] ia64 clocksource

["Bob Picco" <bob.picco@hp.com>]

Hi:
Thanks for the review.
Hidetoshi Seto wrote:	[Tue Jul 17 2007, 06:55:47AM EDT]
> Bob Picco wrote:
> >@@ -214,61 +209,56 @@ ENTRY(fsys_gettimeofday)
>  :
> > 	movl r27 = xtime
>  :
> > .time_redo:
> >-	.pred.rel.mutex p8,p9,p10
> >-	ld4.acq r28 = [r29]	// xtime_lock.sequence. Must come first for 
> >locking purposes
> >+	ld4.acq r28 = [r20]	// gtod_lock.sequence, Must be first in 
> >struct
>  :
> > 	ld8 r8 = [r27],-IA64_TIMESPEC_TV_NSEC_OFFSET	// xtime.tv_nsec
>  :
> >-	ld4 r10 = [r29]		// xtime_lock.sequence
> >+	ld4 r10 = [r20]		// gtod_lock.sequence, old 
> >xtime_lock.sequence
>  :
> > 	cmp4.ne.or p7,p0 = r28,r10
> >-(p7)	br.cond.dpnt.few .time_redo	// sequence number changed ?
> >+(p7)	br.cond.dpnt.few .time_redo	// sequence number changed, outer 
> >loop2
> 
> This patch removes locking xtime_lock but the code still reads xtime
Well what I see is the update_vsyscall holding the xtime_lock and then
acquiring the fsyscall_gtod_data.lock seqlock. This sequence begins in
do_settimeofday. So the vsyscall could have a tiny window of discrepancy
but miminal. Perhaps John can comment on this.  To me this is no
different than x86_64 but perhaps I'm missing a subtle difference.
> 
> Since gtod_lock.sequence will not tell us whether xtime is updated
> (or going to be updated) while in this window, the result may be wrong...
> 
> 
> > 	// r29 = address of seqlock
>  :
> >-	movl r20 = time_interpolator
> >+	movl r20 = fsyscall_gtod_data // load fsyscall gettimeofday data 
> >address
> > 	;;
> >-	ld8 r20 = [r20]		// get pointer to time_interpolator structure
> >-	movl r29 = xtime_lock
> >-	ld4 r2 = [r2]		// process work pending flags
> >+	add r29 = IA64_ITC_JITTER_OFFSET,r20
> 
> It seems that the comment doesn't match to current usage.
Could you elaborate on this please. I'm missing your point.
> 
> 
> Thanks,
> H.Seto
thanks,

bob