Re: [Linuxptp-devel] [PATCH v3 1/4] [Interface Rate TLV] function to support get interface speed via ethtool
On Tue, Dec 13, 2022 at 10:39:59AM +0530, Devasish Dey wrote: > > +struct sk_if_info { > > + bool valid; > It is better not to use bool in a structure, as the size is usually > int, i.e. 64 bits to hold 1 bit. Because we don't have huge arrays of these structs, the size isn't critical IMO. > [Devasish]: Earlier we had it as an integer. We can update it to uint8_t as > well. But Richard suggested updating it to Boolean. > So, leaving this to Richard. I think 'bool' is fine here. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3 4/4] [Interface Rate TLV] adding delay asymmetry calculation
On Tue, 2022-12-13 at 10:56 +0530, Devasish Dey wrote: > + /* Megabits per secon converted to attoseconds per bit. */ > + return 1ULL/ iface->if_info.speed; Performing division in running is not a very good idea. It is better to perform the division when updating the speed and store it in if_info. I agree with Miroslav, calculation is better, I did not suggest using hard coded values, so not need to defend. I simply suggest to move the calculation to where you set "if_info.speed". So instead of calculate it every cycle, we only calculate when speed is changed. Division is expensive (unless it is a left shift). I usually compare new speed with old speed before calculate, if the speed is the same, so is the bit time :-) Are you sure we need attoseconds (10^-18), we usually uses nanoseconds (10^-9). I would except a better resolution, but that much? Please explain your choosing. > +} [Devasish]: The initial changes were with hardcoded values for the speed to avoid the calculation. Miroslav suggested this way to have clean code. Yes the calculation is based on attoseconds and not nanoseconds This is as per standard G.8275.2 From G.8275.2 (06/2021) The linuxptp is mainly implement IEEE. That does not means we can not support or use ITU standards, but if you do, please add a comment with reference. This applies as well as to choosing attoseconds. " interfaceBitPeriod (Uinteger64) The period of 1-bit of the transmitting PTP timestamp interface, excluding line encoding. The value is encoded as an unsigned integer in units of attoseconds (10–18 s) to accommodate interface bit periods less than 1 ns." Thanks, Devasish. On Fri, 9 Dec 2022 at 18:08, Geva, Erez mailto:erez.geva@siemens.com>> wrote: On Wed, 2022-12-07 at 17:34 +0530, SyncMonk Technologies wrote: > Delay asymmetry calculation based on the PTP port interface speed of > master obtained from TLV and the slave interface rate obtained by > ethtool. > > v3: updating network/host byte order handling. > v1: initial commit > > Signed-off-by: Greg Armstrong > mailto:greg.armstrong...@renesas.com>> > Signed-off-by: Leon Goldin > mailto:leon.goldin...@renesas.com>> > Signed-off-by: Devasish Dey > mailto:devasish@syncmonk.net>> > Signed-off-by: Vipin Sharma > mailto:vipin.sha...@syncmonk.net>> > --- > interface.c | 10 ++ > interface.h | 7 +++ > port_private.h| 1 + > port_signaling.c | 39 --- > ptp4l.8 | 7 +++ > tlv.c | 29 + > unicast_service.c | 32 > 7 files changed, 122 insertions(+), 3 deletions(-) > > diff --git a/interface.c b/interface.c > index 3157e8c..02d530e 100644 > --- a/interface.c > +++ b/interface.c > @@ -94,3 +94,13 @@ int interface_get_vclock(struct interface *iface) > { > return iface->vclock; > } > + > +uint64_t interface_bitperiod(struct interface *iface) > +{ > + if (!iface->if_info.valid) > + return 0; > + > + /* Megabits per secon converted to attoseconds per bit. */ > + return 1ULL/ iface->if_info.speed; Performing division in running is not a very good idea. It is better to perform the division when updating the speed and store it in if_info. Are you sure we need attoseconds (10^-18), we usually uses nanoseconds (10^-9). I would except a better resolution, but that much? Please explain your choosing. > +} > + > diff --git a/interface.h b/interface.h > index f4b9545..7c9a6bd 100644 > --- a/interface.h > +++ b/interface.h > @@ -113,4 +113,11 @@ void interface_set_vclock(struct interface > *iface, int vclock); > */ > int interface_get_vclock(struct interface *iface); > > +/** > + * Obtains the interface bit period based on the speed. Perhaps: "Obtains bit period based on interface speed." > + * @param iface The interface of interest. We must be interesting "Pointer to the interface" can be suffiecnt :-) > + * @return if valid speed return interface bitperiod in atto > seconds. No need to make the return complicated, make it simple "return interface bit period in attoseconds". We know functions handle errors. > + */ > +uint64_t interface_bitperiod(struct interface *iface); > + > #endif > diff --git a/port_private.h b/port_private.h > index d6487eb..6ad4af8 100644 > --- a/port_private.h > +++ b/port_private.h > @@ -146,6 +146,7 @@ struct port { > UInteger8 delay_response_counter; > UInteger8 delay_response_timeout; > booliface_rate_tlv; > + Integer64 portAsymmetry; > struct PortStatsstats; > struct PortServiceStatsservice_stats; > /* foreignMasterDS */ > diff --git a/port_signaling.c b/port_signaling.c > index ed217c0..75a0689 100644 > --- a/port_signaling.c > +++ b/port_signaling.c > @@ -103,10 +103,37 @@ static int process_interval_request(struct port >
Re: [Linuxptp-devel] [PATCH v3 1/4] [Interface Rate TLV] function to support get interface speed via ethtool
On Tue, 2022-12-13 at 10:39 +0530, Devasish Dey wrote: > + goto failed; > + } I think it is better to merge this ioctl and the socket creation with sk_get_ts_info(). No reason for duplication. You can use a static function or inline one. > + > + if (ecmd.req.link_mode_masks_nwords >= 0 || > + ecmd.req.cmd != ETHTOOL_GLINKSETTINGS) { > + return 1; > + } [Devasish]: This can be done. > + /* clear data and ensure it is not marked valid */ > + memset(if_info, 0, sizeof(struct sk_if_info)); > + return -1; You need to close the socket! [Devasish]: Noted. Will be updated in the next patch. > +struct sk_if_info { > + bool valid; It is better not to use bool in a structure, as the size is usually int, i.e. 64 bits to hold 1 bit. [Devasish]: Earlier we had it as an integer. We can update it to uint8_t as well. But Richard suggested updating it to Boolean. So, leaving this to Richard. Good point, we can define a Boolean type using uint8_t for structures use. But we can leave this decision to Richard. > + int speed; What are the units here? Old systems use 32 bits, if you measure in bits per second, the maximum is 4 GB. It is better to use uint64_t and use 1 mbps as units. [Devasish]: It is the same as the speed in ethtool_link_settings which is a uint32_t variable and it is in Mbps. Will change it to uint32_t and update the comments. We do not have to follow the kernel ioctl type for speed, they tend to change them from time to time. Probably, someone in the past choose 'int' as he did not predict future speed will pass 31 bits (int is signed). And also the use of uintNN_t was not so common than. But as sk.c is our internal layer, it can translate from kernel units and types to our application units and types. You do provide internal structure and do not use the kernel one :-) So if in the future the kernel will use a new ioctl for speed as int will be too small, we can retain out internal interface. This is my opinion, but you can defer. Thanks, Devasish. Erez ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3 4/4] [Interface Rate TLV] adding delay asymmetry calculation
On Tue, 2022-12-13 at 17:27 +0100, Erez Geva wrote: On Tue, 2022-12-13 at 10:56 +0530, Devasish Dey wrote: > + /* Megabits per secon converted to attoseconds per bit. */ > + return 1ULL/ iface->if_info.speed; Performing division in running is not a very good idea. It is better to perform the division when updating the speed and store it in if_info. I agree with Miroslav, calculation is better, I did not suggest using hard coded values, so not need to defend. I simply suggest to move the calculation to where you set "if_info.speed". So instead of calculate it every cycle, we only calculate when speed is changed. Division is expensive (unless it is a left shift). I usually compare new speed with old speed before calculate, if the speed is the same, so is the bit time :-) Are you sure we need attoseconds (10^-18), we usually uses nanoseconds (10^-9). I would except a better resolution, but that much? Please explain your choosing. > +} [Devasish]: The initial changes were with hardcoded values for the speed to avoid the calculation. Miroslav suggested this way to have clean code. Yes the calculation is based on attoseconds and not nanoseconds This is as per standard G.8275.2 From G.8275.2 (06/2021) The linuxptp is mainly implement IEEE. That does not means we can not support or use ITU standards, but if you do, please add a comment with reference. This applies as well as to choosing attoseconds. This comment is also valid for the 'Uinteger64' type, as it is not currently used by IEEE, please add a comment it is used for ITU standard parameters. If future IEEE will use the Uinteger64 too, we can remove the comment. " interfaceBitPeriod (Uinteger64) The period of 1-bit of the transmitting PTP timestamp interface, excluding line encoding. The value is encoded as an unsigned integer in units of attoseconds (10–18 s) to accommodate interface bit periods less than 1 ns." Thanks, Devasish. Erez ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3 1/4] [Interface Rate TLV] function to support get interface speed via ethtool
On Tue, 2022-12-13 at 17:39 +0100, Erez Geva wrote: On Tue, 2022-12-13 at 10:39 +0530, Devasish Dey wrote: > + goto failed; > + } I think it is better to merge this ioctl and the socket creation with sk_get_ts_info(). No reason for duplication. You can use a static function or inline one. > + > + if (ecmd.req.link_mode_masks_nwords >= 0 || > + ecmd.req.cmd != ETHTOOL_GLINKSETTINGS) { > + return 1; > + } [Devasish]: This can be done. > + /* clear data and ensure it is not marked valid */ > + memset(if_info, 0, sizeof(struct sk_if_info)); > + return -1; You need to close the socket! [Devasish]: Noted. Will be updated in the next patch. > +struct sk_if_info { > + bool valid; It is better not to use bool in a structure, as the size is usually int, i.e. 64 bits to hold 1 bit. [Devasish]: Earlier we had it as an integer. We can update it to uint8_t as well. But Richard suggested updating it to Boolean. So, leaving this to Richard. Good point, we can define a Boolean type using uint8_t for structures use. But we can leave this decision to Richard. > + int speed; What are the units here? Old systems use 32 bits, if you measure in bits per second, the maximum is 4 GB. It is better to use uint64_t and use 1 mbps as units. [Devasish]: It is the same as the speed in ethtool_link_settings which is a uint32_t variable and it is in Mbps. Will change it to uint32_t and update the comments. We do not have to follow the kernel ioctl type for speed, they tend to change them from time to time. Probably, someone in the past choose 'int' as he did not predict future speed will pass 31 bits (int is signed). And also the use of uintNN_t was not so common than. But as sk.c is our internal layer, it can translate from kernel units and types to our application units and types. You do provide internal structure and do not use the kernel one :-) So if in the future the kernel will use a new ioctl for speed as int will be too small, we can retain out internal interface. This is my opinion, but you can defer. Sorry, I miss reading your answer properly, please disregard this part. Erez Thanks, Devasish. Erez ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v3 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, >ports, list) { enum port_state ps; enum fsm_event
[Linuxptp-devel] [PATCH v3 1/1] clock: Fix stale clock parent pid usage after best master change
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. Cause = In handle_state_decision_event() loop, the update of the clock's parent pid to reflect best master change is non-deterministic. It depends on the port processing order and bmc_state_decision() results. However, port_dispatch() called at the end of each loop iteration will use the clock's parent pid in subsequent unicast_client_state_changed() to drive the unicast FSM. If the clock parent pid is not updated properly prior to unicast_client_state_changed(), the unicast FSM may not transition properly. Solution v2 === Refactor port_state_update() to mark unicast_state_dirty instead of calling unicast_client_state_changed(). Add new port loop after handle_state_decision_event() port loop to call unicast_client_state_changed() after all necessary parent pid has been updated. 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 = >dad.pds; memset(>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, >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
Re: [Linuxptp-devel] [PATCH v2 1/1] clock: Fix stale clock parent pid usage after best master change
On 12/9/2022 1:56 AM, vincent.cheng...@renesas.com wrote: > From: Vincent Cheng > Please add a short description to the patch (ex. the problem and solution from the cover letter), as the description from the cover letter will not get to the git repo. > 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 = >dad.pds; > memset(>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, >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) ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel