Re: [PATCH NET-NEXT 02/10] time sync: generic infrastructure to map between time stamps generated by a time counter and system time

[John Stultz <johnstul@us.ibm.com>]

On Mon, 2009-02-09 at 22:46 +0100, Patrick Ohly wrote:
> On Mon, 2009-02-09 at 21:27 +0200, John Stultz wrote:
> > On Mon, 2009-02-09 at 18:02 +0100, Patrick Ohly wrote:
> > > Is this revision of the patch okay? How should the two patches get
> > > included in the main kernel - via netdev-next-2.6?
> >
> > Small comment below, but otherwise it looks ok to me. I usually push
> > patches through Andrew, so I'd probably go that way. But I'd leave it to
> > Dave if he's comfortable pushing them to Linus.
> 
> As you don't mind, I suggest to push through Dave as part of the
> complete patch series. That way we don't need to worry about
> coordinating two subtrees.
> 
> > Acked-by: John Stultz <johnstul@us.ibm.com>
> 
> Thanks! Will add that.
> 
> > > +             /* ignore negative durations */
> > > +             sample.duration_target = ktime_to_ns(ktime_sub(end, start));
> > > +             if (sample.duration_target >= 0) {
> > 
> > You may also want to checking the bounds on the duration_target. If
> > preemption hits and the values are too out of whack, the symetric delay
> > assumption below might be quite invalid.
> > 
> > I guess the outliers removal probably covers this as well, but seems
> > some sanity checking might be good.
> 
> That would require more information, like "duration_target is usually in
> the xxx-yyy range". This could be determined based on past measurements
> or the median of the current sample set, but is this really better than
> the current "remove longest 25%"?
> 
> In practice I haven't seen such a problem, therefore I'd prefer to keep
> the code simple and not change it. It was tested under load conditions
> (both CPU and network).

Ok. I was just thinking more along the lines of "throw out duration
times longer then 25us" or 100us or something relatively sane like that.
Again, the largest 25% will probably cover it, but I'm just looking at
this with the realtime preemption patches in mind.

thanks
-john