Date:	Tue, 28 Dec 2010 12:20:00 +0100
From:	Richard Cochran <richardcochran@...il.com>
To:	John Stultz <john.stultz@...aro.org>
Cc:	linux-kernel@...r.kernel.org,
	Richard Cochran <richard.cochran@...cron.at>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 0/3] Cleanup ADJ_SETOFFSET patch

On Mon, Dec 27, 2010 at 03:40:41PM -0800, John Stultz wrote:
> I was looking to queue Richard's ADJ_SETOFFSET patch to see
> if it could be merged into -tip for 2.6.38, but on second 
> glance I noticed the ugly local_irq_disable bits as well
> as the fact that adding the offset uses a gettime/add/settime
> pattern which will also add a small error as the action isn't
> atomic.
> 
> So I implemented a timekeeping function to add a fixed offset:
> timekeeping_inject_offset(), and reworked Richard's code to
> make use of it.

Okay, so you added an optimized version of do_settimeofday() for
jumping the clock. It certainly helps the code in do_adjtimex(), but
it also (nearly) duplicates do_settimeofday(). I guess you will not
mind having to maintain both code paths...

> Richard: Any objection here? Mind testing this with the rest
> of your patch queue?

Well, you have uncovered a problem.

The code I offered was broken to begin with, but I think your patch is
also troubled. The timespec is awkwardly split into seconds and
nanoseconds, and I think that arithmetic using timespecs is not well
defined. Or perhaps only I am confused by it all.

The problem seems to be, how can a timespec have a sign?

The exisiting code seems to assume that a timespec can only have the
sign in one field. In other words, if tv_sec is non-zero, then tv_nsec
must be non-negative. (I added a check for this into my patch).

I took a second look at ktime_add() vs. timespec_add() and discovered
a problems both. Consider the following test code:

static void kt_add(struct timespec now, struct timespec adj)
{
	ktime_t delta, kt;
	struct timespec ts;
	delta = timespec_to_ktime(adj);
	kt = timespec_to_ktime(now);
	kt = ktime_add(kt, delta);
	ts = ktime_to_timespec(kt);
	pr_err("kt add: {%ld,%ld} + {%ld,%ld} = {%ld,%ld}\n",
	       now.tv_sec, now.tv_nsec,
	       adj.tv_sec, adj.tv_nsec,
	       ts.tv_sec, ts.tv_nsec);
}

static void ts_add(struct timespec now, struct timespec adj)
{
	struct timespec ts;
	ts = timespec_add(now, adj);
	pr_err("ts add: {%ld,%ld} + {%ld,%ld} = {%ld,%ld}\n",
	       now.tv_sec, now.tv_nsec,
	       adj.tv_sec, adj.tv_nsec,
	       ts.tv_sec, ts.tv_nsec);
}

There are (at least) four cases to consider:

1. adj > 1.0

   kt add: {2,0} + {1,100} = {3,100}                                               
   ts add: {2,0} + {1,100} = {3,100}                                               

2. adj < -1.0

   kt add: {2,0} + {-1,100} = {1,100}                                              
   ts add: {2,0} + {-1,100} = {1,100}                                              

3. 0 < adj < 1.0

   kt add: {2,0} + {0,100} = {2,100}                                               
   ts add: {2,0} + {0,100} = {2,100}                                               

4. -1.0 < adj < 0

   kt add: {2,0} + {0,-100} = {6,294967196}                                        
   ts add: {2,0} + {0,-100} = {1,999999900}                                        

Case 2 fails for both functions.
Case 4 fails when using ktime_add().

So it seems that I have now way to correctly encode a negative offset
less than -1.0 into a timespec. Perhaps we could specify new rules for
timespecs.

1. Time Values:
   If tv_sec is non-zero, then tv_nsec must be non-negative.

2. Time Deltas:
   The sign of tv_sec and tv_nsec must be the same.

In any case, I would like you help in clarifying all of this...

Thanks,

Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists