Re: [Linuxptp-devel] [PATCH 0/1] Unselected master port may unintentionally continue to request announce/sync/delay_resp packets
On Fri, Dec 02, 2022 at 03:33:41PM -0500, vincent.cheng...@renesas.com wrote: > Problem > === > Unselected master port may unintentionally continue to request > announce/sync/delay_resp packets. > Expecting port with unselected master to only maintain announce messages. This detailed and clear explanation of the issue is simply awesome work! It really helped me to understand the problem. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/1] clock: Fix stale clock parent pid usage after best master change
On Fri, Dec 02, 2022 at 03:33:42PM -0500, vincent.cheng...@renesas.com wrote:
> @@ -2015,6 +2027,8 @@ static void handle_state_decision_event(struct clock *c)
> c->best = best;
> c->best_id = best_id;
>
> + clock_update_parent_identity(c);
Calling this unconditionally, regardless of port state transitions,
will update the parentPortIdentity incorrectly in some cases.
According the 1588, the update should only occur when a port enters
one of two specific states.
> LIST_FOREACH(piter, &c->ports, list) {
> enum port_state ps;
> enum fsm_event event;
Let me suggest another way that avoids the incorrect
parentPortIdentity update:
In port_state_update() don't call unicast_client_state_changed().
Instead, set a flag in the port that means "unicast state dirty".
3417 int port_state_update(struct port *p, enum fsm_event event, int mdiff)
3418 {
...
3447 if (mdiff) {
3448 - unicast_client_state_changed(p);
+ p->unicast_state_dirty = true;
3449 }
3450 if (next != p->state) {
3451 port_show_transition(p, next, event);
3452 p->state = next;
3453 port_notify_event(p, NOTIFY_PORT_STATE);
3454 - unicast_client_state_changed(p);
+ p->unicast_state_dirty = true;
3455 return 1;
3456 }
Then, in handle_state_decision_event(), after the big
LIST_FOREACH(piter, &c->ports, list) loop, iterate once again over the
ports:
LIST_FOREACH(piter, &c->ports, list) {
port_update_unicast_state(piter);
}
where port_update_unicast_state() calls unicast_client_state_changed()
and clears the flag.
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] phc_ctl: use PTP_CLOCK_GETCAPS2 ioctl if available
On Wed, 2022-12-07 at 06:59 -0800, Richard Cochran wrote: > On Thu, Nov 17, 2022 at 02:15:23PM -0800, Jacob Keller wrote: > > On 11/17/2022 1:34 PM, Geva, Erez wrote: > > > > The problem is the fallback works only on build. > > > But if the build system is newer than the running system, the > > > fallback > > > will fail, as you will use the PTP_CLOCK_GETCAPS2 which does not > > > exist > > > on the running old system. > > > > > > > Fair. We likely have the same problem with some of the other "2" > > ioctls, > > since they're handled in a similar way. I think we do the Right(TM) > > thing > > for the sysoff.c where we probe the kernel at run-time. This could > > be done > > here but is probably not really worth it considering that > > PTP_CLOCK_GETCAPS > > functions the same way as PTP_CLOCK_GETCAPS2 for all kernels I > > checked... So > > I guess this is somewhat less likely. > > Jacob, do you want to have phc_ctl fall back to PTP_CLOCK_GETCAPS at > run time if PTP_CLOCK_GETCAPS2 fails? We can use run-time fallback. But personalty, I do not think it worth it. Using PTP_CLOCK_GETCAPS2 is only done for one new field in a debug tool. We can simply wait till PTP_CLOCK_GETCAPS become obsolete or we have a new PTP_CLOCK_GETCAPS3 to handle. > > > I'm not sure if our other PTP ioctls are checked properly like this > > at run > > time... > > Maybe, but only because of sloppiness. Better to support older > kernels at run time, as this is more user friendly. > > Working on embedded systems over the years, I've have often been that > user, and, believe me, it is super annoying when the latest greatest > App isn't backwards compatible. My view too :-) > > Thanks, > Richard Erez ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/1] clock: Fix stale clock parent pid usage after best master change
On Mon, 2022-12-05 at 12:52 -0500, vincent.cheng...@renesas.com wrote:
> On Mon, Dec 05, 2022 at 02:44:07AM EST, Geva, Erez wrote:
> > On Fri, 2022-12-02 at 15:33 -0500,
> > vincent.cheng...@renesas.com wrote:
> > > From: Vincent Cheng
> > >
> > > In handle_state_decision_event(), the update of the clock's
> > > parent
> > > pid after
> > > best master change is non-deterministic. It depends on the port
> > > processing
> > > order and bmc_state_decision() results.
> > >
> > > +static void clock_update_parent_identity(struct clock *c)
> > > +{
> > > + struct parentDS *pds = &c->dad.pds;
> > > +
> > > + if (c->best) {
> > > + pds->parentPortIdentity = c->best-
> > > >dataset.sender;
> > > + } else {
> > > + pds->parentPortIdentity.clockIdentity = c-
> > > > dds.clockIdentity;
> > > + pds->parentPortIdentity.portNumber = 0;
> >
> > Why is the port 0?
> > It make sense that defaultDS do not have port as we work in the
> > context
> > of the clock.
> > Is it make sense to use port ID here?
>
> I would like to say it is port 0 because I consulted 1588-2019.pdf
> and found the
> requirement in "Table 30 - Updates for state decision code M1 and
> M2".
>
> parentDS.clockIdentity member set to the value of
> defaultDS.clockIdentity field.
> parentDS.parentPortIdentity.portNumber member is 0
>
> However, realistically speaking, I copied the logic from
> clock_update_grandmaster()
> and clock_update_slave() first and then looked it up afterwards.
Make sense, please live a comment on the value assignment :-)
Something like ~ "follow IEEE table 30 updates for M1".
No need for a long one, just a short reference.
Erez
>
> >
> >
> > > + }
> > > +}
> > > +
> > > static int clock_utc_correct(struct clock *c, tmv_t ingress)
> > > {
> > > struct timespec offset;
> > > @@ -2015,6 +2027,8 @@ static void
> > > handle_state_decision_event(struct
> > > clock *c)
> > > c->best = best;
> > > c->best_id = best_id;
> > >
> > > + clock_update_parent_identity(c);
> > > +
> > > LIST_FOREACH(piter, &c->ports, list) {
> > > enum port_state ps;
> > > enum fsm_event event;
> >
> >
> > P.S.
> > All patches should be on top of master, no need to comment on
> > obvious.
>
> Ok, will leave out commit next time if it's not needed. Thanks.
>
> Vincent
--
Erez Geva
Siemens AG
www.siemens.com
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/1] clock: Fix stale clock parent pid usage after best master change
On Thu, Dec 08, 2022 at 04:06:08PM EST, Geva, Erez wrote:
>On Mon, 2022-12-05 at 12:52 -0500, vincent.cheng...@renesas.com wrote:
>> On Mon, Dec 05, 2022 at 02:44:07AM EST, Geva, Erez wrote:
>> > On Fri, 2022-12-02 at 15:33 -0500,
>> > vincent.cheng...@renesas.com wrote:
>> > > From: Vincent Cheng
>> > >
>> > > In handle_state_decision_event(), the update of the clock's
>> > > parent
>> > > pid after
>> > > best master change is non-deterministic. It depends on the port
>> > > processing
>> > > order and bmc_state_decision() results.
>> > >
>> > > +static void clock_update_parent_identity(struct clock *c)
>> > > +{
>> > > + struct parentDS *pds = &c->dad.pds;
>> > > +
>> > > + if (c->best) {
>> > > + pds->parentPortIdentity = c->best-
>> > > >dataset.sender;
>> > > + } else {
>> > > + pds->parentPortIdentity.clockIdentity = c-
>> > > > dds.clockIdentity;
>> > > + pds->parentPortIdentity.portNumber = 0;
>> >
>> > Why is the port 0?
>> > It make sense that defaultDS do not have port as we work in the
>> > context
>> > of the clock.
>> > Is it make sense to use port ID here?
>>
>> I would like to say it is port 0 because I consulted 1588-2019.pdf
>> and found the
>> requirement in "Table 30 - Updates for state decision code M1 and
>> M2".
>>
>> parentDS.clockIdentity member set to the value of
>> defaultDS.clockIdentity field.
>> parentDS.parentPortIdentity.portNumber member is 0
>>
>> However, realistically speaking, I copied the logic from
>> clock_update_grandmaster()
>> and clock_update_slave() first and then looked it up afterwards.
>
>Make sense, please live a comment on the value assignment :-)
>
>Something like ~ "follow IEEE table 30 updates for M1".
>No need for a long one, just a short reference.
Sure thing. Fixed in PATCH v2. However, clock_update_parent_identity() is
not needed, so put the comment in clock_update_grandmaster().
Vincent
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v2 1/1] clock: Fix stale clock parent pid usage after best master change
From: Vincent Cheng
Signed-off-by: Vincent Cheng
---
clock.c| 6 ++
port.c | 12 ++--
port.h | 7 +++
port_private.h | 1 +
4 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/clock.c b/clock.c
index 134c7c3..a68a732 100644
--- a/clock.c
+++ b/clock.c
@@ -669,7 +669,9 @@ static void clock_update_grandmaster(struct clock *c)
struct parentDS *pds = &c->dad.pds;
memset(&c->cur, 0, sizeof(c->cur));
memset(c->ptl, 0, sizeof(c->ptl));
+
pds->parentPortIdentity.clockIdentity = c->dds.clockIdentity;
+ /* Follow IEEE 1588 Table 30: Updates for state decision code M1 and M2
*/
pds->parentPortIdentity.portNumber = 0;
pds->grandmasterIdentity= c->dds.clockIdentity;
pds->grandmasterClockQuality= c->dds.clockQuality;
@@ -2049,6 +2051,10 @@ static void handle_state_decision_event(struct clock *c)
}
port_dispatch(piter, event, fresh_best);
}
+
+ LIST_FOREACH(piter, &c->ports, list) {
+ port_update_unicast_state(piter);
+ }
}
struct clock_description *clock_description(struct clock *c)
diff --git a/port.c b/port.c
index 6baf5c8..c8cba5b 100644
--- a/port.c
+++ b/port.c
@@ -3445,13 +3445,13 @@ int port_state_update(struct port *p, enum fsm_event
event, int mdiff)
}
if (mdiff) {
- unicast_client_state_changed(p);
+ p->unicast_state_dirty = true;
}
if (next != p->state) {
port_show_transition(p, next, event);
p->state = next;
port_notify_event(p, NOTIFY_PORT_STATE);
- unicast_client_state_changed(p);
+ p->unicast_state_dirty = true;
return 1;
}
@@ -3462,3 +3462,11 @@ enum bmca_select port_bmca(struct port *p)
{
return p->bmca;
}
+
+void port_update_unicast_state(struct port *p)
+{
+ if (p->unicast_state_dirty) {
+ unicast_client_state_changed(p);
+ p->unicast_state_dirty = false;
+ }
+}
diff --git a/port.h b/port.h
index 4854698..57c8c2f 100644
--- a/port.h
+++ b/port.h
@@ -357,4 +357,11 @@ enum bmca_select port_bmca(struct port *p);
*/
void tc_cleanup(void);
+/**
+ * Update port's unicast state if port's unicast_state_dirty is true.
+ *
+ * @param port A port instance.
+ */
+void port_update_unicast_state(struct port *p);
+
#endif
diff --git a/port_private.h b/port_private.h
index d27dceb..13a6c6a 100644
--- a/port_private.h
+++ b/port_private.h
@@ -158,6 +158,7 @@ struct port {
int inhibit_multicast_service;
/* slave event monitoring */
struct monitor *slave_event_monitor;
+ bool unicast_state_dirty;
};
#define portnum(p) (p->portIdentity.portNumber)
--
2.34.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v2 0/1] Unselected master port may unintentionally continue to request announce/sync/delay_resp packets
From: Vincent Cheng
Problem
===
Unselected master port may unintentionally continue to request
announce/sync/delay_resp packets.
Expecting port with unselected master to only maintain announce messages.
Setup
=
Client running with unicast mode and 2 ports, each port has single entry
unicast_master_table.
===
| Client |
| clockClass 248 |
| dataset_comparison G.8275.x |
| clock_type BC|
| unicast_req_duration10|
| |
| enp0s8 enp0s9 |
| port 1 port 2 |
===
| |
| Server B
| 080027.fffe.fc19f1, clockClass 7
|
Server A
080027.fffe.fd, clockClass 7
unicast_req_duration is shortened so don't have to wait as long to see effect.
Server A & B must have same timing properties to cause port state PASSIVE.
ie. telecom_dscmp() -> dscmp2() to compare the port identity to determine best
master.
If we don't enter PASSIVE state, it is harder to see the problem.
If we use clockClass to distinguish the masters, the problem is still there,
but quickly fixed in subsequent PRE-MASTER -> MASTER state decision event.
Client
--
clockClass 248
dataset_comparison G.8275.x
clock_type BC
unicast_req_duration10
Test
1. Start Client and Server A.
2. Port 1:Server A is best master.
port 1: MASTER to UNCALIBRATED
port 2: MASTER
3. Start Server B.
4. Port 2:Server B is best master.
port 1: UNCALIBRATED to PASSIVE
port 2: MASTER to UNCALIBRATED
5. Port 1 continues to request annc/sync/delay_resp on next renewal
Failure log (with some debug annotations)
===
# Start Client and Server A
ptp4l[82910.789]: port 1 (enp0s8): unicast ANNOUNCE granted for 10 sec
ptp4l[82910.800]: port 1 (enp0s8): new foreign master 080027.fffe.fd-1
...
# Server A is best master
ptp4l[82915.056]: selected best master clock 080027.fffe.fd
ptp4l[82915.056]: port 1 (enp0s8): unicast_fsm: UC_HAVE_ANN -> UC_NEED_SYDY on
UC_EV_SELECTED
...
ptp4l[82922.789]: port 1 (enp0s8): time to renew unicast subscriptions
(UC_HAVE_SYDY)
ptp4l[82922.790]: port 1 (enp0s8): unicast ANNOUNCE granted for 10 sec
ptp4l[82922.790]: port 1 (enp0s8): unicast SYNC granted for 10 sec
ptp4l[82922.790]: port 1 (enp0s8): unicast DELAY_RESP granted for 10 sec
...
ptp4l[82938.792]: port 1 (enp0s8): time to renew unicast subscriptions
(UC_HAVE_SYDY)
ptp4l[82938.793]: port 1 (enp0s8): unicast ANNOUNCE granted for 10 sec
ptp4l[82938.793]: port 1 (enp0s8): unicast SYNC granted for 10 sec
ptp4l[82938.793]: port 1 (enp0s8): unicast DELAY_RESP granted for 10 sec
# Start Server B
ptp4l[82939.057]: port 2 (enp0s9): new foreign master 080027.fffe.fc19f1-1
# Port 2:Server B is best master
ptp4l[82943.311]: selected best master clock 080027.fffe.fc19f1
ptp4l[82943.311]: port 1 (enp0s8): unicast_fsm: UC_HAVE_SYDY -> UC_HAVE_SYDY on
UC_EV_SELECTED
ptp4l[82943.311]: port 1 (enp0s8): UNCALIBRATED to PASSIVE on RS_PASSIVE
ptp4l[82943.311]: port 2 (enp0s9): unicast_fsm: UC_HAVE_ANN -> UC_NEED_SYDY on
UC_EV_SELECTED
ptp4l[82943.311]: port 2 (enp0s9): MASTER to UNCALIBRATED on RS_SLAVE
...
# Port 1 continues to request annc/sync/delay_resp on subsequent unicast renewal
ptp4l[82946.793]: port 1 (enp0s8): time to renew unicast subscriptions
(UC_HAVE_SYDY)
ptp4l[82946.794]: port 1 (enp0s8): unicast ANNOUNCE granted for 10 sec
ptp4l[82946.795]: port 1 (enp0s8): unicast SYNC granted for 10 sec
ptp4l[82946.795]: port 1 (enp0s8): unicast DELAY_RESP granted for 10 sec
...
ptp4l[82950.794]: port 2 (enp0s9): time to renew unicast subscriptions
(UC_HAVE_SYDY)
ptp4l[82950.795]: port 2 (enp0s9): unicast ANNOUNCE granted for 10 sec
ptp4l[82950.795]: port 2 (enp0s9): unicast SYNC granted for 10 sec
ptp4l[82950.795]: port 2 (enp0s9): unicast DELAY_RESP granted for 10 sec
...
ptp4l[82954.795]: port 1 (enp0s8): time to renew unicast subscriptions
(UC_HAVE_SYDY)
ptp4l[82954.797]: port 1 (enp0s8): unicast ANNOUNCE granted for 10 sec
ptp4l[82954.797]: port 1 (enp0s8): unicast SYNC granted for 10 sec
ptp4l[82954.797]: port 1 (enp0s8): unicast DELAY_RESP granted for 10 sec
...
ptp4l[82958.796]: port 2 (enp0s9): time to renew unicast subscriptions
(UC_HAVE_SYDY)
ptp4l[82958.797]: port 2 (enp0s9): unicast ANNOUNCE granted for 10 sec
ptp4l[82958.797]: port 2 (enp0s9): unicast SYNC granted for 10 sec
ptp4l[82958.797]: port 2 (enp0s9): unicast DELAY_RESP granted for 10 sec
Cause
=
handle_state_decision_event():
c->best = best; // *** parent
pid may have changed
c->best_id = best_id;
LIST_FOREACH(piter, &c->ports, list) {
enum port_state ps;
enum fsm_event
Re: [Linuxptp-devel] [PATCH 1/1] clock: Fix stale clock parent pid usage after best master change
On Thu, Dec 08, 2022 at 10:36:57AM EST, Richard Cochran wrote:
>On Fri, Dec 02, 2022 at 03:33:42PM -0500, vincent.cheng...@renesas.com wrote:
>
>> @@ -2015,6 +2027,8 @@ static void handle_state_decision_event(struct clock
>> *c)
>> c->best = best;
>> c->best_id = best_id;
>>
>> +clock_update_parent_identity(c);
>
>Calling this unconditionally, regardless of port state transitions,
>will update the parentPortIdentity incorrectly in some cases.
>
>According the 1588, the update should only occur when a port enters
>one of two specific states.
>
>> LIST_FOREACH(piter, &c->ports, list) {
>> enum port_state ps;
>> enum fsm_event event;
>
>Let me suggest another way that avoids the incorrect
>parentPortIdentity update:
>
>In port_state_update() don't call unicast_client_state_changed().
>
>Instead, set a flag in the port that means "unicast state dirty".
>
> 3417 int port_state_update(struct port *p, enum fsm_event event, int mdiff)
> 3418 {
>...
> 3447 if (mdiff) {
> 3448 - unicast_client_state_changed(p);
>+ p->unicast_state_dirty = true;
> 3449 }
> 3450 if (next != p->state) {
> 3451 port_show_transition(p, next, event);
> 3452 p->state = next;
> 3453 port_notify_event(p, NOTIFY_PORT_STATE);
> 3454 - unicast_client_state_changed(p);
>+ p->unicast_state_dirty = true;
> 3455 return 1;
> 3456 }
>
>Then, in handle_state_decision_event(), after the big
>LIST_FOREACH(piter, &c->ports, list) loop, iterate once again over the
>ports:
>
>LIST_FOREACH(piter, &c->ports, list) {
>port_update_unicast_state(piter);
>}
>
>where port_update_unicast_state() calls unicast_client_state_changed()
>and clears the flag.
>
>Thanks,
>Richard
Thank-you for the feedback. Implemented above suggestions in PATCH v2.
Thanks,
Vincent
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
