Re: [Linuxptp-devel] [RFC PATCH 00/15] Dynamic sync direction for ts2phc
On Mon, Aug 17, 2020 at 01:44:52PM -0700, Richard Cochran wrote: > On Mon, Aug 17, 2020 at 11:28:34PM +0300, Vladimir Oltean wrote: > > Do I need to fix up the patches now, or are you still going to review > > them? > > I'll go forward, reviewing the patches as is... > > Thanks, > Richard If you haven't started yet, I can still send a RFC v2 which also addresses some of Jacob's feedback. Thanks, -Vladimir ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 00/15] Dynamic sync direction for ts2phc
On Mon, Aug 17, 2020 at 11:28:34PM +0300, Vladimir Oltean wrote: > Do I need to fix up the patches now, or are you still going to review > them? I'll go forward, reviewing the patches as is... Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 00/15] Dynamic sync direction for ts2phc
On Mon, Aug 17, 2020 at 11:58:59AM -0700, Richard Cochran wrote:
> Vladimir,
>
> On Tue, Aug 04, 2020 at 02:40:11PM +0300, Vladimir Oltean wrote:
> > The diff is large, yes, even though I tried to avoid making unnecessary
> > changes. I hope it isn't too difficult to review, it isn't as polished
> > as I typically like, because the idea itself was uncertain, so I
> > preferred not to spend that extra time at this point in the process.
> > Anyways, I'm looking forward to your feedback whenever you have the
> > time.
>
> I was going to begin my review, but first I ran my ts2phc smoke test.
> Valgrind found a memory leak.
>
> Thanks,
> Richard
>
> sudo valgrind ./ts2phc -f ts2phc-TC.cfg --free_running=1
> ...
> Control-C...
>
> ==9363== HEAP SUMMARY:
> ==9363== in use at exit: 1,022 bytes in 10 blocks
> ==9363== total heap usage: 279 allocs, 269 frees, 20,230 bytes allocated
> ==9363==
> ==9363== LEAK SUMMARY:
> ==9363==definitely lost: 568 bytes in 4 blocks
> ==9363==indirectly lost: 454 bytes in 6 blocks
> ==9363== possibly lost: 0 bytes in 0 blocks
> ==9363==still reachable: 0 bytes in 0 blocks
> ==9363== suppressed: 0 bytes in 0 blocks
> ==9363== Rerun with --leak-check=full to see details of leaked memory
>
> --- ts2phc-TC.cfg ---
> # for use with three i210 cards
> [global]
> use_syslog 0
> verbose 1
> logging_level 6
> ts2phc.pulsewidth 5
> [eth3]
> ts2phc.channel 0
> ts2phc.master 1
> ts2phc.pin_index0
> [eth4]
> ts2phc.channel 0
> ts2phc.extts_polarity both
> ts2phc.pin_index0
> [eth6]
> ts2phc.channel 0
> ts2phc.extts_polarity both
> ts2phc.pin_index0
My bad. This is the sort of detail I haven't paid enough attention to.
FWIW, this fixes up the memory leakage for me:
diff --git a/ts2phc.c b/ts2phc.c
index 69cac305c791..cac1819d4060 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -29,6 +29,8 @@ struct interface {
static void ts2phc_cleanup(struct ts2phc_private *priv)
{
+ struct port *p, *tmp;
+
ts2phc_slave_cleanup(priv);
if (priv->master) {
ts2phc_master_destroy(priv->master);
@@ -37,6 +39,15 @@ static void ts2phc_cleanup(struct ts2phc_private *priv)
config_destroy(priv->cfg);
}
close_pmc_node(>node);
+
+ /*
+* Clocks are destroyed by the cleanup methods of the individual
+* master and slave PHC modules.
+*/
+ LIST_FOREACH_SAFE(p, >ports, list, tmp)
+ free(p);
+
+ msg_cleanup();
}
/* FIXME: Copied from phc2sys */
@@ -210,6 +221,14 @@ struct clock *clock_add(struct ts2phc_private *priv, const
char *device)
return c;
}
+void clock_destroy(struct clock *c)
+{
+ servo_destroy(c->servo);
+ posix_clock_close(c->clkid);
+ free(c->name);
+ free(c);
+}
+
/* FIXME: Copied from phc2sys */
static struct port *port_add(struct ts2phc_private *priv, unsigned int number,
char *device)
@@ -236,6 +255,7 @@ static struct port *port_add(struct ts2phc_private *priv,
unsigned int number,
p = malloc(sizeof(*p));
if (!p) {
pr_err("failed to allocate memory for a port");
+ clock_destroy(c);
return NULL;
}
p->number = number;
diff --git a/ts2phc.h b/ts2phc.h
index 9b5df9e25e38..45a5a730d722 100644
--- a/ts2phc.h
+++ b/ts2phc.h
@@ -70,6 +70,7 @@ struct ts2phc_private {
struct servo *servo_add(struct ts2phc_private *priv, struct clock *clock);
struct clock *clock_add(struct ts2phc_private *priv, const char *device);
void clock_add_tstamp(struct clock *clock, tmv_t ts);
+void clock_destroy(struct clock *clock);
#include "ts2phc_master.h"
#include "ts2phc_slave.h"
diff --git a/ts2phc_phc_master.c b/ts2phc_phc_master.c
index 3feaacfbfb39..43444e879d0a 100644
--- a/ts2phc_phc_master.c
+++ b/ts2phc_phc_master.c
@@ -87,7 +87,7 @@ static void ts2phc_phc_master_destroy(struct ts2phc_master
*master)
_request)) {
pr_err(PTP_PEROUT_REQUEST_FAILED);
}
- posix_clock_close(m->clock->clkid);
+ clock_destroy(m->clock);
free(m);
}
diff --git a/ts2phc_slave.c b/ts2phc_slave.c
index 68fe355bdca7..6bb212865626 100644
--- a/ts2phc_slave.c
+++ b/ts2phc_slave.c
@@ -108,6 +108,7 @@ static void ts2phc_slave_array_destroy(struct
ts2phc_private *priv)
free(polling_array->slave);
free(polling_array->pfd);
+ free(polling_array->collected_events);
free(polling_array);
priv->polling_array = NULL;
}
@@ -191,12 +192,6 @@ static struct ts2phc_slave *ts2phc_slave_create(struct
ts2phc_private *priv,
pr_debug("PHC slave %s has ptp index %d", device,
slave->clock->phc_index);
- slave->clock->servo = servo_add(priv, slave->clock);
- if (!slave->clock->servo) {
-
Re: [Linuxptp-devel] [RFC PATCH 00/15] Dynamic sync direction for ts2phc
Vladimir, On Tue, Aug 04, 2020 at 02:40:11PM +0300, Vladimir Oltean wrote: > The diff is large, yes, even though I tried to avoid making unnecessary > changes. I hope it isn't too difficult to review, it isn't as polished > as I typically like, because the idea itself was uncertain, so I > preferred not to spend that extra time at this point in the process. > Anyways, I'm looking forward to your feedback whenever you have the > time. I was going to begin my review, but first I ran my ts2phc smoke test. Valgrind found a memory leak. Thanks, Richard sudo valgrind ./ts2phc -f ts2phc-TC.cfg --free_running=1 ... Control-C... ==9363== HEAP SUMMARY: ==9363== in use at exit: 1,022 bytes in 10 blocks ==9363== total heap usage: 279 allocs, 269 frees, 20,230 bytes allocated ==9363== ==9363== LEAK SUMMARY: ==9363==definitely lost: 568 bytes in 4 blocks ==9363==indirectly lost: 454 bytes in 6 blocks ==9363== possibly lost: 0 bytes in 0 blocks ==9363==still reachable: 0 bytes in 0 blocks ==9363== suppressed: 0 bytes in 0 blocks ==9363== Rerun with --leak-check=full to see details of leaked memory --- ts2phc-TC.cfg --- # for use with three i210 cards [global] use_syslog 0 verbose 1 logging_level 6 ts2phc.pulsewidth 5 [eth3] ts2phc.channel 0 ts2phc.master 1 ts2phc.pin_index0 [eth4] ts2phc.channel 0 ts2phc.extts_polarity both ts2phc.pin_index0 [eth6] ts2phc.channel 0 ts2phc.extts_polarity both ts2phc.pin_index0 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 00/15] Dynamic sync direction for ts2phc
On Mon, Aug 03, 2020 at 06:50:37PM -0700, Richard Cochran wrote: > On Sat, Aug 01, 2020 at 08:46:05PM +0300, Vladimir Oltean wrote: > > At a high level, what I have done is: > > - I moved the PMC related code from phc2sys into pmc_common.c, for > > ts2phc reuse > > - I created an extra abstraction in ts2phc as "struct clock" that would > > represent what's synchronizable. The "master" and "slave" concepts > > retain their meaning, which is: "master" == the device that emits PPS, > > and "slave" == the device that timestamps PPS. > > > > The changes should be backwards-compatible with the non-automatic mode. > > I have only tested non-automatic mode with a PHC master (that's all I > > have) and it still appears to work as before. > > > 22 files changed, 1577 insertions(+), 750 deletions(-) > > This sounds interesting. It is a pretty big diff, and so I'll need > time to go through it all. Right now, I'm rather over committed, and > so please be patient! Sure, there's no hurry. The diff is large, yes, even though I tried to avoid making unnecessary changes. I hope it isn't too difficult to review, it isn't as polished as I typically like, because the idea itself was uncertain, so I preferred not to spend that extra time at this point in the process. Anyways, I'm looking forward to your feedback whenever you have the time. Thanks, -Vladimir ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 00/15] Dynamic sync direction for ts2phc
On Sat, Aug 01, 2020 at 08:46:05PM +0300, Vladimir Oltean wrote: > At a high level, what I have done is: > - I moved the PMC related code from phc2sys into pmc_common.c, for > ts2phc reuse > - I created an extra abstraction in ts2phc as "struct clock" that would > represent what's synchronizable. The "master" and "slave" concepts > retain their meaning, which is: "master" == the device that emits PPS, > and "slave" == the device that timestamps PPS. > > The changes should be backwards-compatible with the non-automatic mode. > I have only tested non-automatic mode with a PHC master (that's all I > have) and it still appears to work as before. > 22 files changed, 1577 insertions(+), 750 deletions(-) This sounds interesting. It is a pretty big diff, and so I'll need time to go through it all. Right now, I'm rather over committed, and so please be patient! Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [RFC PATCH 00/15] Dynamic sync direction for ts2phc
As discussed in this email thread:
https://sourceforge.net/p/linuxptp/mailman/message/37047555/
there is a desire to synchronize multiple DSA switches in a
boundary_clock_jbod setup, using a PPS signal, and the ts2phc program
already offers a solid base.
This patch series extends the ts2phc program to cater for that use case
by introducing a new '-a' option and friends ('--transportSpecific').
This makes it quite similar to phc2sys which has the same ability, just
that ts2phc can give much better performance if the hardware can assist
with that.
The overall board design for my use case is that there's an SoC with an
embedded DSA switch, and hanging off of 3 ports of that embedded switch
are 3 external switches. Every networking device (the DSA master for the
embedded switch, the embedded switch, as well as each individual
external switch) has a PTP clock. The topology for PPS signal
distribution is fixed - that is given by hardware ability - the
/dev/ptp1 clock can emit a valid PPS, and all external switches can
timestamp that PPS (it is connected in a sort-of simplex "bus" design),
and it cannot be any other way around. It looks like this:
+---+
| LS1028A /dev/ptp0 (unused) |
| +--+|
| | DSA master for Felix||
| |(internal ENETC port 2: eno2))||
| ++--+-+ |
| | Felix embedded L2 switch /dev/ptp1 | |
| | PPS master, sync slave | |
| | +--+ +--+ +--+ | |
| | |DSA master for| |DSA master for| |DSA master for| | |
| | | SJA1105 1 | | SJA1105 2 | | SJA1105 3 | | |
| | |(Felix port 1)| |(Felix port 2)| |(Felix port 3)| | |
+--+-+--+---+--+---+--+--+--+
/dev/ptp2/dev/ptp3/dev/ptp4
PPS slave, sync masterPPS slave, sync slave PPS slave, sync slave
+---+ +---+ +---+
| SJA1105 switch 1| | SJA1105 switch 2| | SJA1105 switch 3|
+-+-+-+-+ +-+-+-+-+ +-+-+-+-+
|sw1p0|sw1p1|sw1p2|sw1p3| |sw2p0|sw2p1|sw2p2|sw2p3| |sw3p0|sw3p1|sw3p2|sw3p3|
+-+-+-+-+ +-+-+-+-+ +-+-+-+-+
^
|
GM connected here
In text, it would be described as this:
cat ts2phc.cfg
[global]
first_step_threshold0.2
step_threshold 0.2
ts2phc.pulsewidth 5
ts2phc.perout_phase 0
# Felix
[/dev/ptp1]
ts2phc.master 1
# SJA1105 switch 1
[/dev/ptp2]
ts2phc.channel 0
ts2phc.extts_polarity both
# SJA1105 switch 2
[/dev/ptp3]
ts2phc.channel 0
ts2phc.extts_polarity both
# SJA1105 switch 3
[/dev/ptp4]
ts2phc.channel 0
ts2phc.extts_polarity both
cat gPTP.cfg
[global]
gmCapable 1
priority1 248
priority2 248
logAnnounceInterval 0
logSyncInterval -3
syncReceiptTimeout 3
neighborPropDelayThresh 5
min_neighbor_prop_delay -2000
assume_two_step 1
path_trace_enabled 1
follow_up_info 1
transportSpecific 0x1
ptp_dst_mac 01:80:C2:00:00:0E
network_transport L2
delay_mechanism P2P
step_threshold 0.2
tx_timestamp_timeout20
boundary_clock_jbod 1
[sw1p0]
[sw1p1]
[sw1p2]
[sw1p3]
[sw2p0]
[sw2p1]
[sw2p2]
[sw2p3]
[sw3p0]
[sw3p1]
[sw3p2]
[sw3p3]
At a high level, what I have done is:
- I moved the PMC related code from phc2sys into pmc_common.c, for
ts2phc reuse
- I created an extra abstraction in ts2phc as "struct clock" that would
represent what's synchronizable. The "master" and "slave" concepts
retain their meaning, which is: "master" == the device that emits PPS,
and "slave" == the device that timestamps PPS.
The changes should be backwards-compatible with the non-automatic mode.
I have only tested non-automatic mode with a PHC master (that's all I
have) and it still appears to work as before.
Vladimir Oltean (15):
posix_clock_open: derive PHC index from device name if possible
phc2sys: rename struct node to struct phc2sys_private
phc2sys: break out pmc code into pmc_common.c
pmc_common: fix potential memory leak in run_pmc_events()
ts2phc: create a private data structure
ts2phc: instantiate a full clock structure for every slave PHC
ts2phc: instantiate a full clock structure for every PHC master
ts2phc: instantiate a pmc node
ts2phc_slave: print master offset
ts2phc: split slave poll from servo loop
tmv: introduce a conversion helper from ptp_clock_time
tmv: introduce an initializer from
