[Linuxptp-devel] [PATCH RFC 1/2] Add reset function to servo.

[Miroslav Lichvar]

This will be used in clock sanity check.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
pi.c | 8 ++++++++
servo.c | 5 +++++
servo.h | 6 ++++++
servo_private.h | 2 ++
4 files changed, 21 insertions(+)

diff --git a/pi.c b/pi.c
index 3c8a635..bd78e40 100644
--- a/pi.c
+++ b/pi.c
@@ -171,6 +171,13 @@ static void pi_sync_interval(struct servo *servo, double interval)
interval, s->kp, s->ki);
}

+static void pi_reset(struct servo *servo)
+{
+ struct pi_servo *s = container_of(servo, struct pi_servo, servo);
+
+ s->count = 0;
+}
+
struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
{
struct pi_servo *s;
@@ -182,6 +189,7 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->servo.destroy = pi_destroy;
s->servo.sample = pi_sample;
s->servo.sync_interval = pi_sync_interval;
+ s->servo.reset = pi_reset;
s->drift = fadj;
s->maxppb = max_ppb;
s->first_update = 1;
diff --git a/servo.c b/servo.c
index 45c5832..5af020a 100644
--- a/servo.c
+++ b/servo.c
@@ -46,3 +46,8 @@ void servo_sync_interval(struct servo *servo, double interval)
{
servo->sync_interval(servo, interval);
}
+
+void servo_reset(struct servo *servo)
+{
+ servo->reset(servo);
+}
diff --git a/servo.h b/servo.h
index 052740e..7a5d0d3 100644
--- a/servo.h
+++ b/servo.h
@@ -93,4 +93,10 @@ double servo_sample(struct servo *servo,
*/
void servo_sync_interval(struct servo *servo, double interval);

+/**
+ * Reset a clock servo.
+ * @param servo Pointer to a servo obtained via @ref servo_create().
+ */
+void servo_reset(struct servo *servo);
+
#endif
diff --git a/servo_private.h b/servo_private.h
index 98c7db2..82e2bf5 100644
--- a/servo_private.h
+++ b/servo_private.h
@@ -30,6 +30,8 @@ struct servo {
enum servo_state *state);

void (*sync_interval)(struct servo *servo, double interval);
+
+ void (*reset)(struct servo *servo);
};

#endif

[Miroslav Lichvar]

Check the sanity of the synchronized clock by comparing its uncorrected
frequency with the system monotonic clock. When the measured frequency
offset is larger than the value of the sanity_freq_limit option (20% by
default), a warning message will be printed and the servo will be reset.
Setting the option to zero disables the check.

This is useful to detect when the clock is broken or adjusted by another
program.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
config.c | 6 +++++
config.h | 2 ++
default.cfg | 1 +
gPTP.cfg | 1 +
ptp4l.8 | 7 ++++++
ptp4l.c | 2 ++
servo.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
servo.h | 7 ++++++
servo_private.h | 5 ++++
9 files changed, 103 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 8cae57f..7365bcc 100644
--- a/config.c
+++ b/config.c
@@ -373,6 +373,12 @@ static enum parser_result parse_global_setting(const char *option,
return r;
*cfg->pi_max_frequency = val;

+ } else if (!strcmp(option, "sanity_freq_limit")) {
+ r = get_ranged_int(value, &val, 0, INT_MAX);
+ if (r != PARSED_OK)
+ return r;
+ *cfg->sanity_freq_limit = val;
+
} else if (!strcmp(option, "ptp_dst_mac")) {
if (MAC_LEN != sscanf(value, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
&mac[0], &mac[1], &mac[2], &mac[3], &mac[4], &mac[5]))
diff --git a/config.h b/config.h
index 94704a4..9e0d6e6 100644
--- a/config.h
+++ b/config.h
@@ -86,6 +86,8 @@ struct config {
double *pi_f_offset_const;
int *pi_max_frequency;

+ int *sanity_freq_limit;
+
unsigned char *ptp_dst_mac;
unsigned char *p2p_dst_mac;
unsigned char *udp6_scope;
diff --git a/default.cfg b/default.cfg
index 72665a6..7ff66fb 100644
--- a/default.cfg
+++ b/default.cfg
@@ -52,6 +52,7 @@ pi_offset_const 0.0
pi_f_offset_const 0.0000001
pi_max_frequency 900000000
clock_servo pi
+sanity_freq_limit 200000000
#
# Transport options
#
diff --git a/gPTP.cfg b/gPTP.cfg
index 30719b6..a3ddd9d 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -51,6 +51,7 @@ pi_offset_const 0.0
pi_f_offset_const 0.0000001
pi_max_frequency 900000000
clock_servo pi
+sanity_freq_limit 200000000
#
# Transport options
#
diff --git a/ptp4l.8 b/ptp4l.8
index 493626d..c887b29 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -340,6 +340,13 @@ The maximum allowed frequency adjustment of the clock in parts per billion
set to 0, the hardware limit will be used.
The default is 900000000 (90%).
.TP
+.B sanity_freq_limit
+The maximum allowed frequency offset between uncorrected clock and the system
+monotonic clock in parts per billion (ppb). This is used in a sanity check of
+the clock. When a larger offset is measured, a warning message will be printed
+and the servo will be reset. When set to 0, the sanity check is disabled.
+The default is 200000000 (20%).
+.TP
.B ptp_dst_mac
The MAC address where should be PTP messages sent.
Relevant only with L2 transport. The default is 01:1B:19:00:00:00.
diff --git a/ptp4l.c b/ptp4l.c
index b0d1c9c..fc14071 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -108,6 +108,8 @@ static struct config cfg_settings = {
.pi_f_offset_const = &configured_pi_f_offset,
.pi_max_frequency = &configured_pi_max_freq,

+ .sanity_freq_limit = &sanity_freq_limit,
+
.ptp_dst_mac = ptp_dst_mac,
.p2p_dst_mac = p2p_dst_mac,
.udp6_scope = &udp6_scope,
diff --git a/servo.c b/servo.c
index 5af020a..ab459ab 100644
--- a/servo.c
+++ b/servo.c
@@ -17,16 +17,32 @@
* 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"

+#define SANITY_CHECK_MIN_INTERVAL 10000000
+#define SANITY_CHECK_MAX_FREQ 900000000
+
+int sanity_freq_limit = 200000000; /* ppb */
+
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);
+ servo = pi_servo_create(fadj, max_ppb, sw_ts);
+ } else {
+ return NULL;
}
- return NULL;
+
+ servo->last_mono_ts = 0;
+ servo->last_local_ts = 0;
+ servo->last_freq = fadj;
+
+ return servo;
}

void servo_destroy(struct servo *servo)
@@ -39,7 +55,60 @@ double servo_sample(struct servo *servo,
uint64_t local_ts,
enum servo_state *state)
{
- return servo->sample(servo, offset, local_ts, state);
+ struct timespec now;
+ double freq, foffset;
+ uint64_t mono_ts;
+ int64_t mono_diff, local_diff;
+
+ if (sanity_freq_limit) {
+ /* Check the sanity of the synchronized clock by comparing its
+ uncorrected frequency with the system monotonic clock. If
+ the synchronized clock is the system clock, the measured
+ frequency offset will be the current frequency correction of
+ the system clock. */
+ clock_gettime(CLOCK_MONOTONIC, &now);
+ mono_ts = now.tv_sec * 1000000000LL + now.tv_nsec;
+ mono_diff = (int64_t)mono_ts - servo->last_mono_ts;
+ local_diff = (int64_t)local_ts - servo->last_local_ts;
+
+ if (servo->last_local_ts &&
+ servo->last_freq <= SANITY_CHECK_MAX_FREQ &&
+ mono_diff >= SANITY_CHECK_MIN_INTERVAL) {
+ foffset = 1e9 * (local_diff /
+ (1.0 - servo->last_freq / 1e9) /
+ mono_diff - 1.0);
+
+ if (foffset > sanity_freq_limit) {
+ pr_warning("servo: clock jumped forward or"
+ " running faster than expected!");
+ servo->reset(servo);
+ } else if (foffset < -sanity_freq_limit) {
+ pr_warning("servo: clock jumped backward or"
+ " running slower than expected!");
+ servo->reset(servo);
+ }
+ }
+
+ servo->last_mono_ts = mono_ts;
+ servo->last_local_ts = local_ts;
+ }
+
+ freq = servo->sample(servo, offset, local_ts, state);
+
+ if (sanity_freq_limit) {
+ switch (*state) {
+ case SERVO_JUMP:
+ servo->last_local_ts -= offset;
+ /* Fall through. */
+ case SERVO_LOCKED:
+ servo->last_freq = freq;
+ break;
+ default:
+ break;
+ }
+ }
+
+ return freq;
}

void servo_sync_interval(struct servo *servo, double interval)
diff --git a/servo.h b/servo.h
index 7a5d0d3..275dbeb 100644
--- a/servo.h
+++ b/servo.h
@@ -22,6 +22,13 @@

#include <stdint.h>

+/**
+ * When set to a non-zero value, this variable determines the
+ * maximum allowed frequency offset between uncorrected clock and
+ * the system monotonic clock (in ppb).
+ */
+extern int sanity_freq_limit;
+
/** Opaque type */
struct servo;

diff --git a/servo_private.h b/servo_private.h
index 82e2bf5..55b0c0c 100644
--- a/servo_private.h
+++ b/servo_private.h
@@ -32,6 +32,11 @@ struct servo {
void (*sync_interval)(struct servo *servo, double interval);

void (*reset)(struct servo *servo);
+
+ /* Used for clock sanity checking. */
+ uint64_t last_mono_ts;
+ uint64_t last_local_ts;
+ double last_freq;
};

#endif

[Miroslav Lichvar]

Post by Miroslav Lichvar
Check the sanity of the synchronized clock by comparing its uncorrected
frequency with the system monotonic clock. When the measured frequency
offset is larger than the value of the sanity_freq_limit option (20% by
default), a warning message will be printed and the servo will be reset.
Setting the option to zero disables the check.

I've just realized the servo is normally called when the follow up
message is received and not on the sync message. This means the local
time stamp passed the servo is not the current time of the PTP clock.
If the network jitter is in hundreds of milliseconds it could
trigger a false positive in the sanity check (or possibly break a
servo using the time stamps).

Could we use the time stamp of the follow up message instead?

[Miroslav Lichvar]

Post by Miroslav Lichvar
I've just realized the servo is normally called when the follow up
message is received and not on the sync message. This means the local
time stamp passed the servo is not the current time of the PTP clock.
If the network jitter is in hundreds of milliseconds it could
trigger a false positive in the sanity check (or possibly break a
servo using the time stamps).
Could we use the time stamp of the follow up message instead?

On second thought, it's probably better to move the sanity checking
code out of the servo and call it when the sync message is received.
I'll look into that.

[Richard Cochran]

Post by Miroslav Lichvar

Post by Miroslav Lichvar
I've just realized the servo is normally called when the follow up
message is received and not on the sync message. This means the local
time stamp passed the servo is not the current time of the PTP clock.
If the network jitter is in hundreds of milliseconds it could
trigger a false positive in the sanity check (or possibly break a
servo using the time stamps).
Could we use the time stamp of the follow up message instead?

On second thought, it's probably better to move the sanity checking
code out of the servo and call it when the sync message is received.
I'll look into that.

Why not just take the CLOCK_MONOTONIC - PHC offset periodically (in
the manner of phc2sys) and then just look at the slope?

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar

Post by Miroslav Lichvar
I've just realized the servo is normally called when the follow up
message is received and not on the sync message. This means the local
time stamp passed the servo is not the current time of the PTP clock.
If the network jitter is in hundreds of milliseconds it could
trigger a false positive in the sanity check (or possibly break a
servo using the time stamps).
Could we use the time stamp of the follow up message instead?

On second thought, it's probably better to move the sanity checking
code out of the servo and call it when the sync message is received.
I'll look into that.

Why not just take the CLOCK_MONOTONIC - PHC offset periodically (in
the manner of phc2sys) and then just look at the slope?

You mean read the PHC directly? I think we should avoid that as it
may be slow. The code I proposed has the problem that the
CLOCK_MONOTONIC - PHC offset includes the random fup-sync delay. I'm
not trying to create a new module for the sanity checker and call it
right when a PHC time stamp is made.