Thanks for the feedback Erez,
My goal of this initial patch was to give an API to extract the statistics that
are already calculated/existing in the clock module and already printed to the
logs, i.e. the statistics that generate the numbers in this log message:
/* Path delay stats are updated separately, they may be empty. */
if (c->delay_stats_valid) {
pr_info("rms %4.0f max %4.0f "
"freq %+6.0f +/- %3.0f "
"delay %5.0f +/- %3.0f",
c->offset_stats.rms, c->offset_stats.max_abs,
c->freq_stats.mean, c->freq_stats.stddev,
c->delay_stats.mean, c->delay_stats.stddev);
} else {
pr_info("rms %4.0f max %4.0f "
"freq %+6.0f +/- %3.0f",
c->offset_stats.rms, c->offset_stats.max_abs,
c->freq_stats.mean, c->freq_stats.stddev);
The log message looks like this:
ptp4l[5024110.731]: rms 9 max 17 freq -17594 +/- 14 delay 220 +/- 1
One method of course would be to scrape the logs for these 6 numbers, but when
I looked into the implementation I found the other statistics so I decided to
include all of them in the CLOCK_STATS_NP TLV for thoroughness:
struct stats_result {
double min;
double max;
double max_abs;
double mean;
double rms;
double stddev;
};
As far as I can tell, the PARENT_DATA_SET TLV isn’t actually implemented?
/* Initialize the parentDS. */
clock_update_grandmaster(c);
c->dad.pds.parentStats = 0;
c->dad.pds.observedParentOffsetScaledLogVariance = 0xffff;
c->dad.pds.observedParentClockPhaseChangeRate = 0x7fffffff;
Doing a pmc get confirms this, at least for my setup:
sending: GET PARENT_DATA_SET
0c42a1.fffe.d1d1bd-0 seq 0 RESPONSE MANAGEMENT PARENT_DATA_SET
…
parentStats 0
observedParentOffsetScaledLogVariance 0xffff
observedParentClockPhaseChangeRate 0x7fffffff
…
After reading the relevant 1588-2019 paragraphs, I’m not 100% sure the existing
calculation of freq_stats and offset_stats are transformable to
“observedParentClockPhaseChangeRate” and
“observedParentOffsetScaledLogVariance”, respectively. Modification of these
existing stats could be future work after a more careful review of 1588-2019
(i.e. section 7.6.3, etc).
Thanks,
Tim
From: Geva, Erez <erez.geva....@siemens.com>
Date: Wednesday, May 26, 2021 at 4:09 AM
To: Tim Martin <timot...@nvidia.com>
Cc: linuxptp-devel@lists.sourceforge.net <linuxptp-devel@lists.sourceforge.net>
Subject: RE: [Linuxptp-devel] [PATCH] Add new CLOCK_STATS_NP TLV GET to pmc and
clock
External email: Use caution opening links or attachments
In IEEE 1588-2019
I see PARENT_DATA_SET with
- observedParentOffsetScaledLogVariance
- observedParentClockPhaseChangeRate
You use a natural statistics syntax.
Perhaps you may use a more coherent phrasing that match the standard.
May be something that match the performanceMonitoringDS, like
- averageMasterSlaveDelay
- minMasterSlaveDelay
...
Erez
-----Original Message-----
From: Tim Martin <timot...@nvidia.com>
Sent: Tuesday, 25 May 2021 22:29
To: linuxptp-devel@lists.sourceforge.net
Subject: [Linuxptp-devel] [PATCH] Add new CLOCK_STATS_NP TLV GET to pmc and
clock
Currently there is no way to programmatically access statistics about the clock
frequency offset, time delay, or time offset (collectively, the "clock_stats"
metrics), except for parsing the ptp4l logs. One option for time offset would
be to poll TLV_TIME_STATUS_NP in regular intervals from a custom client, but
that has the disadvantage of either very high poll rates or missed updates
which would affect the statistics.
To address these issues, introduce a management mesage, CLOCK_STATS_NP, which
reports the statistics that would be displayed in the ptp4l log file. This
fixes the above problem by allowing a custom client to poll CLOCK_STATS_NP at
longer intervals without missing any updates.
Follow-up patches could then introduc knobs to configure the time over which
CLOCK_STATS_NP integrates statistics, separate from any other tracking loop
updates, which would allow even longer polling intervals.
Signed-off-by: Tim Martin <timot...@nvidia.com>
---
clock.c | 50 ++++++++++++++++++++++++++++++++++----------------
pmc.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
pmc_common.c | 1 +
tlv.h | 10 ++++++++++
4 files changed, 93 insertions(+), 16 deletions(-)
diff --git a/clock.c b/clock.c
index e545a9b..550d222 100644
--- a/clock.c
+++ b/clock.c
@@ -129,6 +129,9 @@ struct clock {
double nrr;
struct clock_description desc;
struct clock_stats stats;
+ struct stats_result offset_stats, freq_stats, delay_stats;
+ int offset_freq_stats_valid;
+ int delay_stats_valid;
int stats_interval;
struct clockcheck *sanity_check;
struct interface *uds_rw_if;
@@ -144,7 +147,7 @@ struct clock the_clock; static void
handle_state_decision_event(struct clock *c); static int
clock_resize_pollfd(struct clock *c, int new_nports); static void
clock_remove_port(struct clock *c, struct port *p); -static void
clock_stats_display(struct clock_stats *s);
+static void clock_stats_display(struct clock *c);
static void remove_subscriber(struct clock_subscriber *s) { @@ -351,6 +354,7
@@ static int clock_management_fill_response(struct clock *c, struct port *p,
struct tlv_extra *extra;
struct PTPText *text;
uint16_t duration;
+ struct clock_stats_np_tlv *stats;
int datalen = 0;
extra = tlv_extra_alloc();
@@ -462,6 +466,15 @@ static int clock_management_fill_response(struct clock *c,
struct port *p,
mtd->val = c->local_sync_uncertain;
datalen = sizeof(*mtd);
break;
+ case TLV_CLOCK_STATS_NP:
+ stats = (struct clock_stats_np_tlv *) tlv->data;
+ stats->offset_stats = c->offset_stats;
+ stats->freq_stats = c->freq_stats;
+ stats->delay_stats = c->delay_stats;
+ stats->offset_freq_stats_valid = c->offset_freq_stats_valid;
+ stats->delay_stats_valid = c->delay_stats_valid;
+ datalen = sizeof(*stats);
+ break;
default:
/* The caller should *not* respond to this message. */
tlv_extra_recycle(extra);
@@ -544,7 +557,7 @@ static int clock_management_set(struct clock *c, struct
port *p,
/* Display stats on change of local_sync_uncertain */
if (c->local_sync_uncertain != mtd->val
&& stats_get_num_values(c->stats.offset))
- clock_stats_display(&c->stats);
+ clock_stats_display(c);
c->local_sync_uncertain = mtd->val;
respond = 1;
break;
@@ -556,38 +569,41 @@ static int clock_management_set(struct clock *c, struct
port *p,
return respond ? 1 : 0;
}
-static void clock_stats_update(struct clock_stats *s,
+static void clock_stats_update(struct clock *c,
double offset, double freq)
{
+ struct clock_stats *s = &c->stats;
stats_add_value(s->offset, offset);
stats_add_value(s->freq, freq);
if (stats_get_num_values(s->offset) < s->max_count)
return;
- clock_stats_display(s);
+ clock_stats_display(c);
}
-static void clock_stats_display(struct clock_stats *s)
+static void clock_stats_display(struct clock *c)
{
- struct stats_result offset_stats, freq_stats, delay_stats;
+ struct clock_stats *s = &c->stats;
- stats_get_result(s->offset, &offset_stats);
- stats_get_result(s->freq, &freq_stats);
+ stats_get_result(s->offset, &c->offset_stats);
+ stats_get_result(s->freq, &c->freq_stats);
+ c->offset_freq_stats_valid = 1;
+ c->delay_stats_valid = !stats_get_result(s->delay, &c->delay_stats);
/* Path delay stats are updated separately, they may be empty. */
- if (!stats_get_result(s->delay, &delay_stats)) {
+ if (c->delay_stats_valid) {
pr_info("rms %4.0f max %4.0f "
"freq %+6.0f +/- %3.0f "
"delay %5.0f +/- %3.0f",
- offset_stats.rms, offset_stats.max_abs,
- freq_stats.mean, freq_stats.stddev,
- delay_stats.mean, delay_stats.stddev);
+ c->offset_stats.rms, c->offset_stats.max_abs,
+ c->freq_stats.mean, c->freq_stats.stddev,
+ c->delay_stats.mean, c->delay_stats.stddev);
} else {
pr_info("rms %4.0f max %4.0f "
"freq %+6.0f +/- %3.0f",
- offset_stats.rms, offset_stats.max_abs,
- freq_stats.mean, freq_stats.stddev);
+ c->offset_stats.rms, c->offset_stats.max_abs,
+ c->freq_stats.mean, c->freq_stats.stddev);
}
stats_reset(s->offset);
@@ -638,7 +654,7 @@ static enum servo_state clock_no_adjust(struct clock *c,
tmv_t ingress,
freq = (1.0 - ratio) * 1e9;
if (c->stats.max_count > 1) {
- clock_stats_update(&c->stats, tmv_dbl(c->master_offset), freq);
+ clock_stats_update(c, tmv_dbl(c->master_offset), freq);
} else {
pr_info("master offset %10" PRId64 " s%d freq %+7.0f "
"path delay %9" PRId64,
@@ -1190,6 +1206,8 @@ struct clock *clock_create(enum clock_type type, struct
config *config,
pr_err("failed to create stats");
return NULL;
}
+ c->offset_freq_stats_valid = 0;
+ c->delay_stats_valid = 0;
sfl = config_get_int(config, NULL, "sanity_freq_limit");
if (sfl) {
c->sanity_check = clockcheck_create(sfl); @@ -1857,7 +1875,7 @@
enum servo_state clock_synchronize(struct clock *c, tmv_t ingress, tmv_t origin)
}
if (c->stats.max_count > 1) {
- clock_stats_update(&c->stats, tmv_dbl(c->master_offset), adj);
+ clock_stats_update(c, tmv_dbl(c->master_offset), adj);
} else {
pr_info("master offset %10" PRId64 " s%d freq %+7.0f "
"path delay %9" PRId64,
diff --git a/pmc.c b/pmc.c
index 00d6014..a405f65 100644
--- a/pmc.c
+++ b/pmc.c
@@ -155,6 +155,7 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
struct parentDS *pds;
struct portDS *p;
struct TLV *tlv;
+ struct clock_stats_np_tlv *stats;
int action;
if (msg_type(msg) == SIGNALING) {
@@ -525,6 +526,53 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
fprintf(fp, "LOG_MIN_PDELAY_REQ_INTERVAL "
IFMT "logMinPdelayReqInterval %hhd", mtd->val);
break;
+ case TLV_CLOCK_STATS_NP:
+ stats = (struct clock_stats_np_tlv *) mgt->data;
+ fprintf(fp, "CLOCK_STATS_NP " IFMT);
+ if (stats->offset_freq_stats_valid) {
+ fprintf(fp,
+ "offset min %lf"
+ IFMT "offset max %lf"
+ IFMT "offset max_abs %lf"
+ IFMT "offset mean %lf"
+ IFMT "offset rms %lf"
+ IFMT "offset stdev %lf"
+ IFMT "freq min %lf"
+ IFMT "freq max %lf"
+ IFMT "freq max_abs %lf"
+ IFMT "freq mean %lf"
+ IFMT "freq rms %lf"
+ IFMT "freq stdev %lf"
+ IFMT,
+ stats->offset_stats.min,
+ stats->offset_stats.max,
+ stats->offset_stats.max_abs,
+ stats->offset_stats.mean,
+ stats->offset_stats.rms,
+ stats->offset_stats.stddev,
+ stats->freq_stats.min,
+ stats->freq_stats.max,
+ stats->freq_stats.max_abs,
+ stats->freq_stats.mean,
+ stats->freq_stats.rms,
+ stats->freq_stats.stddev);
+ }
+ if (stats->delay_stats_valid) {
+ fprintf(fp,
+ "delay min %lf"
+ IFMT "delay max %lf"
+ IFMT "delay max_abs %lf"
+ IFMT "delay mean %lf"
+ IFMT "delay rms %lf"
+ IFMT "delay stdev %lf",
+ stats->delay_stats.min,
+ stats->delay_stats.max,
+ stats->delay_stats.max_abs,
+ stats->delay_stats.mean,
+ stats->delay_stats.rms,
+ stats->delay_stats.stddev);
+ }
+ break;
}
out:
fprintf(fp, "\n");
diff --git a/pmc_common.c b/pmc_common.c index 101df55..a97e156 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -132,6 +132,7 @@ struct management_id idtab[] = {
{ "PORT_DATA_SET_NP", TLV_PORT_DATA_SET_NP, do_set_action },
{ "PORT_STATS_NP", TLV_PORT_STATS_NP, do_get_action },
{ "PORT_PROPERTIES_NP", TLV_PORT_PROPERTIES_NP, do_get_action },
+ { "CLOCK_STATS_NP", TLV_CLOCK_STATS_NP, do_get_action },
};
static void do_get_action(struct pmc *pmc, int action, int index, char *str)
diff --git a/tlv.h b/tlv.h index a205119..58857b2 100644
--- a/tlv.h
+++ b/tlv.h
@@ -24,6 +24,7 @@
#include "ddt.h"
#include "ds.h"
+#include "stats.h"
/* TLV types */
#define TLV_MANAGEMENT 0x0001
@@ -100,6 +101,7 @@ enum management_action {
#define TLV_GRANDMASTER_SETTINGS_NP 0xC001
#define TLV_SUBSCRIBE_EVENTS_NP 0xC003
#define TLV_SYNCHRONIZATION_UNCERTAIN_NP 0xC006
+#define TLV_CLOCK_STATS_NP 0xC007
/* Port management ID values */
#define TLV_NULL_MANAGEMENT 0x0000
@@ -180,6 +182,14 @@ struct management_tlv_datum {
uint8_t reserved;
} PACKED;
+struct clock_stats_np_tlv {
+ struct stats_result offset_stats;
+ struct stats_result freq_stats;
+ struct stats_result delay_stats;
+ uint8_t offset_freq_stats_valid;
+ uint8_t delay_stats_valid;
+} PACKED;
+
struct management_error_status {
Enumeration16 type;
UInteger16 length;
--
2.31.1
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.sourceforge.net%2Flists%2Flistinfo%2Flinuxptp-devel&data=04%7C01%7Ctimothym%40nvidia.com%7C969837d3e5dd4b02f94d08d92036c211%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637576241821860507%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gizbmB6l0a8hTbCuv9Rh5akGM0%2B4dlQli%2FvIYhi2Sl4%3D&reserved=0
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
