[Linuxptp-devel] [PATCH v2 2/2] Make fault_reset_seconds a per-port configuration parameter

[Delio Brignoli]

A timeout of 15 seconds is not always acceptable, make it configurable.

Signed-off-by: Delio Brignoli <***@audioscience.com>
---
clock.c | 15 +++++++++------
config.c | 5 +++++
default.cfg | 1 +
ds.h | 1 +
gPTP.cfg | 1 +
ptp4l.8 | 5 +++++
ptp4l.c | 1 +
7 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/clock.c b/clock.c
index 5d5ccbf..b2f40e6 100644
--- a/clock.c
+++ b/clock.c
@@ -37,7 +37,6 @@
#include "util.h"

#define CLK_N_PORTS (MAX_PORTS + 1) /* plus one for the UDS interface */
-#define FAULT_RESET_SECONDS 15
#define N_CLOCK_PFD (N_POLLFD + 1) /* one extra per port, for the fault timer */
#define MAVE_LENGTH 10
#define POW2_41 ((double)(1ULL << 41))
@@ -65,7 +64,7 @@ struct clock {
struct port *port[CLK_N_PORTS];
struct pollfd pollfd[CLK_N_PORTS*N_CLOCK_PFD];
int fault_fd[CLK_N_PORTS];
- time_t fault_timeout;
+ time_t fault_timeout[CLK_N_PORTS];
int nports; /* does not include the UDS port */
int free_running;
int freq_est_interval;
@@ -115,8 +114,8 @@ static int clock_fault_timeout(struct clock *c, int index, int set)
};
if (set) {
pr_debug("waiting %d seconds to clear fault on port %d",
- c->fault_timeout, index);
- tmo.it_value.tv_sec = c->fault_timeout;
+ c->fault_timeout[index], index);
+ tmo.it_value.tv_sec = c->fault_timeout[index];
} else {
pr_debug("clearing fault on port %d", index);
}
@@ -479,10 +478,10 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
c->pollfd[i].events = 0;
}

- c->fault_timeout = FAULT_RESET_SECONDS;
c->fest.max_count = 2;

for (i = 0; i < count; i++) {
+ c->fault_timeout[i] = iface[i].pod.fault_reset_seconds;
c->port[i] = port_open(phc_index, timestamping, 1+i, &iface[i], c);
if (!c->port[i]) {
pr_err("failed to open port %s", iface[i].name);
@@ -721,7 +720,11 @@ int clock_poll(struct clock *c)
port_dispatch(c->port[i], event, 0);
/* Clear any fault after a little while. */
if (PS_FAULTY == port_state(c->port[i])) {
- clock_fault_timeout(c, i, 1);
+ if (c->fault_timeout[i] == 0)
+ port_dispatch(c->port[i],
+ EV_FAULT_CLEARED, 0);
+ else
+ clock_fault_timeout(c, i, 1);
}
}
}
diff --git a/config.c b/config.c
index a4a6261..379733f 100644
--- a/config.c
+++ b/config.c
@@ -106,6 +106,11 @@ static enum parser_result parse_pod_setting(const char *option,
return BAD_VALUE;
pod->follow_up_info = val ? 1 : 0;

+ } else if (!strcmp(option, "fault_reset_seconds")) {
+ if (1 != sscanf(value, "%u", &val))
+ return BAD_VALUE;
+ pod->fault_reset_seconds = val;
+
} else
return NOT_PARSED;

diff --git a/default.cfg b/default.cfg
index ee2700d..87ace08 100644
--- a/default.cfg
+++ b/default.cfg
@@ -21,6 +21,7 @@ logMinDelayReqInterval 0
logMinPdelayReqInterval 0
announceReceiptTimeout 3
delayAsymmetry 0
+fault_reset_seconds 15
#
# Run time options
#
diff --git a/ds.h b/ds.h
index 514679b..43cd4a6 100644
--- a/ds.h
+++ b/ds.h
@@ -112,6 +112,7 @@ struct port_defaults {
int path_trace_enabled;
int follow_up_info;
int freq_est_interval; /*log seconds*/
+ int fault_reset_seconds;
};

#endif
diff --git a/gPTP.cfg b/gPTP.cfg
index f186e78..90a55ac 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -20,6 +20,7 @@ logSyncInterval -3
logMinPdelayReqInterval 0
announceReceiptTimeout 3
delayAsymmetry 0
+fault_reset_seconds 15
#
# Run time options
#
diff --git a/ptp4l.8 b/ptp4l.8
index fadf854..791236c 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -181,6 +181,11 @@ The default is 0 (disabled).
Include the 802.1AS data in the Follow_Up messages if enabled.
The default is 0 (disabled).
.TP
+.B fault_reset_seconds
+The time in seconds between the detection of a port's fault and the fault
+being reset.
+The default is 15 (15 seconds).
+.TP
.B delay_mechanism
Select the delay mechanism. Possible values are E2E, P2P and Auto.
The default is E2E.
diff --git a/ptp4l.c b/ptp4l.c
index 4fc0c88..0ce6a5c 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -64,6 +64,7 @@ static struct config cfg_settings = {
.path_trace_enabled = 0,
.follow_up_info = 0,
.freq_est_interval = 1,
+ .fault_reset_seconds = 15,
},

.timestamping = TS_HARDWARE,

[Richard Cochran]

Post by Delio Brignoli
A timeout of 15 seconds is not always acceptable, make it configurable.

Having this configurable is a nice improvement.

One question though ...

Post by Delio Brignoli
@@ -721,7 +720,11 @@ int clock_poll(struct clock *c)
port_dispatch(c->port[i], event, 0);
/* Clear any fault after a little while. */
if (PS_FAULTY == port_state(c->port[i])) {
- clock_fault_timeout(c, i, 1);
+ if (c->fault_timeout[i] == 0)
+ port_dispatch(c->port[i],
+ EV_FAULT_CLEARED, 0);

You have a special case here. I guess you want to configure
fault_timeout to zero in order to be able to clear the fault ASAP.

Would it also work for you to let the timeout be less than one second
(say 1000 nanoseconds) and then clear the fault after the next call to
poll?

That would be nicer, to have just one place to clear the fault, I think.

Post by Delio Brignoli
+ else
+ clock_fault_timeout(c, i, 1);
}
}
}

Thanks,
Richard

[Keller, Jacob E]

-----Original Message-----
Sent: Wednesday, January 30, 2013 10:49 AM
To: Delio Brignoli
Subject: Re: [Linuxptp-devel] [PATCH v2 2/2] Make fault_reset_seconds
a per-port configuration parameter

Post by Delio Brignoli
A timeout of 15 seconds is not always acceptable, make it

configurable.
Having this configurable is a nice improvement.
One question though ...

Post by Delio Brignoli
@@ -721,7 +720,11 @@ int clock_poll(struct clock *c)
port_dispatch(c->port[i], event, 0);
/* Clear any fault after a little while. */
if (PS_FAULTY == port_state(c->port[i])) {
- clock_fault_timeout(c, i, 1);
+ if (c->fault_timeout[i] == 0)
+ port_dispatch(c->port[i],
+

EV_FAULT_CLEARED, 0);
You have a special case here. I guess you want to configure
fault_timeout to zero in order to be able to clear the fault ASAP.
Would it also work for you to let the timeout be less than one second
(say 1000 nanoseconds) and then clear the fault after the next call to
poll?
That would be nicer, to have just one place to clear the fault, I think.

Post by Delio Brignoli
+ else
+ clock_fault_timeout(c, i,

1);

Post by Delio Brignoli
}
}
}

Thanks,
Richard

I agree with richard. If the timeout is small enough it won't cause noticeable delay in restarting, and it would reduce code duplication which is good.

- Jake

[Delio Brignoli]

Hi Jakob and Richard,

Post by Keller, Jacob E

-----Original Message-----
Sent: Wednesday, January 30, 2013 10:49 AM
To: Delio Brignoli
Subject: Re: [Linuxptp-devel] [PATCH v2 2/2] Make fault_reset_seconds
a per-port configuration parameter

Post by Delio Brignoli
A timeout of 15 seconds is not always acceptable, make it

configurable.
Having this configurable is a nice improvement.
One question though ...

Post by Delio Brignoli
@@ -721,7 +720,11 @@ int clock_poll(struct clock *c)
port_dispatch(c->port[i], event, 0);
/* Clear any fault after a little while. */
if (PS_FAULTY == port_state(c->port[i])) {
- clock_fault_timeout(c, i, 1);
+ if (c->fault_timeout[i] == 0)
+ port_dispatch(c->port[i],
+

EV_FAULT_CLEARED, 0);
You have a special case here. I guess you want to configure
fault_timeout to zero in order to be able to clear the fault ASAP.
Would it also work for you to let the timeout be less than one second
(say 1000 nanoseconds) and then clear the fault after the next call to
poll?
That would be nicer, to have just one place to clear the fault, I think.

[...]

Post by Keller, Jacob E
I agree with richard. If the timeout is small enough it won't cause noticeable delay in restarting, and it would reduce code duplication which is good.

It's an extra call to port_dispatch() in the same clock_poll() function. Are function calls really code duplication?
The case when timeout is 0 (which I think is a legitimate case) has to be handled separately because a timeout of 0 dis-arms the timer and I do not see a simple way around that.
Also, I need to have the option for linuxptp not to sleep (not even for a little while) when a fault is detected because of strict timing requirements.

I was wondering what people prefer: a linear integer number of seconds for the config option or log_2 seconds like other 1588/AS options (with -127 interpreted as 'do not wait').
Richard, if people are happy with the current format for the config option, can you please apply?

Thanks
--
Delio

[Miroslav Lichvar]

Post by Delio Brignoli

Subject: Re: [Linuxptp-devel] [PATCH v2 2/2] Make fault_reset_seconds
a per-port configuration parameter

I was wondering what people prefer: a linear integer number of
seconds for the config option or log_2 seconds like other 1588/AS
options (with -127 interpreted as 'do not wait').

I'd prefer the log scale, it would be consistent with the other
options. The name could be "fault_reset_interval".

Thanks,

[Richard Cochran]

Post by Delio Brignoli
I was wondering what people prefer: a linear integer number of seconds for the config option or log_2 seconds like other 1588/AS options (with -127 interpreted as 'do not wait').
Richard, if people are happy with the current format for the config option, can you please apply?

I still would like to handle the special case of "don't wait" in
another way. Let me see what I can come up with...

Thanks,
Richard

[Richard Cochran]

Post by Richard Cochran

Post by Delio Brignoli
I was wondering what people prefer: a linear integer number of seconds for the config option or log_2 seconds like other 1588/AS options (with -127 interpreted as 'do not wait').
Richard, if people are happy with the current format for the config option, can you please apply?

I still would like to handle the special case of "don't wait" in
another way. Let me see what I can come up with...

Well, that was easy. I hope this will make everyone happy. I had an
intuition that there was a better way, and lo, we already have the
exception block in the port code.

Delio, if it is okay with you, I would like to squash the patch,
below, onto your patch #2.

I don't mind leaving the timeout in whole seconds as it is (but I am
not opposed to a log value either). Is having a log timeout really
worth reworking these two patches?

BTW, I fix another latent bug by adding the break statement. Since the
port closes all its fds after a fault, if a fault occurs at the same
as, say, an incoming packet, then the port would try to read from an
invalid socket.

Thanks,
Richard

---
diff --git a/clock.c b/clock.c
index b2f40e6..880effc 100644
--- a/clock.c
+++ b/clock.c
@@ -720,11 +720,8 @@ int clock_poll(struct clock *c)
port_dispatch(c->port[i], event, 0);
/* Clear any fault after a little while. */
if (PS_FAULTY == port_state(c->port[i])) {
- if (c->fault_timeout[i] == 0)
- port_dispatch(c->port[i],
- EV_FAULT_CLEARED, 0);
- else
- clock_fault_timeout(c, i, 1);
+ clock_fault_timeout(c, i, 1);
+ break;
}
}
}
diff --git a/port.c b/port.c
index 8b03bd9..c486812 100644
--- a/port.c
+++ b/port.c
@@ -1419,7 +1419,8 @@ void port_dispatch(struct port *p, enum fsm_event event, int mdiff)
next = ptp_fsm(p->state, event, mdiff);
}

- if (PS_INITIALIZING == next) {
+ if (PS_INITIALIZING == next ||
+ (PS_FAULTY == next && 0 == p->pod.fault_reset_seconds)) {
/*
* This is a special case. Since we initialize the
* port immediately, we can skip right to listening

[Delio Brignoli]

Hello Richard,

Post by Richard Cochran

Post by Richard Cochran

Post by Delio Brignoli
I was wondering what people prefer: a linear integer number of seconds for the config option or log_2 seconds like other 1588/AS options (with -127 interpreted as 'do not wait').
Richard, if people are happy with the current format for the config option, can you please apply?

I still would like to handle the special case of "don't wait" in
another way. Let me see what I can come up with...

Well, that was easy. I hope this will make everyone happy. I had an
intuition that there was a better way, and lo, we already have the
exception block in the port code.
Delio, if it is okay with you, I would like to squash the patch,
below, onto your patch #2.

yes, go ahead with squashing the two patches.... I see you already did in the new patch series, good.

Post by Richard Cochran
I don't mind leaving the timeout in whole seconds as it is (but I am
not opposed to a log value either). Is having a log timeout really
worth reworking these two patches?

I lean towards the log timeout and name change but I will not be able to work on this until mid-week next week. So I'd say apply it and a later patch can effect the conversion to log seconds. It also gives people time to comment on their preferences.

Post by Richard Cochran
BTW, I fix another latent bug by adding the break statement. Since the
port closes all its fds after a fault, if a fault occurs at the same
as, say, an incoming packet, then the port would try to read from an
invalid socket.

Thanks!
--
Delio

Post by Richard Cochran
Thanks,
Richard
---
diff --git a/clock.c b/clock.c
index b2f40e6..880effc 100644
--- a/clock.c
+++ b/clock.c
@@ -720,11 +720,8 @@ int clock_poll(struct clock *c)
port_dispatch(c->port[i], event, 0);
/* Clear any fault after a little while. */
if (PS_FAULTY == port_state(c->port[i])) {
- if (c->fault_timeout[i] == 0)
- port_dispatch(c->port[i],
- EV_FAULT_CLEARED, 0);
- else
- clock_fault_timeout(c, i, 1);
+ clock_fault_timeout(c, i, 1);
+ break;
}
}
}
diff --git a/port.c b/port.c
index 8b03bd9..c486812 100644
--- a/port.c
+++ b/port.c
@@ -1419,7 +1419,8 @@ void port_dispatch(struct port *p, enum fsm_event event, int mdiff)
next = ptp_fsm(p->state, event, mdiff);
}
- if (PS_INITIALIZING == next) {
+ if (PS_INITIALIZING == next ||
+ (PS_FAULTY == next && 0 == p->pod.fault_reset_seconds)) {
/*
* This is a special case. Since we initialize the
* port immediately, we can skip right to listening

[Richard Cochran]

Post by Delio Brignoli
I lean towards the log timeout and name change but I will not be able to work on this until mid-week next week. So I'd say apply it and a later patch can effect the conversion to log seconds. It also gives people time to comment on their preferences.

I did the change to log seconds and rebased, combining the change with
your original patch (which is thus mostly rewritten). Please take a
look at "v4" and let me know what you think.

Thanks,
Richard

[Delio Brignoli]

Hello Richard,

Thanks for doing this. Given the patch is mostly rewritten, feel free to remove my signed-off-by and change it into a acked-by.

--
Delio

Post by Richard Cochran

Post by Delio Brignoli
I lean towards the log timeout and name change but I will not be able to work on this until mid-week next week. So I'd say apply it and a later patch can effect the conversion to log seconds. It also gives people time to comment on their preferences.

I did the change to log seconds and rebased, combining the change with
your original patch (which is thus mostly rewritten). Please take a
look at "v4" and let me know what you think.
Thanks,
Richard

[Richard Cochran]

Post by Delio Brignoli
Hello Richard,
Thanks for doing this. Given the patch is mostly rewritten, feel free to remove my signed-off-by and change it into a acked-by.

Oh well, too late - you got the credit ;)

It was your idea anyhow.

Thanks,
Richard