Discussion:
[Linuxptp-devel] [PATCH RFC 0/3] Poor man's boundary clock
Richard Cochran
2014-11-01 19:00:21 UTC
Permalink
Before releasing v1.5, I wanted to test phc2sys's new automatic mode
in order to run a BC using a bunch of PCIe cards, but I found it did
not quite work. First of all, the port logic bails out when the PHC
device don't match, and secondly phc2sys has one line bug.

After hacking those two problems away, the BC worked okay. With a
GM-BC-slave setup, the slave had about an additional 9 usec offset.

I made the "jbod" a non-default option, so that users who want this
really know not to expect perfect performance.

After this series and the SO_SELECT_ERR_QUEUE get settled, I will
release the 1.5 version.

Feedback is most welcome.

Thanks,
Richard


Richard Cochran (3):
config: add a option to enable a poor man's boundary clock.
port: allow running a boundary clock with multiple clock devices.
phc2sys: make automatic mode actually work.

config.c | 13 +++++++++++++
config.h | 1 +
default.cfg | 1 +
ds.h | 1 +
gPTP.cfg | 1 +
phc2sys.c | 1 -
port.c | 6 +++++-
ptp4l.8 | 12 +++++++++++-
ptp4l.c | 1 +
9 files changed, 34 insertions(+), 3 deletions(-)
--
1.7.10.4


------------------------------------------------------------------------------
Richard Cochran
2014-11-01 19:00:22 UTC
Permalink
This patch adds a configuration option that allows running a boundary clock
using "just a bunch of devices". Normally each port is probed to make sure
they all share the same PTP hardware clock, but this option will allow a
heterogeneous collection of devices, should the user really want it.

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 13 +++++++++++++
config.h | 1 +
default.cfg | 1 +
ds.h | 1 +
gPTP.cfg | 1 +
ptp4l.8 | 12 +++++++++++-
ptp4l.c | 1 +
7 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index d5fa378..154c191 100644
--- a/config.c
+++ b/config.c
@@ -205,6 +205,12 @@ static enum parser_result parse_port_setting(const char *option,
return r;
iface->delay_filter_length = val;

+ } else if (!strcmp(option, "boundary_clock_jbod")) {
+ r = get_ranged_int(value, &val, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ iface->boundary_clock_jbod = val;
+
} else
return NOT_PARSED;

@@ -562,6 +568,12 @@ static enum parser_result parse_global_setting(const char *option,
return r;
cfg->dds.delay_filter_length = val;

+ } else if (!strcmp(option, "boundary_clock_jbod")) {
+ r = get_ranged_int(value, &val, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ cfg->dds.boundary_clock_jbod = val;
+
} else
return NOT_PARSED;

@@ -750,6 +762,7 @@ void config_init_interface(struct interface *iface, struct config *cfg)

iface->delay_filter = cfg->dds.delay_filter;
iface->delay_filter_length = cfg->dds.delay_filter_length;
+ iface->boundary_clock_jbod = cfg->dds.boundary_clock_jbod;
}

void config_destroy(struct config *cfg)
diff --git a/config.h b/config.h
index d580496..c870e42 100644
--- a/config.h
+++ b/config.h
@@ -41,6 +41,7 @@ struct interface {
struct sk_ts_info ts_info;
enum filter_type delay_filter;
int delay_filter_length;
+ int boundary_clock_jbod;
};

#define CFG_IGNORE_DM (1 << 0)
diff --git a/default.cfg b/default.cfg
index 9e794ba..4e3ea65 100644
--- a/default.cfg
+++ b/default.cfg
@@ -70,6 +70,7 @@ delay_mechanism E2E
time_stamping hardware
delay_filter moving_median
delay_filter_length 10
+boundary_clock_jbod 0
#
# Clock description
#
diff --git a/ds.h b/ds.h
index ea25fbb..496b120 100644
--- a/ds.h
+++ b/ds.h
@@ -61,6 +61,7 @@ struct default_ds {
struct clock_description clock_desc;
enum filter_type delay_filter;
int delay_filter_length;
+ int boundary_clock_jbod;
};

struct dataset {
diff --git a/gPTP.cfg b/gPTP.cfg
index e15a05a..824b854 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -69,3 +69,4 @@ delay_mechanism P2P
time_stamping hardware
delay_filter moving_median
delay_filter_length 10
+boundary_clock_jbod 0
diff --git a/ptp4l.8 b/ptp4l.8
index 687beb6..6a9e331 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -1,4 +1,4 @@
-.TH PTP4l 8 "October 2013" "linuxptp"
+.TH PTP4l 8 "November 2014" "linuxptp"
.SH NAME
ptp4l \- PTP Boundary/Ordinary Clock

@@ -205,6 +205,16 @@ The default is moving_median.
.B delay_filter_length
The length of the delay filter in samples.
The default is 10.
+.TP
+.B boundary_clock_jbod
+When running as a boundary clock (that is, when more than one network
+interface is configured), ptp4l performs a sanity check to make sure
+that all of the ports share the same hardware clock device. This
+option allows ptp4l to work as a boundary clock using "just a bunch of
+devices" that are not synchronized to each other. For this mode, the
+collection of clocks must be synchronized by an external program, for
+example phc2sys(8) in "automatic" mode.
+The default is 0 (disabled).

.SH PROGRAM AND CLOCK OPTIONS

diff --git a/ptp4l.c b/ptp4l.c
index 83824f7..dadc4da 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -75,6 +75,7 @@ static struct config cfg_settings = {
},
.delay_filter = FILTER_MOVING_MEDIAN,
.delay_filter_length = 10,
+ .boundary_clock_jbod = 0,
},

.pod = {
--
1.7.10.4


------------------------------------------------------------------------------
Richard Cochran
2014-11-01 19:00:23 UTC
Permalink
If the user has configured the appropriate option, then simply warn about
the clock device mismatch, and then go on.

Signed-off-by: Richard Cochran <***@gmail.com>
---
port.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/port.c b/port.c
index f39ad1e..05d94f5 100644
--- a/port.c
+++ b/port.c
@@ -2462,7 +2462,11 @@ struct port *port_open(int phc_index,
pr_err("port %d: PHC device mismatch", number);
pr_err("port %d: /dev/ptp%d requested, but /dev/ptp%d attached",
number, phc_index, interface->ts_info.phc_index);
- goto err_port;
+ if (interface->boundary_clock_jbod) {
+ pr_warning("port %d: just a bunch of devices", number);
+ } else {
+ goto err_port;
+ }
}

p->pod = interface->pod;
--
1.7.10.4


------------------------------------------------------------------------------
Richard Cochran
2014-11-01 19:00:24 UTC
Permalink
The reconfigure function unnecessarily clears the 'new_state' variable.
On the second and subsequent calls, this zero value incorrectly clobbers
the state of any port whose state has *not* changed.

If, for example, a port in the MASTER state goes FAULTY, when it returns
to MASTER state, phc2sys will not synchronize it, because the SLAVE port's
state variable will have been cleared to zero, effectively erasing the
local time source.

This patch fixes the issue by removing the assignment.

Signed-off-by: Richard Cochran <***@gmail.com>
---
phc2sys.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/phc2sys.c b/phc2sys.c
index 22eb9c9..47ee3b8 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -309,7 +309,6 @@ static void reconfigure(struct node *node)
clock_reinit(c);

c->state = c->new_state;
- c->new_state = 0;

if (c->state == PS_SLAVE) {
src = c;
--
1.7.10.4


------------------------------------------------------------------------------
Richard Cochran
2014-11-01 19:35:15 UTC
Permalink
Post by Richard Cochran
After hacking those two problems away, the BC worked okay. With a
GM-BC-slave setup, the slave had about an additional 9 usec offset.
Hm, not too sure about that figure. I will make a more careful
measurement and report that later.

In any case, the multi device BC with auto-phc2sys is working
reasonably well.

Thanks,
Richard

------------------------------------------------------------------------------
Keller, Jacob E
2014-11-03 19:19:28 UTC
Permalink
Post by Richard Cochran
Before releasing v1.5, I wanted to test phc2sys's new automatic mode
in order to run a BC using a bunch of PCIe cards, but I found it did
not quite work. First of all, the port logic bails out when the PHC
device don't match, and secondly phc2sys has one line bug.
After hacking those two problems away, the BC worked okay. With a
GM-BC-slave setup, the slave had about an additional 9 usec offset.
I made the "jbod" a non-default option, so that users who want this
really know not to expect perfect performance.
After this series and the SO_SELECT_ERR_QUEUE get settled, I will
release the 1.5 version.
Feedback is most welcome.
Thanks,
Richard
This looks great. Hopefully in a few days I will be able to set this up
and see how it works with one of the Intel NICs.

Thanks,
Jake
Post by Richard Cochran
config: add a option to enable a poor man's boundary clock.
port: allow running a boundary clock with multiple clock devices.
phc2sys: make automatic mode actually work.
config.c | 13 +++++++++++++
config.h | 1 +
default.cfg | 1 +
ds.h | 1 +
gPTP.cfg | 1 +
phc2sys.c | 1 -
port.c | 6 +++++-
ptp4l.8 | 12 +++++++++++-
ptp4l.c | 1 +
9 files changed, 34 insertions(+), 3 deletions(-)
------------------------------------------------------------------------------
Miroslav Lichvar
2014-11-05 14:02:14 UTC
Permalink
Post by Richard Cochran
Before releasing v1.5, I wanted to test phc2sys's new automatic mode
in order to run a BC using a bunch of PCIe cards, but I found it did
not quite work. First of all, the port logic bails out when the PHC
device don't match, and secondly phc2sys has one line bug.
After hacking those two problems away, the BC worked okay. With a
GM-BC-slave setup, the slave had about an additional 9 usec offset.
I made the "jbod" a non-default option, so that users who want this
really know not to expect perfect performance.
Hm, I think this will work only when the first port is the slave.
Otherwise, the servo will be trying to steer the clock of the master
port from slave's timestamps and the slave clock will never be
synchronized.

The PHC device would need to be reopened when a new port becomes
slave. Or there could be a check that allows only the first port to be
slave, I think this would still be a very useful feature.

There is also the problem that the master port will be sending sync
messages before it's actually synchronized to the slave port by
phc2sys. Maybe a new management message could be introduced to mark
the port as synchronized from phc2sys and allow ptp4l to switch it to
the master state.

But I'm wondering how difficult it would be to implement a BC from
separate PHCs more cleanly. How about adding a new virtual transport
that uses clock_gettime() to timestamp the messages, create a clock
instance in ptp4l for each PHC with a port to the virtual transport?
Instead of one BC handling multiple PHCs there are multiple BCs in a
virtual network.

What do you think?
--
Miroslav Lichvar

------------------------------------------------------------------------------
Richard Cochran
2014-11-05 16:24:14 UTC
Permalink
Post by Miroslav Lichvar
The PHC device would need to be reopened when a new port becomes
slave.
I assumed the code did this. Oh well. That needs to be fixed.
Post by Miroslav Lichvar
There is also the problem that the master port will be sending sync
messages before it's actually synchronized to the slave port by
phc2sys. Maybe a new management message could be introduced to mark
the port as synchronized from phc2sys and allow ptp4l to switch it to
the master state.
Really once a port goes SLAVE, then all the other ports should be
synched, regardless of state. I had coded this up, but I didn't get so
far as to posting a patch (below).

Thanks,
Richard
---
diff --git a/phc2sys.c b/phc2sys.c
index 47ee3b8..cf47990 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -310,14 +310,23 @@ static void reconfigure(struct node *node)

c->state = c->new_state;

- if (c->state == PS_SLAVE) {
- src = c;
- src_cnt++;
- } else if (c->state == PS_UNCALIBRATED) {
- src_cnt++;
- } else if (c->state == PS_MASTER) {
+ switch (c->state) {
+ case PS_FAULTY:
+ case PS_DISABLED:
+ case PS_LISTENING:
+ case PS_PRE_MASTER:
+ case PS_MASTER:
+ case PS_PASSIVE:
pr_info("selecting %s for synchronization", c->device);
dst_cnt++;
+ break;
+ case PS_UNCALIBRATED:
+ src_cnt++;
+ break;
+ case PS_SLAVE:
+ src = c;
+ src_cnt++;
+ break;
}
}
if (src_cnt > 1) {
@@ -559,6 +568,23 @@ static int do_pps_loop(struct node *node, struct clock *clock, int fd)
return 0;
}

+static int update_needed(struct clock *c)
+{
+ switch (c->state) {
+ case PS_FAULTY:
+ case PS_DISABLED:
+ case PS_LISTENING:
+ case PS_PRE_MASTER:
+ case PS_MASTER:
+ case PS_PASSIVE:
+ return 1;
+ case PS_UNCALIBRATED:
+ case PS_SLAVE:
+ break;
+ }
+ return 0;
+}
+
static int do_loop(struct node *node, int subscriptions)
{
struct timespec interval;
@@ -590,7 +616,7 @@ static int do_loop(struct node *node, int subscriptions)
continue;

LIST_FOREACH(clock, &node->clocks, list) {
- if (clock->state != PS_MASTER)
+ if (!update_needed(clock))
continue;

if (clock->clkid == CLOCK_REALTIME &&


------------------------------------------------------------------------------
Richard Cochran
2014-11-05 16:34:23 UTC
Permalink
Post by Miroslav Lichvar
But I'm wondering how difficult it would be to implement a BC from
separate PHCs more cleanly. How about adding a new virtual transport
that uses clock_gettime() to timestamp the messages, create a clock
instance in ptp4l for each PHC with a port to the virtual transport?
Instead of one BC handling multiple PHCs there are multiple BCs in a
virtual network.
What do you think?
This similar to what Patrick Ohly (inventor of so_timestamping)
suggested in his paper, with "two-layer ptp" or something like that.

It would add a bunch of protocol overhead, messages back and forth,
and so on. Also it would change the BMC network topology. This impacts
the hops for managment message forwarding.

I like the idea of "automatic" phc2sys, if it would only work right.

Thanks,
Richard



------------------------------------------------------------------------------
Jiri Benc
2014-11-05 16:56:28 UTC
Permalink
Post by Richard Cochran
I like the idea of "automatic" phc2sys, if it would only work right.
It's definitely not complete yet and I'm not surprised there are bugs
(although I did my best to support also future cases, it's hard to get
it right without the actual code to test it).

My plan for the next steps has been allowing ptp4l to work with multiple
independent PHCs that would form a PTP clock (and rely on phc2sys to
sync those PHCs). This needs separation of struct clock into two
structures, as some fields are per-PTP clock and some are per-PHC.
I have a patch that does this but it needs to be rebased.

I think this is a prerequisite to having a boundary clock that uses
multiple PHCs, I cannot imagine how it could work without this
separation.

What is not solved is the problem that Miroslav pointed out (and also
pointed out to me earlier): the sync messages cannot be sent until the
clock is really synced. This will need further communication between
ptp4l and phc2sys.

I still intend to work on this but have less time to work on linuxptp
than before.

Jiri
--
Jiri Benc

------------------------------------------------------------------------------
Richard Cochran
2014-11-05 18:58:32 UTC
Permalink
Post by Jiri Benc
My plan for the next steps has been allowing ptp4l to work with multiple
independent PHCs that would form a PTP clock (and rely on phc2sys to
sync those PHCs).
Doesn't my patch #2 do this?
Post by Jiri Benc
This needs separation of struct clock into two
structures, as some fields are per-PTP clock and some are per-PHC.
I have a patch that does this but it needs to be rebased.
You mean, the phc2sys 'struct clock', I guess?

Please post the patch, so we see what you are talking about.
Post by Jiri Benc
I think this is a prerequisite to having a boundary clock that uses
multiple PHCs, I cannot imagine how it could work without this
separation.
So, in the current form, what does the 'automatic' mode (I mean, -a
without -r) provide? Nothing useful? We can't do a release with half
baked stuff like that.


Thanks,
Richard

------------------------------------------------------------------------------
Jiri Benc
2014-11-05 19:28:04 UTC
Permalink
Post by Richard Cochran
Post by Jiri Benc
My plan for the next steps has been allowing ptp4l to work with
multiple independent PHCs that would form a PTP clock (and rely on
phc2sys to sync those PHCs).
Doesn't my patch #2 do this?
It's surely the intention. I just don't know how it works with things
like servo state shared across all of the ports.
Post by Richard Cochran
Post by Jiri Benc
This needs separation of struct clock into two
structures, as some fields are per-PTP clock and some are per-PHC.
I have a patch that does this but it needs to be rebased.
You mean, the phc2sys 'struct clock', I guess?
I really mean ptp4l.
Post by Richard Cochran
Please post the patch, so we see what you are talking about.
See below. It's very outdated, does not apply and is untested. I also
don't like the structure name (although wasn't able to come up with
anything better). It depends on a few more patches but those basically
do some code reorganization only and should not be crucial to
understanding of the intention.

I'm not sure whether this patch was enough to support the boundary
clock or more was needed but I remember I had the boundary clock stuff
done (though untested) and I cannot find anything on top of this, so
this was probably enough.
Post by Richard Cochran
Post by Jiri Benc
I think this is a prerequisite to having a boundary clock that uses
multiple PHCs, I cannot imagine how it could work without this
separation.
So, in the current form, what does the 'automatic' mode (I mean, -a
without -r) provide? Nothing useful? We can't do a release with half
baked stuff like that.
-a without -r can indeed be considered as not much useful with the
current code. I intended to send the rebased support for boundary
clock shortly afterwards but as the review was much slower than
I anticipated, I ran out of time allocated for this.

I wouldn't call that half baked but if you want to have the boundary
clock support before a new release, I can try to get some time to work
on this. If it's really needed--maybe your patchset works okay, I admit
I'm not 100% sure, I would have to spend some time looking into the
code, my original patchset is obsolete, the patches it depended on were
redesigned heavily.

Thanks,

Jiri


commit abf3fde8ff5878b3aeab6ecafeae951ca0d6b876
Author: Jiri Benc <***@redhat.com>
Date: Wed Dec 4 16:35:22 2013 +0100

Support for multiple physical clocks

Add support for multiple physical clocks. The "clock" term refers to the PTP
node (as defined in IEEE 1588), the "physical clock" term refers to the
piece of hardware that keeps time.

With this patch, ptp4l still represents a single clock (= single PTP node).
The ports it handles can have separate physical clocks attached, though.
Obviously, such physical clocks have to be synchronized. For now, it is the
responsiblity of the amdinistrator to set up phc2sys to synchronize the
physical clocks in the correct direction. The ultimate plan is to modify
phc2sys to do this automatically.

Signed-off-by: Jiri Benc <***@redhat.com>

diff --git a/clock.c b/clock.c
index 1bdf18445b13..e70d58f9eb84 100644
--- a/clock.c
+++ b/clock.c
@@ -57,9 +57,19 @@ struct clock_stats {
unsigned int max_count;
};

-struct clock {
+struct phys_clock {
+ LIST_ENTRY(phys_clock) list;
+ struct clock *clock;
clockid_t clkid;
struct servo *servo;
+ enum servo_state servo_state;
+ struct clockcheck *sanity_check;
+ int nports;
+ int phc_index;
+};
+
+struct clock {
+ LIST_HEAD(phys_head, phys_clock) phys;
struct defaultDS dds;
struct dataset default_dataset;
struct currentDS cur;
@@ -79,10 +89,11 @@ struct clock {
int utc_timescale;
int leap_set;
int kernel_leap;
+ int sanity_freq_limit;
+ enum servo_type default_servo;
int utc_offset; /* grand master role */
int time_flags; /* grand master role */
int time_source; /* grand master role */
- enum servo_state servo_state;
tmv_t master_offset;
tmv_t path_delay;
struct filter *delay_filter;
@@ -96,7 +107,6 @@ struct clock {
struct clock_description desc;
struct clock_stats stats;
int stats_interval;
- struct clockcheck *sanity_check;
};

struct clock the_clock;
@@ -126,10 +136,6 @@ void clock_destroy(struct clock *c)
}
port_close(c->uds_port);
free(c->pollfd);
- if (c->clkid != CLOCK_REALTIME) {
- phc_close(c->clkid);
- }
- servo_destroy(c->servo);
filter_destroy(c->delay_filter);
stats_destroy(c->stats.offset);
stats_destroy(c->stats.freq);
@@ -488,8 +494,9 @@ static void clock_update_slave(struct clock *c)
}
}

-static int clock_utc_correct(struct clock *c, tmv_t ingress)
+static int clock_utc_correct(struct phys_clock *pc, tmv_t ingress)
{
+ struct clock *c = pc->clock;
struct timespec offset;
int utc_offset, leap, clock_leap;
uint64_t ts;
@@ -514,10 +521,10 @@ static int clock_utc_correct(struct clock *c, tmv_t ingress)
}

/* Handle leap seconds. */
- if ((leap || c->leap_set) && c->clkid == CLOCK_REALTIME) {
+ if ((leap || c->leap_set) && pc->clkid == CLOCK_REALTIME) {
/* If the clock will be stepped, the time stamp has to be the
target time. Ignore possible 1 second error in utc_offset. */
- if (c->servo_state == SERVO_UNLOCKED) {
+ if (pc->servo_state == SERVO_UNLOCKED) {
ts = tmv_to_nanoseconds(tmv_sub(ingress,
c->master_offset));
if (c->tds.flags & PTP_TIMESCALE)
@@ -577,6 +584,144 @@ UInteger8 clock_class(struct clock *c)
return c->dds.clockQuality.clockClass;
}

+static struct phys_clock *clock_phys_new(struct clock *c,
+ struct interface *iface, int sw_ts)
+{
+ int fadj = 0, max_adj = 0;
+ char phc[32];
+ struct phys_clock *pc;
+
+ /* First, check for compatibility with already used clocks and
+ * configuration mismatch. */
+ if (iface->requested_phc >= 0 &&
+ iface->requested_phc != iface->ts_info.phc_index) {
+ pr_err("%s: PHC device mismatch: port %d: /dev/ptp%d requested, but /dev/ptp%d attached",
+ iface->name, iface->requested_phc,
+ iface->ts_info.phc_index);
+ return NULL;
+ }
+ if (c->utc_timescale && iface->requested_phc >= 0) {
+ pr_err("PTP Hardware Clock cannot be used with software or legacy time stamping");
+ return NULL;
+ }
+ if (!LIST_EMPTY(&c->phys)) {
+ pc = LIST_FIRST(&c->phys);
+ if ((pc->phc_index < 0 && pc->phc_index != iface->requested_phc) ||
+ (pc->phc_index >= 0 && iface->requested_phc < 0)) {
+ pr_err("Ports with incompatible clocks specified");
+ return NULL;
+ }
+ }
+
+ pc = calloc(1, sizeof(struct phys_clock));
+ if (!pc) {
+ pr_err("Failed to allocate memory for a clock");
+ return NULL;
+ }
+ pc->clock = c;
+ pc->phc_index = iface->requested_phc;
+ pc->nports = 0;
+
+ if (iface->requested_phc == PHC_NONE) {
+ pc->clkid = CLOCK_INVALID;
+ } else if (iface->requested_phc == PHC_REALTIME) {
+ pc->clkid = CLOCK_REALTIME;
+ max_adj = sysclk_max_freq();
+ sysclk_set_leap(0);
+ } else {
+ snprintf(phc, 31, "/dev/ptp%d", iface->requested_phc);
+ pc->clkid = phc_open(phc);
+ if (pc->clkid == CLOCK_INVALID) {
+ pr_err("Failed to open %s: %m", phc);
+ goto err_free;
+ }
+ max_adj = phc_max_adj(pc->clkid);
+ if (!max_adj) {
+ pr_err("clock is not adjustable");
+ goto err_close;
+ }
+ pr_info("%s: selected /dev/ptp%d as PTP clock", iface->name,
+ iface->requested_phc);
+ }
+
+ if (pc->clkid != CLOCK_INVALID) {
+ fadj = (int) clockadj_get_freq(pc->clkid);
+ /* Due to a bug in older kernels, the reading may silently fail
+ and return 0. Set the frequency back to make sure fadj is
+ the actual frequency of the clock. */
+ clockadj_set_freq(pc->clkid, fadj);
+ }
+ pc->servo = servo_create(c->default_servo, -fadj, max_adj, sw_ts);
+ if (!pc->servo) {
+ pr_err("Failed to create clock servo");
+ goto err_close;
+ }
+ pc->servo_state = SERVO_UNLOCKED;
+
+ if (c->sanity_freq_limit) {
+ pc->sanity_check = clockcheck_create(c->sanity_freq_limit);
+ if (!pc->sanity_check) {
+ pr_err("Failed to create clock sanity check");
+ goto err_servo;
+ }
+ }
+
+ LIST_INSERT_HEAD(&c->phys, pc, list);
+ pc->nports = 1;
+ return pc;
+
+err_servo:
+ servo_destroy(pc->servo);
+err_close:
+ if (pc->clkid != CLOCK_INVALID && pc->clkid != CLOCK_REALTIME)
+ phc_close(pc->clkid);
+err_free:
+ free(pc);
+ return NULL;
+}
+
+static void clock_phys_destroy(struct phys_clock *pc)
+{
+ LIST_REMOVE(pc, list);
+ clockcheck_destroy(pc->sanity_check);
+ servo_destroy(pc->servo);
+ if (pc->clkid != CLOCK_INVALID && pc->clkid != CLOCK_REALTIME)
+ phc_close(pc->clkid);
+ free(pc);
+}
+
+struct phys_clock *clock_phys_get(struct clock *c, struct interface *iface,
+ enum timestamp_type timestamping)
+{
+ struct phys_clock *pc;
+
+ if (iface->requested_phc == PHC_AUTO) {
+ if (!iface->ts_info.valid) {
+ pr_err("ptp device not specified and automatic determination is not "
+ "supported. Please specify ptp device");
+ return NULL;
+ }
+ iface->requested_phc = iface->ts_info.phc_index;
+ }
+
+ /* Look whether we have this clock already */
+ LIST_FOREACH(pc, &c->phys, list) {
+ if (pc->phc_index == iface->requested_phc) {
+ pc->nports++;
+ return pc;
+ }
+ }
+
+ return clock_phys_new(c, iface, timestamping == TS_SOFTWARE);
+}
+
+void clock_phys_put(struct phys_clock *pc)
+{
+ pc->nports--;
+ if (!pc->nports)
+ clock_phys_destroy(pc);
+}
+
static struct port *clock_add_port(struct clock *c,
enum timestamp_type timestamping,
struct interface *iface)
@@ -609,11 +754,8 @@ static void clock_remove_port(struct clock *c, struct port *p)

struct clock *clock_create(const struct config *cfg)
{
- int fadj = 0, max_adj = 0.0, sw_ts = cfg->timestamping == TS_SOFTWARE;
- int phc_index;
struct clock *c = &the_clock;
struct port *p;
- char phc[32];
struct interface udsif, *iface;

memset(&udsif, 0, sizeof(udsif));
@@ -629,59 +771,18 @@ struct clock *clock_create(const struct config *cfg)
c->free_running = cfg->dds.free_running;
c->freq_est_interval = cfg->dds.freq_est_interval;
c->kernel_leap = cfg->dds.kernel_leap;
+ c->sanity_freq_limit = cfg->dds.sanity_freq_limit;
c->utc_offset = CURRENT_UTC_OFFSET;
c->time_source = cfg->dds.time_source;
+ c->default_servo = cfg->clock_servo;
c->desc = cfg->dds.clock_desc;

if (cfg->timestamping == TS_SOFTWARE || cfg->timestamping == TS_LEGACY_HW)
c->utc_timescale = 1;

- iface = STAILQ_FIRST(&cfg->interfaces);
- phc_index = iface->requested_phc;
- if (phc_index == PHC_NONE) {
- c->clkid = CLOCK_INVALID;
- } else if (phc_index == PHC_REALTIME) {
- c->clkid = CLOCK_REALTIME;
- max_adj = sysclk_max_freq();
- sysclk_set_leap(0);
- } else if (phc_index == PHC_AUTO) {
- if (!iface->ts_info.valid) {
- pr_err("ptp device not specified and automatic determination is not "
- "supported. Please specify ptp device");
- return NULL;
- }
- phc_index = iface->ts_info.phc_index;
- }
- if (phc_index >= 0) {
- snprintf(phc, 31, "/dev/ptp%d", phc_index);
- c->clkid = phc_open(phc);
- if (c->clkid == CLOCK_INVALID) {
- pr_err("Failed to open %s: %m", phc);
- return NULL;
- }
- max_adj = phc_max_adj(c->clkid);
- if (!max_adj) {
- pr_err("clock is not adjustable");
- return NULL;
- }
- pr_info("selected /dev/ptp%d as PTP clock", phc_index);
- }
c->leap_set = 0;
c->time_flags = c->utc_timescale ? 0 : PTP_TIMESCALE;

- if (c->clkid != CLOCK_INVALID) {
- fadj = (int) clockadj_get_freq(c->clkid);
- /* Due to a bug in older kernels, the reading may silently fail
- and return 0. Set the frequency back to make sure fadj is
- the actual frequency of the clock. */
- clockadj_set_freq(c->clkid, fadj);
- }
- c->servo = servo_create(cfg->clock_servo, -fadj, max_adj, sw_ts);
- if (!c->servo) {
- pr_err("Failed to create clock servo");
- return NULL;
- }
- c->servo_state = SERVO_UNLOCKED;
c->delay_filter = filter_create(cfg->dds.delay_filter,
cfg->dds.delay_filter_length);
if (!c->delay_filter) {
@@ -696,13 +797,6 @@ struct clock *clock_create(const struct config *cfg)
pr_err("failed to create stats");
return NULL;
}
- if (cfg->dds.sanity_freq_limit) {
- c->sanity_check = clockcheck_create(cfg->dds.sanity_freq_limit);
- if (!c->sanity_check) {
- pr_err("Failed to create clock sanity check");
- return NULL;
- }
- }

c->dds = cfg->dds.dds;

@@ -713,8 +807,7 @@ struct clock *clock_create(const struct config *cfg)
c->dad.pds.observedParentClockPhaseChangeRate = 0x7fffffff;
c->dad.ptl = c->ptl;

- clock_sync_interval(c, 0);
-
+ LIST_INIT(&c->phys);
LIST_INIT(&c->ports);
c->last_port_number = 0;

@@ -734,8 +827,8 @@ struct clock *clock_create(const struct config *cfg)

/* Create the ports. */
STAILQ_FOREACH(iface, &cfg->interfaces, list) {
- iface->requested_phc = phc_index;
- if (!clock_add_port(c, cfg->timestamping, iface)) {
+ p = clock_add_port(c, cfg->timestamping, iface);
+ if (!p) {
pr_err("failed to open port %s", iface->name);
return NULL;
}
@@ -1117,7 +1210,7 @@ UInteger16 clock_steps_removed(struct clock *c)
return c->cur.stepsRemoved;
}

-enum servo_state clock_synchronize(struct clock *c,
+enum servo_state clock_synchronize(struct phys_clock *pc,
struct timespec ingress_ts,
struct timestamp origin_ts,
Integer64 correction1,
@@ -1126,6 +1219,7 @@ enum servo_state clock_synchronize(struct clock *c,
double adj;
tmv_t ingress, origin;
enum servo_state state = SERVO_UNLOCKED;
+ struct clock *c = pc->clock;

ingress = timespec_to_tmv(ingress_ts);
origin = timestamp_to_tmv(origin_ts);
@@ -1145,17 +1239,17 @@ enum servo_state clock_synchronize(struct clock *c,
if (!c->path_delay)
return state;

- if (clock_utc_correct(c, ingress))
- return c->servo_state;
+ if (clock_utc_correct(pc, ingress))
+ return pc->servo_state;

c->cur.offsetFromMaster = tmv_to_TimeInterval(c->master_offset);

if (c->free_running)
return clock_no_adjust(c);

- adj = servo_sample(c->servo, tmv_to_nanoseconds(c->master_offset),
+ adj = servo_sample(pc->servo, tmv_to_nanoseconds(c->master_offset),
tmv_to_nanoseconds(ingress), &state);
- c->servo_state = state;
+ pc->servo_state = state;

if (c->stats.max_count > 1) {
clock_stats_update(&c->stats,
@@ -1171,29 +1265,30 @@ enum servo_state clock_synchronize(struct clock *c,
case SERVO_UNLOCKED:
break;
case SERVO_JUMP:
- clockadj_set_freq(c->clkid, -adj);
- clockadj_step(c->clkid, -tmv_to_nanoseconds(c->master_offset));
+ clockadj_set_freq(pc->clkid, -adj);
+ clockadj_step(pc->clkid, -tmv_to_nanoseconds(c->master_offset));
c->t1 = tmv_zero();
c->t2 = tmv_zero();
- if (c->sanity_check) {
- clockcheck_set_freq(c->sanity_check, -adj);
- clockcheck_step(c->sanity_check,
+ if (pc->sanity_check) {
+ clockcheck_set_freq(pc->sanity_check, -adj);
+ clockcheck_step(pc->sanity_check,
-tmv_to_nanoseconds(c->master_offset));
}
break;
case SERVO_LOCKED:
- clockadj_set_freq(c->clkid, -adj);
- if (c->clkid == CLOCK_REALTIME)
+ clockadj_set_freq(pc->clkid, -adj);
+ if (pc->clkid == CLOCK_REALTIME)
sysclk_set_sync();
- if (c->sanity_check)
- clockcheck_set_freq(c->sanity_check, -adj);
+ if (pc->sanity_check)
+ clockcheck_set_freq(pc->sanity_check, -adj);
break;
}
return state;
}

-void clock_sync_interval(struct clock *c, int n)
+void clock_sync_interval(struct phys_clock *pc, int n)
{
+ struct clock *c = pc->clock;
int shift;

shift = c->freq_est_interval - n;
@@ -1214,7 +1309,7 @@ void clock_sync_interval(struct clock *c, int n)
}
c->stats.max_count = (1 << shift);

- servo_sync_interval(c->servo, n < 0 ? 1.0 / (1 << -n) : 1 << n);
+ servo_sync_interval(pc->servo, n < 0 ? 1.0 / (1 << -n) : 1 << n);
}

struct timePropertiesDS *clock_time_properties(struct clock *c)
@@ -1304,11 +1399,11 @@ int clock_num_ports(struct clock *c)
return c->nports;
}

-void clock_check_ts(struct clock *c, struct timespec ts)
+void clock_check_ts(struct phys_clock *pc, struct timespec ts)
{
- if (c->sanity_check &&
- clockcheck_sample(c->sanity_check,
+ if (pc->sanity_check &&
+ clockcheck_sample(pc->sanity_check,
ts.tv_sec * NS_PER_SEC + ts.tv_nsec)) {
- servo_reset(c->servo);
+ servo_reset(pc->servo);
}
}
diff --git a/clock.h b/clock.h
index 3bc3d0c75b79..a0b5c6d67166 100644
--- a/clock.h
+++ b/clock.h
@@ -37,6 +37,7 @@ struct ptp_message; /*forward declaration*/

/** Opaque type. */
struct clock;
+struct phys_clock;

/**
* Obtains a reference to the best foreign master of a clock.
@@ -177,7 +178,7 @@ UInteger16 clock_steps_removed(struct clock *c);

/**
* Provide a data point to synchronize the clock.
- * @param c The clock instance to synchronize.
+ * @param pc The physical clock instance to synchronize.
* @param ingress_ts The ingress time stamp on the sync message.
* @param origin_ts The reported transmission time of the sync message.
* @param correction1 The correction field of the sync message.
@@ -185,7 +186,7 @@ UInteger16 clock_steps_removed(struct clock *c);
* Pass zero in the case of one step operation.
* @return The state of the clock's servo.
*/
-enum servo_state clock_synchronize(struct clock *c,
+enum servo_state clock_synchronize(struct phys_clock *pc,
struct timespec ingress_ts,
struct timestamp origin_ts,
Integer64 correction1,
@@ -193,10 +194,10 @@ enum servo_state clock_synchronize(struct clock *c,

/**
* Inform a slaved clock about the master's sync interval.
- * @param c The clock instance.
+ * @param pc The physical clock instance.
* @param n The logarithm base two of the sync interval.
*/
-void clock_sync_interval(struct clock *c, int n);
+void clock_sync_interval(struct phys_clock *pc, int n);

/**
* Obtain a clock's time properties data set.
@@ -228,9 +229,27 @@ int clock_num_ports(struct clock *c);

/**
* Perform a sanity check on a time stamp made by a clock.
- * @param c The clock instance.
+ * @param pc The physical clock instance.
* @param ts The time stamp.
*/
-void clock_check_ts(struct clock *c, struct timespec ts);
+void clock_check_ts(struct phys_clock *c, struct timespec ts);
+
+/**
+ * Gets a reference to physical clock belonging to the given interface,
+ * allocating new entry if necessary.
+ * @param c The clock instance
+ * @param iface Network interface configuration
+ * @param timestamping Requested time stamping mode
+ * @return Physical clock entry with reference count increased
+ */
+struct phys_clock *clock_phys_get(struct clock *c, struct interface *iface,
+ enum timestamp_type timestamping);
+
+/**
+ * Puts the reference obtained by clock_phys_get, freeing the entry if
+ * appropriate.
+ * @param pc Entry obtained by clock_phys_get
+ */
+void clock_phys_put(struct phys_clock *pc);

#endif
diff --git a/port.c b/port.c
index 63f513ea31a0..222e878343b6 100644
--- a/port.c
+++ b/port.c
@@ -65,6 +65,7 @@ struct port_data {
struct port port;
char *name;
struct clock *clock;
+ struct phys_clock *phys_clock;
struct transport *trp;
enum timestamp_type timestamping;
struct fdarray fda;
@@ -894,7 +895,7 @@ static void port_synchronize(struct port_data *p,

port_set_sync_rx_tmo(p);

- state = clock_synchronize(p->clock, ingress_ts, origin_ts,
+ state = clock_synchronize(p->phys_clock, ingress_ts, origin_ts,
correction1, correction2);
switch (state) {
case SERVO_UNLOCKED:
@@ -1866,7 +1867,7 @@ static void process_sync(struct port_data *p, struct ptp_message *m)

if (m->header.logMessageInterval != p->log_sync_interval) {
p->log_sync_interval = m->header.logMessageInterval;
- clock_sync_interval(p->clock, p->log_sync_interval);
+ clock_sync_interval(p->phys_clock, p->log_sync_interval);
}

m->header.correction += p->pod.asymmetry;
@@ -1901,6 +1902,8 @@ void port_close(struct port *port)
filter_destroy(p->delay_filter);
if (p->fault_fd >= 0)
close(p->fault_fd);
+ if (p->phys_clock)
+ clock_phys_put(p->phys_clock);
free(p);
}

@@ -2141,7 +2144,7 @@ enum fsm_event port_event(struct port *port, int fd_index)
return EV_NONE;
}
if (msg->hwts.ts.tv_sec && msg->hwts.ts.tv_nsec) {
- clock_check_ts(p->clock, msg->hwts.ts);
+ clock_check_ts(p->phys_clock, msg->hwts.ts);
}
if (port_ignore(p, msg)) {
msg_put(msg);
@@ -2353,15 +2356,10 @@ struct port *port_open(enum timestamp_type timestamping,

if (interface->transport == TRANS_UDS)
; /* UDS cannot have a PHC. */
- else if (!interface->ts_info.valid)
- pr_warning("port %d: get_ts_info not supported", number);
- else if (interface->requested_phc >= 0 &&
- interface->requested_phc != interface->ts_info.phc_index) {
- pr_err("port %d: PHC device mismatch", number);
- pr_err("port %d: /dev/ptp%d requested, but /dev/ptp%d attached",
- number, interface->requested_phc,
- interface->ts_info.phc_index);
- goto err_port;
+ else {
+ p->phys_clock = clock_phys_get(clock, interface, timestamping);
+ if (!p->phys_clock)
+ goto err_port;
}

p->pod = interface->pod;
@@ -2369,7 +2367,7 @@ struct port *port_open(enum timestamp_type timestamping,
p->clock = clock;
p->trp = transport_create(interface->transport);
if (!p->trp)
- goto err_port;
+ goto err_pclock;
p->timestamping = timestamping;
p->portIdentity.clockIdentity = clock_identity(clock);
p->portIdentity.portNumber = number;
@@ -2394,12 +2392,17 @@ struct port *port_open(enum timestamp_type timestamping,
goto err_filter;
}
}
+ if (p->phys_clock)
+ clock_sync_interval(p->phys_clock, 0);
return &p->port;

err_filter:
filter_destroy(p->delay_filter);
err_transport:
transport_destroy(p->trp);
+err_pclock:
+ if (p->phys_clock)
+ clock_phys_put(p->phys_clock);
err_port:
free(p);
return NULL;
diff --git a/port.h b/port.h
index 6af9ebfbbb95..5c3d4a15714c 100644
--- a/port.h
+++ b/port.h
@@ -27,6 +27,7 @@

/* forward declarations */
struct interface;
+struct phys_clock;
struct clock;

/** Public part of port structure data. */

------------------------------------------------------------------------------
Richard Cochran
2014-11-05 20:40:48 UTC
Permalink
Post by Jiri Benc
I'm not sure whether this patch was enough to support the boundary
clock or more was needed but I remember I had the boundary clock stuff
done (though untested) and I cannot find anything on top of this, so
this was probably enough.
Having looked at the patch, and now I understand what you are saying,
and why ptp4l must steer the SLAVE port's clock, duh.
Post by Jiri Benc
-a without -r can indeed be considered as not much useful with the
current code. I intended to send the rebased support for boundary
clock shortly afterwards but as the review was much slower than
I anticipated, I ran out of time allocated for this.
Yeah, but now I have time to review :P
Post by Jiri Benc
I wouldn't call that half baked but if you want to have the boundary
clock support before a new release, I can try to get some time to work
on this. If it's really needed--maybe your patchset works okay, I admit
I'm not 100% sure, I would have to spend some time looking into the
code, my original patchset is obsolete, the patches it depended on were
redesigned heavily.
No, my patch isn't going to fix it.

But I think there is an easier way than what you posted. We only need
to push the clockid into the port structure and then pass it along into
clock_synchronize().

Well, I will take a closer look anyhow...

Thanks,
Richard

------------------------------------------------------------------------------
Miroslav Lichvar
2014-11-06 08:22:23 UTC
Permalink
Post by Richard Cochran
Post by Miroslav Lichvar
But I'm wondering how difficult it would be to implement a BC from
separate PHCs more cleanly. How about adding a new virtual transport
that uses clock_gettime() to timestamp the messages, create a clock
instance in ptp4l for each PHC with a port to the virtual transport?
Instead of one BC handling multiple PHCs there are multiple BCs in a
virtual network.
What do you think?
This similar to what Patrick Ohly (inventor of so_timestamping)
suggested in his paper, with "two-layer ptp" or something like that.
Looking at the paper, it seems the two level PTP is exactly what we
have now with ptp4l and phc2sys, one servo steers the NIC clock from
PTP and the second servo steers the system clock to the NIC clock.
Post by Richard Cochran
It would add a bunch of protocol overhead, messages back and forth,
and so on. Also it would change the BMC network topology. This impacts
the hops for managment message forwarding.
Yes, managing multiple clock instances in ptp4l would be more
expensive and there would be an extra hop visible on the network, but
is that really a problem? They are separate physical clocks.
Post by Richard Cochran
I like the idea of "automatic" phc2sys, if it would only work right.
I like the automatic phc2sys mode when used to synchronize the system
clock. It's used by timemaster and seems to be working nicely. I'm just
worried the tight coupling between ptp4l and phc2sys that will be
needed for the jbod mode will be problematic in the future and phc2sys
will eventually have to be included in ptp4l for some reason we don't
see know.
--
Miroslav Lichvar

------------------------------------------------------------------------------
Loading...