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 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 4/4] [Interface Rate TLV] adding delay asymmetry calculation
>
> > + /* 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.
>
> > +}
>
[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)
"
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 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
> > Signed-off-by: Leon Goldin
> > Signed-off-by: Devasish Dey
> > Signed-off-by: Vipin Sharma
> > ---
> > 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
> > *p,
> > return 0;
> > }
> >
> > +static int process_interface_rate(struct port *p,
> > + struct msg_interface_rate_tlv *r)
> > +{
> > + Integer64 delayAsymmetry;
> > + doublensDelay;
> > + 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);
> > +
Re: [Linuxptp-devel] [PATCH v3 4/4] [Interface Rate TLV] adding delay asymmetry calculation
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
> Signed-off-by: Leon Goldin
> Signed-off-by: Devasish Dey
> Signed-off-by: Vipin Sharma
> ---
> 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;
> 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,
