[Linuxptp-devel] Clock sanity checking?

Discussion:

Miroslav Lichvar

2013-10-09 13:30:46 UTC

I was thinking about adding some code to the servo that would check if
the synchronized clock is advancing as expected by comparing it to the
system monotonic clock. It would allow us to see when something else
is touching the clock or the PHC/driver is broken.

We would probably need to move the clock_settime()/clock_adjtime()
calls to the servo first to allow the servo to make these assumptions,
but I was thinking something like this. What do you think?

diff --git a/servo.c b/servo.c
index 45c5832..f5daa9a 100644
--- a/servo.c
+++ b/servo.c
@@ -17,16 +17,26 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include <string.h>
+#include <time.h>

#include "pi.h"
+#include "print.h"
#include "servo_private.h"

struct servo *servo_create(enum servo_type type, int fadj, int max_ppb, int sw_ts)
{
+ struct servo *servo;
+
if (type == CLOCK_SERVO_PI) {
- return pi_servo_create(fadj, max_ppb, sw_ts);
- }
- return NULL;
+ servo = pi_servo_create(fadj, max_ppb, sw_ts);
+ } else
+ return NULL;
+
+ servo->last_mono_ts = 0;
+ servo->last_local_ts = 0;
+ servo->last_fadj = fadj;
+
+ return servo;
}

void servo_destroy(struct servo *servo)
@@ -39,7 +49,42 @@ double servo_sample(struct servo *servo,
uint64_t local_ts,
enum servo_state *state)
{
- return servo->sample(servo, offset, local_ts, state);
+ double fadj;
+ struct timespec now;
+ uint64_t mono_ts;
+
+ clock_gettime(CLOCK_MONOTONIC, &now);
+ mono_ts = now.tv_sec * 1000000000 + now.tv_nsec;
+
+ if (servo->last_local_ts) {
+ if (servo->last_local_ts > local_ts) {
+ pr_warning("unexpected backward clock step!");
+ } else if ((mono_ts - servo->last_mono_ts) *
+ (1.1 - servo->last_fadj / 1e9) <
+ (local_ts - servo->last_local_ts)) {
+ pr_warning("clock running faster than expected!");
+ } else if ((mono_ts - servo->last_mono_ts) *
+ (0.9 + servo->last_fadj / 1e9) >
+ (local_ts - servo->last_local_ts)) {
+ pr_warning("clock running slower than expected!");
+ }
+ }
+
+ fadj = servo->sample(servo, offset, local_ts, state);
+
+ servo->last_mono_ts = mono_ts;
+ servo->last_local_ts = local_ts;
+ servo->last_fadj = fadj;
+
+ switch (*state) {
+ case SERVO_JUMP:
+ servo->last_local_ts -= offset;
+ break;
+ default:
+ break;
+ }
+
+ return fadj;
}

void servo_sync_interval(struct servo *servo, double interval)
diff --git a/servo_private.h b/servo_private.h
index 98c7db2..e6d3ae1 100644
--- a/servo_private.h
+++ b/servo_private.h
@@ -22,6 +22,9 @@
#include "contain.h"

struct servo {
+ uint64_t last_mono_ts;
+ uint64_t last_local_ts;
+ double last_fadj;

void (*destroy)(struct servo *servo);

--
Miroslav Lichvar

Miroslav Lichvar

2013-10-10 11:29:42 UTC

Permalink

(CCing linuxptp-devel back)

I really like this idea. Some nits below, but overall seems good. I do
agree that to properly make the adjustments, the servo would need some
form of callback from the clock.. might not have to totally pull those
tests in..
What if we just had a servo function like "servo_reset_sanity()" which
we called whenever the clock was set or adjusted, and would simply reset
the last_local_ts to 0? It seems to me that would be better overall vs
the alternative of having tighter coupling between the classes?

When not in the unlocked state, every servo call results in a clock
adjustment, so I was considering to give the servo the clock id in its
constructor and let it adjust the clock itself. Do you think we should
avoid that?

Post by Miroslav Lichvar
+ servo = pi_servo_create(fadj, max_ppb, sw_ts);
+ } else
+ return NULL;

a nit here, but I do prefer the kernel style where you add { } around
all blocks if any block requires them.

Ok.

Post by Miroslav Lichvar
+ if (servo->last_local_ts) {
+ if (servo->last_local_ts > local_ts) {
+ pr_warning("unexpected backward clock step!");
+ } else if ((mono_ts - servo->last_mono_ts) *
+ (1.1 - servo->last_fadj / 1e9) <
+ (local_ts - servo->last_local_ts)) {
+ pr_warning("clock running faster than expected!");
+ } else if ((mono_ts - servo->last_mono_ts) *
+ (0.9 + servo->last_fadj / 1e9) >
+ (local_ts - servo->last_local_ts)) {
+ pr_warning("clock running slower than expected!");
+ }

I think here we could make the messages a bit more clearly indicating
they came from the servo? That would be nice.

Ok, prefix the messages with "servo: "?

--
Miroslav Lichvar

Keller, Jacob E

2013-10-10 20:56:24 UTC

Permalink

-----Original Message-----
Sent: Thursday, October 10, 2013 4:30 AM
To: Keller, Jacob E
Subject: Re: [Linuxptp-devel] Clock sanity checking?
(CCing linuxptp-devel back)

Oops.. didn't mean to only email you..

I really like this idea. Some nits below, but overall seems good. I do
agree that to properly make the adjustments, the servo would need

some

form of callback from the clock.. might not have to totally pull those
tests in..
What if we just had a servo function like "servo_reset_sanity()" which
we called whenever the clock was set or adjusted, and would simply

reset

the last_local_ts to 0? It seems to me that would be better overall vs
the alternative of having tighter coupling between the classes?

I just don't like the idea of the servo doing what the clock is supposed
to do. That leads to tighter coupling which makes it harder to re-use code.

It's not really a bigger issue, I think Richard's idea of just doing this
inside the clock isn't a bad thing, either.

Post by Miroslav Lichvar
+ servo = pi_servo_create(fadj, max_ppb, sw_ts);
+ } else
+ return NULL;

a nit here, but I do prefer the kernel style where you add { } around
all blocks if any block requires them.

Ok.

expected!");

Post by Miroslav Lichvar
+ } else if ((mono_ts - servo->last_mono_ts) *
+ (0.9 + servo->last_fadj / 1e9) >
+ (local_ts - servo->last_local_ts)) {
+ pr_warning("clock running slower than

expected!");

Post by Miroslav Lichvar
+ }

I think here we could make the messages a bit more clearly indicating
they came from the servo? That would be nice.

Ok, prefix the messages with "servo: "?

Yes, something like that, or "clock:" if this was moved into the clock.c file?

Regards,
Jake

--
Miroslav Lichvar

Richard Cochran

2013-10-10 18:25:12 UTC

Permalink

Post by Miroslav Lichvar
I was thinking about adding some code to the servo that would check if
the synchronized clock is advancing as expected by comparing it to the
system monotonic clock. It would allow us to see when something else
is touching the clock

Okay.

Post by Miroslav Lichvar
or the PHC/driver is broken.

IMHO, a better way to check for this would be to add a "no adjust"
option to phc2sys and just track the two clocks.

Post by Miroslav Lichvar
We would probably need to move the clock_settime()/clock_adjtime()
calls to the servo first to allow the servo to make these assumptions,
but I was thinking something like this. What do you think?

I don't see why this code cannot live in clock.c or in another module
call by clock_synchronize().

Post by Miroslav Lichvar
+ if (servo->last_local_ts) {
+ if (servo->last_local_ts > local_ts) {
+ pr_warning("unexpected backward clock step!");

Okay, maybe pr_err instead?

Post by Miroslav Lichvar
+ } else if ((mono_ts - servo->last_mono_ts) *
+ (1.1 - servo->last_fadj / 1e9) <
+ (local_ts - servo->last_local_ts)) {

Maybe let the test conditions be configurable?

Post by Miroslav Lichvar
+ pr_warning("clock running faster than expected!");

This will message repeat endlessly whenever it appears, right?

Maybe there is better response to this, like resetting the servo,
switching to no adjust mode, or just ending the program.

Post by Miroslav Lichvar
+ } else if ((mono_ts - servo->last_mono_ts) *
+ (0.9 + servo->last_fadj / 1e9) >
+ (local_ts - servo->last_local_ts)) {
+ pr_warning("clock running slower than expected!");
+ }
+ }

Thanks,
Richard

Miroslav Lichvar

2013-10-11 09:55:48 UTC