[Linuxptp-devel] tsproc_update_offset() fails when it should succeed

[Burkhard Ilsen]

Hello everyone,

while debugging my 802.1AS endpoint I recognized that the servo was idle
after the first step.
It stopped processing sync messages until the next peer delay was measured.
The reason is that t3 is reset after the first step letting
tsproc_update_offset() fail.
I think tsproc_update_offset() should not fail in this case because the
filtered delay is still available and t3 is not needed.
T3 is needed only when using raw_mode or weighting.

I propose checking the variables only if they are used:

--- a\tsproc.c
+++ b\tsproc.c
@@ -163,12 +163,17 @@
{
tmv_t delay, raw_delay = 0;

- if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2) ||
- tmv_is_zero(tsp->t3))
+ if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2))
return -1;

- if (tsp->raw_mode || tsp->weighting)
+ if (!tsp->raw_mode && tmv_is_zero(tsp->filtered_delay))
+ return -1;
+
+ if (tsp->raw_mode || tsp->weighting) {
+ if (tmv_is_zero(tsp->t3))
+ return -1;
raw_delay = get_raw_delay(tsp);
+ }

delay = tsp->raw_mode ? raw_delay : tsp->filtered_delay;

[Keller, Jacob E]

Post by Burkhard Ilsen
Hello everyone,

Hi,

Just FYI, this mailing lists preference is to send plain-text emails if
possible.

Post by Burkhard Ilsen
while debugging my 802.1AS endpoint I recognized that the servo was
idle after the first step.
It stopped processing sync messages until the next peer delay was measured.
The reason is that t3 is reset after the first step letting
tsproc_update_offset() fail.
I think tsproc_update_offset() should not fail in this case because
the filtered delay is still available and t3 is not needed.
T3 is needed only when using raw_mode or weighting.

This makes sense to me, if we don't need t3 then we shouldn't be
erroring out when there isn't a t3 value.

Thanks,
Jake

Post by Burkhard Ilsen
--- a\tsproc.c
+++ b\tsproc.c
@@ -163,12 +163,17 @@
 {
  tmv_t delay, raw_delay = 0;
 
- if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2) ||
-     tmv_is_zero(tsp->t3))
+ if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2))
  return -1;
 
- if (tsp->raw_mode || tsp->weighting)
+ if (!tsp->raw_mode && tmv_is_zero(tsp->filtered_delay))
+ return -1;
+
+ if (tsp->raw_mode || tsp->weighting) {
+ if (tmv_is_zero(tsp->t3))
+ return -1;
  raw_delay = get_raw_delay(tsp);
+ }
 
  delay = tsp->raw_mode ? raw_delay : tsp->filtered_delay;
 
-------------------------------------------------------------------
-----------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Richard Cochran]

Post by Burkhard Ilsen
The reason is that t3 is reset after the first step letting
tsproc_update_offset() fail.

You mean the call to tsproc_reset() after SERVO_JUMP...

Post by Burkhard Ilsen
I think tsproc_update_offset() should not fail in this case because the
filtered delay is still available and t3 is not needed.

which passes fail=0, preserving tsproc->filter_delay. Since we took
the trouble to keep filter_delay, then yes, we should also use it!

Post by Burkhard Ilsen
T3 is needed only when using raw_mode or weighting.

Like Jake said, your patch is whitespace damaged. Please teach your
mailer about plain text.

Thanks,
Richard

[Burkhard Ilsen]

Hi Richard, Jake

Post by Richard Cochran
You mean the call to tsproc_reset() after SERVO_JUMP...
which passes fail=0, preserving tsproc->filter_delay. Since we took
the trouble to keep filter_delay, then yes, we should also use it!

Exaclty.

Hearing no complains I propose the attached patch.

Post by Richard Cochran
Like Jake said, your patch is whitespace damaged.

Thanks for the info, I am now using your pre-commit hook.

Burkhard

[Miroslav Lichvar]

Post by Burkhard Ilsen
Hearing no complains I propose the attached patch.

Post by Richard Cochran
Like Jake said, your patch is whitespace damaged.

Thanks for the info, I am now using your pre-commit hook.

There is still an odd hunk adding some whitespace.

As for the change itself, it makes sense to me, but I think
the "tmv_is_zero(tsp->filtered_delay))" check could prevent updates if
the measured and filtered delay is actually zero (which can happen for
various reasons). I'd suggest to define a special value for an unknown
delay (e.g. the minimum or maximum tmv_t value?), or add a new field
to be used as a flag.

[Burkhard Ilsen]

After the first SERVO_JUMP the following calls of
tsproc_update_offset() from clock_synchronize() fail
because t3 was reset.
But the offset calculation does not necessarily depend on t3,
i.e. when filtered_delay is available and neither raw nor weighting
is used.

Signed-off-by: Burkhard Ilsen <***@gmail.com>
---
servo.c | 2 +-
tsproc.c | 11 ++++++++---
2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/servo.c b/servo.c
index 8be4b92..809e65e 100644
--- a/servo.c
+++ b/servo.c
@@ -53,7 +53,7 @@ struct servo *servo_create(struct config *cfg, enum servo_type type,
}

if (!servo)
- return NULL;
+ return NULL;

servo_step_threshold = config_get_double(cfg, NULL, "step_threshold");
if (servo_step_threshold > 0.0) {
diff --git a/tsproc.c b/tsproc.c
index cf5f0dc..a9c1c48 100644
--- a/tsproc.c
+++ b/tsproc.c
@@ -163,12 +163,17 @@ int tsproc_update_offset(struct tsproc *tsp, tmv_t *offset, double *weight)
{
tmv_t delay, raw_delay = 0;

- if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2) ||
- tmv_is_zero(tsp->t3))
+ if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2))
return -1;

- if (tsp->raw_mode || tsp->weighting)
+ if (!tsp->raw_mode && tmv_is_zero(tsp->filtered_delay))
+ return -1;
+
+ if (tsp->raw_mode || tsp->weighting) {
+ if (tmv_is_zero(tsp->t3))
+ return -1;
raw_delay = get_raw_delay(tsp);
+ }

delay = tsp->raw_mode ? raw_delay : tsp->filtered_delay;

[Richard Cochran]

Post by Burkhard Ilsen
diff --git a/tsproc.c b/tsproc.c
index cf5f0dc..a9c1c48 100644
--- a/tsproc.c
+++ b/tsproc.c
@@ -163,12 +163,17 @@ int tsproc_update_offset(struct tsproc *tsp, tmv_t *offset, double *weight)
{
tmv_t delay, raw_delay = 0;
- if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2) ||
- tmv_is_zero(tsp->t3))
+ if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2))
return -1;
- if (tsp->raw_mode || tsp->weighting)
+ if (!tsp->raw_mode && tmv_is_zero(tsp->filtered_delay))
+ return -1;
+
+ if (tsp->raw_mode || tsp->weighting) {
+ if (tmv_is_zero(tsp->t3))
+ return -1;
raw_delay = get_raw_delay(tsp);
+ }

As Miroslav pointed out, 'filtered_delay' can legitimately be zero.
Instead of testing for zero, we need a new flag.

Also, the new code tests for raw_mode, !raw_mode, and raw_mode again.
This is becoming unreadable, and I started hacking around to make it
somehow clearer. I ended up with a whole new series which I'll post
as a RFC.

Thanks,
Richard

[Keller, Jacob E]

-----Original Message-----
Sent: Friday, April 07, 2017 3:30 PM
Subject: [Linuxptp-devel] [PATCH] tsproc_update_offset() fails when it should
succeed
After the first SERVO_JUMP the following calls of
tsproc_update_offset() from clock_synchronize() fail
because t3 was reset.
But the offset calculation does not necessarily depend on t3,
i.e. when filtered_delay is available and neither raw nor weighting
is used.
---
servo.c | 2 +-
tsproc.c | 11 ++++++++---
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/servo.c b/servo.c
index 8be4b92..809e65e 100644
--- a/servo.c
+++ b/servo.c
@@ -53,7 +53,7 @@ struct servo *servo_create(struct config *cfg, enum servo_type type,
}
if (!servo)
- return NULL;
+ return NULL;
servo_step_threshold = config_get_double(cfg, NULL,
"step_threshold");
if (servo_step_threshold > 0.0) {
diff --git a/tsproc.c b/tsproc.c
index cf5f0dc..a9c1c48 100644
--- a/tsproc.c
+++ b/tsproc.c
@@ -163,12 +163,17 @@ int tsproc_update_offset(struct tsproc *tsp, tmv_t
*offset, double *weight)
{
tmv_t delay, raw_delay = 0;
- if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2) ||
- tmv_is_zero(tsp->t3))
+ if (tmv_is_zero(tsp->t1) || tmv_is_zero(tsp->t2))
return -1;
- if (tsp->raw_mode || tsp->weighting)
+ if (!tsp->raw_mode && tmv_is_zero(tsp->filtered_delay))
+ return -1;
+

So t1, t2, t3, and t4 usually can't be zero, so using zero as an "empty" value makes sense here. However, as Miroslav said, filtered_delay could theoretically be zero, so we might either not need a value or we need to use some other method for determining if it's valid. A flag could work? Or maybe we don't even need this check at all? It wasn't there in the original code. What is the goal of this check, what are we trying to prevent here? Just ensure that we don't use the initial filterd_delay until after we've calculated it at least once?

Thanks,
Jake

+ if (tsp->raw_mode || tsp->weighting) {
+ if (tmv_is_zero(tsp->t3))
+ return -1;
raw_delay = get_raw_delay(tsp);
+ }
delay = tsp->raw_mode ? raw_delay : tsp->filtered_delay;
--
2.12.2.windows.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel