On Tue, 2022-12-13 at 10:56 +0530, Devasish Dey wrote: > + /* Megabits per secon converted to attoseconds per bit. */ > + return 1000000000000ULL/ 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 <erez.geva....@siemens.com<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 > <greg.armstrong...@renesas.com<mailto:greg.armstrong...@renesas.com>> > Signed-off-by: Leon Goldin > <leon.goldin...@renesas.com<mailto:leon.goldin...@renesas.com>> > Signed-off-by: Devasish Dey > <devasish....@syncmonk.net<mailto:devasish....@syncmonk.net>> > Signed-off-by: Vipin Sharma > <vipin.sha...@syncmonk.net<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 1000000000000ULL/ 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; > bool iface_rate_tlv; > + Integer64 portAsymmetry; > struct PortStats stats; > struct PortServiceStats service_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 > *p, > return 0; > } > > +static int process_interface_rate(struct port *p, > + struct msg_interface_rate_tlv *r) > +{ > + Integer64 delayAsymmetry; > + double nsDelay; > + Integer64 slaveBitPeriod; > + Integer64 masterBitPeriod; > + > + if (p->iface_rate_tlv && interface_ifinfo_valid(p->iface)) { > + slaveBitPeriod = interface_bitperiod(p->iface); > + masterBitPeriod = r->interfaceBitPeriod; > + > + /* Delay Asymmetry Calculation */ > + nsDelay = (masterBitPeriod - slaveBitPeriod) / (2 * > 1.0e9); > + delayAsymmetry = > + (r->numberOfBitsAfterTimestamp - r- > >numberOfBitsBeforeTimestamp) * nsDelay; > + > + if (delayAsymmetry != p->portAsymmetry) { > + p->asymmetry += ((delayAsymmetry - p- > >portAsymmetry) << 16); > + p->portAsymmetry = delayAsymmetry; > + } > + } > + return 0; > +} > + > int process_signaling(struct port *p, struct ptp_message *m) > { > struct tlv_extra *extra; > + struct organization_tlv *org; > struct msg_interval_req_tlv *r; > + struct msg_interface_rate_tlv *rate; > int err = 0, result; > > switch (p->state) { > @@ -160,11 +187,17 @@ int process_signaling(struct port *p, struct > ptp_message *m) > break; > > case TLV_ORGANIZATION_EXTENSION: > - r = (struct msg_interval_req_tlv *) extra- > >tlv; > + org = (struct organization_tlv *)extra->tlv; > > - if (0 == memcmp(r->id, ieee8021_id, > sizeof(ieee8021_id)) && > - r->subtype[0] == 0 && r->subtype[1] == 0 > && r->subtype[2] == 2) > + if (0 == memcmp(org->id, ieee8021_id, > sizeof(ieee8021_id)) && > + org->subtype[0] == 0 && org->subtype[1] > == 0 && org->subtype[2] == 2) { > + r = (struct msg_interval_req_tlv *) > extra->tlv; > err = process_interval_request(p, r); > + } else if (0 == memcmp(org->id, itu_t_id, > sizeof(itu_t_id)) && > + org->subtype[0] == 0 && org- > >subtype[1] == 0 && org->subtype[2] == 2) { > + rate = (struct msg_interface_rate_tlv > *) extra->tlv; > + err = process_interface_rate(p, > rate); > + } > break; > } > } > diff --git a/ptp4l.8 b/ptp4l.8 > index cd6299f..e96d090 100644 > --- a/ptp4l.8 > +++ b/ptp4l.8 > @@ -697,6 +697,13 @@ The default is 0 (disabled). > Print messages to the system log if enabled. > The default is 1 (enabled). > .TP > +.B interface_rate_tlv > +When the client and server are operating are operating at different > interface rate, > +delay asymmetry caused due to different interface rate needs to be > compensated. > +The server sends its interface rate using interface rate TLV > +as per G.8275.2 Annex D. > +The default is 0 (does not support interface rate tlv). > +.TP > .B summary_interval > The time interval in which are printed summary statistics of the > clock. It is > specified as a power of two in seconds. The statistics include > offset root mean > diff --git a/tlv.c b/tlv.c > index 35bee4f..7a2a4fa 100644 > --- a/tlv.c > +++ b/tlv.c > @@ -681,6 +681,7 @@ static void nsm_resp_pre_send(struct tlv_extra > *extra) > static int org_post_recv(struct organization_tlv *org) > { > struct follow_up_info_tlv *f; > + struct msg_interface_rate_tlv *m; > > if (0 == memcmp(org->id, ieee8021_id, sizeof(ieee8021_id))) { > if (org->subtype[0] || org->subtype[1]) { > @@ -701,6 +702,21 @@ static int org_post_recv(struct organization_tlv > *org) > if (org->length + sizeof(struct TLV) != > sizeof(struct msg_interval_req_tlv)) > goto bad_length; > } > + } else if (0 == memcmp(org->id, itu_t_id, sizeof(itu_t_id))) > { > + if (org->subtype[0] || org->subtype[1]) { > + return 0; > + } > + switch (org->subtype[2]) { > + case 2: > + if (org->length + sizeof(struct TLV) != > sizeof(struct msg_interface_rate_tlv)) > + goto bad_length; > + m = (struct msg_interface_rate_tlv *)org; > + m->interfaceBitPeriod = net2host64(m- > >interfaceBitPeriod); > + m->numberOfBitsBeforeTimestamp = ntohs(m- > >numberOfBitsBeforeTimestamp); > + m->numberOfBitsAfterTimestamp = ntohs(m- > >numberOfBitsAfterTimestamp); > + break; > + } > + > } > return 0; > bad_length: > @@ -710,6 +726,7 @@ bad_length: > static void org_pre_send(struct organization_tlv *org) > { > struct follow_up_info_tlv *f; > + struct msg_interface_rate_tlv *m; > > if (0 == memcmp(org->id, ieee8021_id, sizeof(ieee8021_id))) { > if (org->subtype[0] || org->subtype[1]) { > @@ -724,6 +741,18 @@ static void org_pre_send(struct organization_tlv > *org) > f->scaledLastGmPhaseChange = htonl(f- > >scaledLastGmPhaseChange); > break; > } > + } else if (0 == memcmp(org->id, itu_t_id, sizeof(itu_t_id))) > { > + if (org->subtype[0] || org->subtype[1]) { > + return; > + } > + switch (org->subtype[2]) { > + case 2: > + m = (struct msg_interface_rate_tlv *)org; > + m->interfaceBitPeriod = host2net64(m- > >interfaceBitPeriod); > + m->numberOfBitsBeforeTimestamp = htons(m- > >numberOfBitsBeforeTimestamp); > + m->numberOfBitsAfterTimestamp = htons(m- > >numberOfBitsAfterTimestamp); > + break; > + } > } > } > > diff --git a/unicast_service.c b/unicast_service.c > index 3154894..1078041 100644 > --- a/unicast_service.c > +++ b/unicast_service.c > @@ -84,6 +84,30 @@ static int attach_grant(struct ptp_message *msg, > return 0; > } > > +static int attach_interface_rate(struct ptp_message *msg, > + uint64_t iface_bit_period, > + uint16_t no_of_bits_before_ts, > + uint16_t no_of_bits_after_ts) > +{ > + struct msg_interface_rate_tlv *mir; > + struct tlv_extra *extra; > + > + extra = msg_tlv_append(msg, sizeof(*mir)); > + if (!extra) { > + return -1; > + } > + mir = (struct msg_interface_rate_tlv *) extra->tlv; > + mir->type = TLV_ORGANIZATION_EXTENSION; > + mir->length = sizeof(*mir) - sizeof(mir->type) - sizeof(mir- > >length); > + memcpy(mir->id, itu_t_id, sizeof(itu_t_id)); > + mir->subtype[2] = 2; > + mir->interfaceBitPeriod = iface_bit_period; > + mir->numberOfBitsBeforeTimestamp = no_of_bits_before_ts; > + mir->numberOfBitsAfterTimestamp = no_of_bits_after_ts; > + > + return 0; > +} > + > static int compare_timeout(void *ain, void *bin) > { > struct unicast_service_interval *a, *b; > @@ -256,6 +280,14 @@ static int unicast_service_reply(struct port *p, > struct ptp_message *dst, > if (err) { > goto out; > } > + if (p->iface_rate_tlv && duration > 0 && > interface_ifinfo_valid(p->iface)) { > + err = attach_interface_rate(msg, > + interface_bitperiod(p->iface), 64, > 720); > + if (err) { > + goto out; > + } > + } > + > err = port_prepare_and_send(p, msg, TRANS_GENERAL); > if (err) { > pr_err("%s: signaling message failed", p->log_name); Erez Erez
_______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
