[Linuxptp-devel] [PATCH 0/3] timemaster improvements

[Miroslav Lichvar]

Here are some small improvements for the timemaster program I had in
my todo list. The first extends possibilities for configuration of
individual PTP/NTP sources, the second is useful to avoid conflicts
with time sources that are not started by timemaster (e.g. gpsd), and
the third one should allow timemaster to continue when a PTP source
fails due to a crash or unexpected termination of ptp4l or phc2sys.

Miroslav Lichvar (3):
timemaster: allow arbitrary options in server/refclock directives.
timemaster: add option to specify first SHM segment.
timemaster: ignore failures of non-essential processes.

timemaster.8 | 22 +++++++++++++
timemaster.c | 106 ++++++++++++++++++++++++++++++++++++++++-------------------
2 files changed, 94 insertions(+), 34 deletions(-)

[Miroslav Lichvar]

Instead of trying to support all options of the server and refclock
directives in both NTP implementations, add an "ntp_options" option
which specifies a string that is added directly to the lines in the
chronyd/ntpd configuration file.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
timemaster.8 | 14 ++++++++++++++
timemaster.c | 61 +++++++++++++++++++++++++++++++++++-------------------------
2 files changed, 50 insertions(+), 25 deletions(-)

diff --git a/timemaster.8 b/timemaster.8
index 4da4299..5987d26 100644
--- a/timemaster.8
+++ b/timemaster.8
@@ -103,6 +103,12 @@ Enable or disable sending a burst of NTP packets on start to speed up the
initial synchronization. Possible values are 1 and 0. The default value is 0
(disabled).

+.TP
+.B ntp_options
+Specify extra options that should be added for this source to the \fBserver\fR
+directive in the configuration file of the selected NTP implementation. No
+extra options are added by default.
+
.SS [ptp_domain number]

The \fBptp_domain\fR section specifies a PTP domain that should be used as a
@@ -143,6 +149,12 @@ microseconds). With \fBntpd\fR, the \fBtos mindist\fR command can be used to
set a limit with similar purpose globally for all time sources.

.TP
+.B ntp_options
+Specify extra options that should be added for this source to the
+\fBrefclock\fR or \fBserver\fR directives in the configuration file of the
+selected NTP implementation. No extra options are added by default.
+
+.TP
.B ptp4l_option
Specify an extra \fBptp4l\fR option specific to this PTP domain that should be
added to the configuration files generated for \fBptp4l\fR. This option may be
@@ -284,12 +296,14 @@ A more complex example using all \fBtimemaster\fR options would be:
minpoll 3
maxpoll 4
iburst 1
+ntp_options key 12

[ptp_domain 0]
interfaces eth0 eth1
ntp_poll 0
phc2sys_poll \-2
delay 10e\-6
+ntp_options prefer
ptp4l_option clock_servo linreg
ptp4l_option delay_mechanism P2P

diff --git a/timemaster.c b/timemaster.c
index 9c71b42..53f4275 100644
--- a/timemaster.c
+++ b/timemaster.c
@@ -72,6 +72,7 @@ struct ntp_server {
int minpoll;
int maxpoll;
int iburst;
+ char *ntp_options;
};

struct ptp_domain {
@@ -81,6 +82,7 @@ struct ptp_domain {
double delay;
char **interfaces;
char **ptp4l_settings;
+ char *ntp_options;
};

struct source {
@@ -223,10 +225,12 @@ static void source_destroy(struct source *source)
switch (source->type) {
case NTP_SERVER:
free(source->ntp.address);
+ free(source->ntp.ntp_options);
break;
case PTP_DOMAIN:
free_parray((void **)source->ptp.interfaces);
free_parray((void **)source->ptp.ptp4l_settings);
+ free(source->ptp.ntp_options);
break;
}
free(source);
@@ -235,7 +239,6 @@ static void source_destroy(struct source *source)
static struct source *source_ntp_parse(char *parameter, char **settings)
{
char *name, *value;
- struct ntp_server ntp_server;
struct source *source;
int r = 0;

@@ -244,35 +247,38 @@ static struct source *source_ntp_parse(char *parameter, char **settings)
return NULL;
}

- ntp_server.address = parameter;
- ntp_server.minpoll = DEFAULT_NTP_MINPOLL;
- ntp_server.maxpoll = DEFAULT_NTP_MAXPOLL;
- ntp_server.iburst = 0;
+ source = xmalloc(sizeof(*source));
+ source->type = NTP_SERVER;
+ source->ntp.address = xstrdup(parameter);
+ source->ntp.minpoll = DEFAULT_NTP_MINPOLL;
+ source->ntp.maxpoll = DEFAULT_NTP_MAXPOLL;
+ source->ntp.iburst = 0;
+ source->ntp.ntp_options = xstrdup("");

for (; *settings; settings++) {
parse_setting(*settings, &name, &value);
if (!strcasecmp(name, "minpoll")) {
- r = parse_int(value, &ntp_server.minpoll);
+ r = parse_int(value, &source->ntp.minpoll);
} else if (!strcasecmp(name, "maxpoll")) {
- r = parse_int(value, &ntp_server.maxpoll);
+ r = parse_int(value, &source->ntp.maxpoll);
} else if (!strcasecmp(name, "iburst")) {
- r = parse_bool(value, &ntp_server.iburst);
+ r = parse_bool(value, &source->ntp.iburst);
+ } else if (!strcasecmp(name, "ntp_options")) {
+ replace_string(value, &source->ntp.ntp_options);
} else {
pr_err("unknown ntp_server setting %s", name);
- return NULL;
+ goto failed;
}
if (r) {
pr_err("invalid value %s for %s", value, name);
- return NULL;
+ goto failed;
}
}

- source = xmalloc(sizeof(*source));
- source->type = NTP_SERVER;
- source->ntp = ntp_server;
- source->ntp.address = xstrdup(source->ntp.address);
-
return source;
+failed:
+ source_destroy(source);
+ return NULL;
}

static struct source *source_ptp_parse(char *parameter, char **settings)
@@ -288,6 +294,7 @@ static struct source *source_ptp_parse(char *parameter, char **settings)
source->ptp.phc2sys_poll = DEFAULT_PTP_PHC2SYS_POLL;
source->ptp.interfaces = (char **)parray_new();
source->ptp.ptp4l_settings = (char **)parray_new();
+ source->ptp.ntp_options = xstrdup("");

if (parse_int(parameter, &source->ptp.domain)) {
pr_err("invalid ptp_domain number %s", parameter);
@@ -305,6 +312,8 @@ static struct source *source_ptp_parse(char *parameter, char **settings)
} else if (!strcasecmp(name, "ptp4l_option")) {
parray_append((void ***)&source->ptp.ptp4l_settings,
xstrdup(value));
+ } else if (!strcasecmp(name, "ntp_options")) {
+ replace_string(value, &source->ptp.ntp_options);
} else if (!strcasecmp(name, "interfaces")) {
parse_words(value, &source->ptp.interfaces);
} else {
@@ -609,8 +618,8 @@ static char *get_refid(char *prefix, unsigned int number)
};

static void add_shm_source(int shm_segment, int poll, int dpoll, double delay,
- char *prefix, struct timemaster_config *config,
- char **ntp_config)
+ char *ntp_options, char *prefix,
+ struct timemaster_config *config, char **ntp_config)
{
char *refid = get_refid(prefix, shm_segment);

@@ -618,15 +627,17 @@ static void add_shm_source(int shm_segment, int poll, int dpoll, double delay,
case CHRONYD:
string_appendf(ntp_config,
"refclock SHM %d poll %d dpoll %d "
- "refid %s precision 1.0e-9 delay %.1e\n",
- shm_segment, poll, dpoll, refid, delay);
+ "refid %s precision 1.0e-9 delay %.1e %s\n",
+ shm_segment, poll, dpoll, refid, delay,
+ ntp_options);
break;
case NTPD:
string_appendf(ntp_config,
"server 127.127.28.%d minpoll %d maxpoll %d "
- "mode 1\n"
+ "mode 1 %s\n"
"fudge 127.127.28.%d refid %s\n",
- shm_segment, poll, poll, shm_segment, refid);
+ shm_segment, poll, poll, ntp_options,
+ shm_segment, refid);
break;
}

@@ -637,9 +648,9 @@ static int add_ntp_source(struct ntp_server *source, char **ntp_config)
{
pr_debug("adding NTP server %s", source->address);

- string_appendf(ntp_config, "server %s minpoll %d maxpoll %d%s\n",
+ string_appendf(ntp_config, "server %s minpoll %d maxpoll %d %s %s\n",
source->address, source->minpoll, source->maxpoll,
- source->iburst ? " iburst" : "");
+ source->iburst ? "iburst" : "", source->ntp_options);
return 0;
}

@@ -766,8 +777,8 @@ static int add_ptp_source(struct ptp_domain *source,
parray_append((void ***)&script->configs, config_file);

add_shm_source(*shm_segment, source->ntp_poll,
- source->phc2sys_poll, source->delay, "PTP",
- config, ntp_config);
+ source->phc2sys_poll, source->delay,
+ source->ntp_options, "PTP", config, ntp_config);

(*shm_segment)++;

[Miroslav Lichvar]

This allows using a sequence of SHM segments that starts with a number
larger than zero, which can be useful to avoid conflicts with time
sources that are not started by timemaster, e.g. gpsd using segments
number 0 and 1.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
timemaster.8 | 8 ++++++++
timemaster.c | 14 +++++++++++++-
2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/timemaster.8 b/timemaster.8
index 5987d26..e0e22eb 100644
--- a/timemaster.8
+++ b/timemaster.8
@@ -80,6 +80,13 @@ Specify the directory where should be generated \fBchronyd\fR, \fBntpd\fR and
\fBptp4l\fR configuration files and sockets. The directory will be created if
it doesn't exist. The default value is \fB/var/run/timemaster\fR.

+.TP
+.B first_shm_segment
+Specify the first number in a sequence of SHM segments that will be used by
+\fBptp4l\fR and \fBphc2sys\fR. The default value is 0. Increasing the number
+can be useful to avoid conflicts with time sources that are not started by
+\fBtimemaster\fR, e.g. \fBgpsd\fR using segments number 0 and 1.
+
.SS [ntp_server address]

The \fBntp_server\fR section specifies an NTP server that should be used as a
@@ -310,6 +317,7 @@ ptp4l_option delay_mechanism P2P
[timemaster]
ntp_program chronyd
rundir /var/run/timemaster
+first_shm_segment 1

[chronyd]
path /usr/sbin/chronyd
diff --git a/timemaster.c b/timemaster.c
index 53f4275..7a22a8b 100644
--- a/timemaster.c
+++ b/timemaster.c
@@ -41,6 +41,8 @@

#define DEFAULT_RUNDIR "/var/run/timemaster"

+#define DEFAULT_FIRST_SHM_SEGMENT 0
+
#define DEFAULT_NTP_PROGRAM CHRONYD
#define DEFAULT_NTP_MINPOLL 6
#define DEFAULT_NTP_MAXPOLL 10
@@ -103,6 +105,7 @@ struct timemaster_config {
struct source **sources;
enum ntp_program ntp_program;
char *rundir;
+ int first_shm_segment;
struct program_config chronyd;
struct program_config ntpd;
struct program_config phc2sys;
@@ -363,6 +366,7 @@ static int parse_timemaster_settings(char **settings,
struct timemaster_config *config)
{
char *name, *value;
+ int r = 0;

for (; *settings; settings++) {
parse_setting(*settings, &name, &value);
@@ -377,10 +381,16 @@ static int parse_timemaster_settings(char **settings,
}
} else if (!strcasecmp(name, "rundir")) {
replace_string(value, &config->rundir);
+ } else if (!strcasecmp(name, "first_shm_segment")) {
+ r = parse_int(value, &config->first_shm_segment);
} else {
pr_err("unknown timemaster setting %s", name);
return 1;
}
+ if (r) {
+ pr_err("invalid value %s for %s", value, name);
+ return 1;
+ }
}

return 0;
@@ -495,6 +505,7 @@ static struct timemaster_config *config_parse(char *path)
config->sources = (struct source **)parray_new();
config->ntp_program = DEFAULT_NTP_PROGRAM;
config->rundir = xstrdup(DEFAULT_RUNDIR);
+ config->first_shm_segment = DEFAULT_FIRST_SHM_SEGMENT;

init_program_config(&config->chronyd, "chronyd",
NULL, DEFAULT_CHRONYD_SETTINGS, NULL);
@@ -876,12 +887,13 @@ static struct script *script_create(struct timemaster_config *config)
struct source *source, **sources;
struct config_file *ntp_config = NULL;
int **allocated_phcs = (int **)parray_new();
- int ret = 0, shm_segment = 0;
+ int ret = 0, shm_segment;

script->configs = (struct config_file **)parray_new();
script->commands = (char ***)parray_new();

ntp_config = add_ntp_program(config, script);
+ shm_segment = config->first_shm_segment;

for (sources = config->sources; (source = *sources); sources++) {
switch (source->type) {

[Miroslav Lichvar]

Assume only the chronyd or ntpd process is essential for synchronization
of the system clock and ignore SIGCHLD from other processes. This should
provide resiliency against possible bugs in ptp4l or phc2sys that can
terminate the processes.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
timemaster.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/timemaster.c b/timemaster.c
index 7a22a8b..66ac521 100644
--- a/timemaster.c
+++ b/timemaster.c
@@ -1035,6 +1035,14 @@ static int script_run(struct script *script)
pid_t pid, *pids;
int i, num_commands, status, ret = 0;

+ for (num_commands = 0; script->commands[num_commands]; num_commands++)
+ ;
+
+ if (!num_commands) {
+ /* nothing to do */
+ return 0;
+ }
+
if (create_config_files(script->configs))
return 1;

@@ -1050,9 +1058,6 @@ static int script_run(struct script *script)
return 1;
}

- for (num_commands = 0; script->commands[num_commands]; num_commands++)
- ;
-
pids = xcalloc(num_commands, sizeof(*pids));

for (i = 0; i < num_commands; i++) {
@@ -1065,15 +1070,25 @@ static int script_run(struct script *script)

/* wait for one of the blocked signals */
while (1) {
- if (sigwaitinfo(&mask, &info) > 0)
- break;
- if (errno != EINTR) {
+ if (sigwaitinfo(&mask, &info) < 0) {
+ if (errno == EINTR)
+ continue;
pr_err("sigwaitinfo() failed: %m");
break;
}
- }

- pr_info("received signal %d", info.si_signo);
+ /*
+ * assume only the first process (i.e. chronyd or ntpd) is
+ * essential and continue if other processes terminate
+ */
+ if (info.si_signo == SIGCHLD && info.si_pid != pids[0]) {
+ pr_info("process %d terminated (ignored)", info.si_pid);
+ continue;
+ }
+
+ pr_info("received signal %d", info.si_signo);
+ break;
+ }

/* kill all started processes */
for (i = 0; i < num_commands; i++) {

[Richard Cochran]

Post by Miroslav Lichvar
Here are some small improvements for the timemaster program I had in
my todo list. The first extends possibilities for configuration of
individual PTP/NTP sources, the second is useful to avoid conflicts
with time sources that are not started by timemaster (e.g. gpsd), and
the third one should allow timemaster to continue when a PTP source
fails due to a crash or unexpected termination of ptp4l or phc2sys.
timemaster: allow arbitrary options in server/refclock directives.
timemaster: add option to specify first SHM segment.
timemaster: ignore failures of non-essential processes.

Series applied. If you don't mind, this will go into v1.7 as well.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
timemaster: allow arbitrary options in server/refclock directives.
timemaster: add option to specify first SHM segment.
timemaster: ignore failures of non-essential processes.

Series applied. If you don't mind, this will go into v1.7 as well.

Great, thanks! I was hoping it wouldn't be too late for that :).