Discussion:
[Linuxptp-devel] [PATCH RFC 0/7] Link state tracking
Richard Cochran
2016-07-31 21:48:33 UTC
Permalink
This series implements link state tracking over all port via a single
RT netlink socket. Unfortunately this ends up being a fair amount of
code simply because the netlink API does not allow selective per-port
tracking. We work around this limitation by de-multiplexing the
notifications from a single socket in the clock module.

Patches 1 and 2 add generic rtnl code started long ago.
Patches 3 and 4 fix coding style I noticed in passing.
Patches 5-7 implement the link state tracking.

Comments and criticism are most welcome!

Thanks,
Richard


Richard Cochran (7):
rtnl: Introduce RT netlink sockets.
sk: Add a method to obtain a socket for utility purposes.
clock: Remove stray semicolon.
clock: Fix coding style within a helper function.
clock: Remember each port's interface index.
port: Provide methods to set and get the link status.
clock: Monitor the link status using a RT netlink socket.

clock.c | 104 ++++++++++++++++++++++++++++++++++-------
makefile | 2 +-
port.c | 13 ++++++
port.h | 14 ++++++
rtnl.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
rtnl.h | 54 +++++++++++++++++++++
sk.c | 10 ++++
sk.h | 6 +++
8 files changed, 347 insertions(+), 17 deletions(-)
create mode 100644 rtnl.c
create mode 100644 rtnl.h
--
2.1.4


------------------------------------------------------------------------------
Richard Cochran
2016-07-31 21:48:36 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clock.c b/clock.c
index 0107ef2..aaff688 100644
--- a/clock.c
+++ b/clock.c
@@ -314,7 +314,7 @@ static void clock_freq_est_reset(struct clock *c)
c->fest.origin1 = tmv_zero();
c->fest.ingress1 = tmv_zero();
c->fest.count = 0;
-};
+}

static void clock_management_send_error(struct port *p,
struct ptp_message *msg, int error_id)
--
2.1.4


------------------------------------------------------------------------------
Richard Cochran
2016-07-31 21:48:34 UTC
Permalink
This patch adds a source module that implements RT netlink sockets
for the purpose of link monitoring. Unfortunately the netlink API
offers no possibility for per-port notification. Instead it
forces us to use a de-multiplexing pattern.

Signed-off-by: Richard Cochran <***@gmail.com>
---
makefile | 2 +-
rtnl.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
rtnl.h | 54 +++++++++++++++++++++
3 files changed, 216 insertions(+), 1 deletion(-)
create mode 100644 rtnl.c
create mode 100644 rtnl.h

diff --git a/makefile b/makefile
index bcdbb92..44be067 100644
--- a/makefile
+++ b/makefile
@@ -25,7 +25,7 @@ LDLIBS = -lm -lrt $(EXTRA_LDFLAGS)
PRG = ptp4l pmc phc2sys hwstamp_ctl phc_ctl timemaster
OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o fault.o \
filter.o fsm.o hash.o linreg.o mave.o mmedian.o msg.o ntpshm.o nullf.o phc.o \
- pi.o port.o print.o ptp4l.o raw.o servo.o sk.o stats.o tlv.o \
+ pi.o port.o print.o ptp4l.o raw.o rtnl.o servo.o sk.o stats.o tlv.o \
transport.o tsproc.o udp.o udp6.o uds.o util.o version.o

OBJECTS = $(OBJ) hwstamp_ctl.o phc2sys.o phc_ctl.o pmc.o pmc_common.o \
diff --git a/rtnl.c b/rtnl.c
new file mode 100644
index 0000000..cf11835
--- /dev/null
+++ b/rtnl.c
@@ -0,0 +1,161 @@
+/**
+ * @file rtnl.c
+ * @note Copyright (C) 2012 Richard Cochran <***@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#include <asm/types.h>
+#include <linux/netlink.h>
+#include <linux/rtnetlink.h>
+#include <net/if.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+#include "print.h"
+#include "rtnl.h"
+
+static int rtnl_len;
+static char *rtnl_buf;
+
+int rtnl_close(int fd)
+{
+ return close(fd);
+}
+
+int rtnl_link_query(int fd)
+{
+ struct sockaddr_nl sa;
+ struct msghdr msg;
+ struct iovec iov;
+ int cnt;
+
+ struct {
+ struct nlmsghdr hdr;
+ struct rtgenmsg gen;
+ } __attribute__((packed)) request;
+
+ memset(&sa, 0, sizeof(sa));
+ sa.nl_family = AF_NETLINK;
+
+ memset(&request, 0, sizeof(request));
+ request.hdr.nlmsg_len = NLMSG_LENGTH(sizeof(request.gen));
+ request.hdr.nlmsg_type = RTM_GETLINK;
+ request.hdr.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP;
+ request.hdr.nlmsg_seq = 1;
+ request.hdr.nlmsg_pid = 0;
+ request.gen.rtgen_family = AF_UNSPEC;
+
+ iov.iov_base = &request;
+ iov.iov_len = sizeof(request);
+
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_name = &sa;
+ msg.msg_namelen = sizeof(sa);
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+
+ cnt = sendmsg(fd, &msg, 0);
+ if (cnt < 0) {
+ pr_err("rtnl: sendmsg: %m");
+ return -1;
+ }
+ return 0;
+}
+
+int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
+{
+ int index, len;
+ struct iovec iov;
+ struct sockaddr_nl sa;
+ struct msghdr msg;
+ struct nlmsghdr *nh;
+ struct ifinfomsg *info = NULL;
+
+ if (!rtnl_buf) {
+ rtnl_len = 4096;
+ rtnl_buf = malloc(rtnl_len);
+ if (!rtnl_buf) {
+ pr_err("rtnl: low memory");
+ return -1;
+ }
+ }
+
+ iov.iov_base = rtnl_buf;
+ iov.iov_len = rtnl_len;
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_name = &sa;
+ msg.msg_namelen = sizeof(sa);
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+
+ len = recvmsg(fd, &msg, MSG_PEEK | MSG_TRUNC);
+ if (len < 1) {
+ pr_err("rtnl: recvmsg: %m");
+ return -1;
+ }
+ if (len > rtnl_len) {
+ free(rtnl_buf);
+ rtnl_len = len;
+ rtnl_buf = malloc(len);
+ if (!rtnl_buf) {
+ pr_err("rtnl: failed to resize to %d bytes", len);
+ return -1;
+ }
+ iov.iov_base = rtnl_buf;
+ iov.iov_len = rtnl_len;
+ }
+
+ len = recvmsg(fd, &msg, 0);
+ if (len < 1) {
+ pr_err("rtnl: recvmsg: %m");
+ return -1;
+ }
+ nh = (struct nlmsghdr *) rtnl_buf;
+
+ for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
+ if (nh->nlmsg_type == RTM_NEWLINK) {
+ info = NLMSG_DATA(nh);
+ index = info->ifi_index;
+ pr_debug("interface index %d is %s", index,
+ info->ifi_flags & IFF_RUNNING ? "up" : "down");
+ cb(ctx, index, info->ifi_flags & IFF_RUNNING ? 1 : 0);
+ }
+ }
+ return 0;
+}
+
+int rtnl_open(void)
+{
+ int fd;
+ struct sockaddr_nl sa;
+
+ memset(&sa, 0, sizeof(sa));
+ sa.nl_family = AF_NETLINK;
+ sa.nl_groups = RTMGRP_LINK;
+
+ fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
+ if (fd < 0) {
+ pr_err("failed to open netlink socket: %m");
+ return -1;
+ }
+ if (bind(fd, (struct sockaddr *) &sa, sizeof(sa))) {
+ pr_err("failed to bind netlink socket: %m");
+ return -1;
+ }
+ return fd;
+}
diff --git a/rtnl.h b/rtnl.h
new file mode 100644
index 0000000..f1871f2
--- /dev/null
+++ b/rtnl.h
@@ -0,0 +1,54 @@
+/**
+ * @file rtnl.h
+ * @brief Interface link status via RT netlink
+ * @note Copyright (C) 2012 Richard Cochran <***@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#ifndef HAVE_RTNL_H
+#define HAVE_RTNL_H
+
+typedef void (*rtnl_callback)(void *ctx, int index, int linkup);
+
+/**
+ * Close a RT netlink socket.
+ * @param fd A socket obtained via rtnl_open().
+ * @return Zero on success, non-zero otherwise.
+ */
+int rtnl_close(int fd);
+
+/**
+ * Request the link status from the kernel.
+ * @param fd A socket obtained via rtnl_open().
+ * @return Zero on success, non-zero otherwise.
+ */
+int rtnl_link_query(int fd);
+
+/**
+ * Read kernel messages looking for a link up/down events.
+ * @param fd Readable socket obtained via rtnl_open().
+ * @param cb Callback function to be invoked on each event.
+ * @param ctx Private context passed to the callback.
+ * @return Zero on success, non-zero otherwise.
+ */
+int rtnl_link_status(int fd, rtnl_callback cb, void *ctx);
+
+/**
+ * Open a RT netlink socket for monitoring link state.
+ * @return A valid socket, or -1 on error.
+ */
+int rtnl_open(void);
+
+#endif
--
2.1.4


------------------------------------------------------------------------------
Richard Cochran
2016-07-31 21:48:37 UTC
Permalink
No functional changes.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/clock.c b/clock.c
index aaff688..7b0d923 100644
--- a/clock.c
+++ b/clock.c
@@ -768,20 +768,22 @@ static int clock_add_port(struct clock *c, int phc_index,
{
struct port *p, *piter, *lastp = NULL;

- if (clock_resize_pollfd(c, c->nports + 1))
+ if (clock_resize_pollfd(c, c->nports + 1)) {
return -1;
- p = port_open(phc_index, timestamping, ++c->last_port_number,
- iface, c);
+ }
+ p = port_open(phc_index, timestamping, ++c->last_port_number, iface, c);
if (!p) {
/* No need to shrink pollfd */
return -1;
}
- LIST_FOREACH(piter, &c->ports, list)
+ LIST_FOREACH(piter, &c->ports, list) {
lastp = piter;
- if (lastp)
+ }
+ if (lastp) {
LIST_INSERT_AFTER(lastp, p, list);
- else
+ } else {
LIST_INSERT_HEAD(&c->ports, p, list);
+ }
c->nports++;
clock_fda_changed(c);
return 0;
--
2.1.4


------------------------------------------------------------------------------
Richard Cochran
2016-07-31 21:48:35 UTC
Permalink
The clock module will want to know the interface indexes, in order to
implement link monitoring. However, the clock does not open any sockets
directly. This helper function lets us keep the clock module free of
socket level code.

Signed-off-by: Richard Cochran <***@gmail.com>
---
sk.c | 10 ++++++++++
sk.h | 6 ++++++
2 files changed, 16 insertions(+)

diff --git a/sk.c b/sk.c
index ad30f71..63ec206 100644
--- a/sk.c
+++ b/sk.c
@@ -80,6 +80,16 @@ static int hwts_init(int fd, const char *device, int rx_filter, int one_step)

/* public methods */

+int sk_interface_fd(void)
+{
+ int fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);
+ if (fd < 0) {
+ pr_err("socket failed: %m");
+ return -1;
+ }
+ return fd;
+}
+
int sk_interface_index(int fd, const char *name)
{
struct ifreq ifreq;
diff --git a/sk.h b/sk.h
index 31cc88e..d91d5d8 100644
--- a/sk.h
+++ b/sk.h
@@ -40,6 +40,12 @@ struct sk_ts_info {
};

/**
+ * Obtains a socket suitable for use with sk_interface_index().
+ * @return An open socket on success, -1 otherwise.
+ */
+int sk_interface_fd(void);
+
+/**
* Obtain the numerical index from a network interface by name.
* @param fd An open socket.
* @param device The name of the network interface of interest.
--
2.1.4


------------------------------------------------------------------------------
Richard Cochran
2016-07-31 21:48:38 UTC
Permalink
We use a hash table to remember the mapping. Since our existing hash
table is a simply string hash, we convert the integer index into a decimal
string. Although hashing integers in this way is sub-optimal, the table
will not be used in a performance critical path.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)

diff --git a/clock.c b/clock.c
index 7b0d923..7bfc053 100644
--- a/clock.c
+++ b/clock.c
@@ -31,6 +31,7 @@
#include "clockcheck.h"
#include "foreign.h"
#include "filter.h"
+#include "hash.h"
#include "missing.h"
#include "msg.h"
#include "phc.h"
@@ -38,6 +39,7 @@
#include "servo.h"
#include "stats.h"
#include "print.h"
+#include "sk.h"
#include "tlv.h"
#include "tsproc.h"
#include "uds.h"
@@ -93,6 +95,7 @@ struct clock {
int pollfd_valid;
int nports; /* does not include the UDS port */
int last_port_number;
+ struct hash *index2port;
int free_running;
int freq_est_interval;
int grand_master_capable; /* for 802.1AS only */
@@ -270,6 +273,7 @@ void clock_destroy(struct clock *c)
}
port_close(c->uds_port);
free(c->pollfd);
+ hash_destroy(c->index2port, NULL);
if (c->clkid != CLOCK_REALTIME) {
phc_close(c->clkid);
}
@@ -767,6 +771,8 @@ static int clock_add_port(struct clock *c, int phc_index,
struct interface *iface)
{
struct port *p, *piter, *lastp = NULL;
+ int fd, index;
+ char key[16];

if (clock_resize_pollfd(c, c->nports + 1)) {
return -1;
@@ -786,6 +792,23 @@ static int clock_add_port(struct clock *c, int phc_index,
}
c->nports++;
clock_fda_changed(c);
+
+ /* Remember the index to port mapping, for link status tracking. */
+ fd = sk_interface_fd();
+ if (fd < 0) {
+ return -1;
+ }
+ index = sk_interface_index(fd, iface->name);
+ if (index < 0) {
+ return -1;
+ }
+ snprintf(key, sizeof(key), "%d", index);
+ if (hash_insert(c->index2port, key, p)) {
+ pr_err("failed to add port with index %d twice!", index);
+ return -1;
+ }
+ close(fd);
+
return 0;
}

@@ -1087,6 +1110,11 @@ struct clock *clock_create(enum clock_type type, struct config *config,
}
clock_fda_changed(c);

+ c->index2port = hash_create();
+ if (!c->index2port) {
+ pr_err("failed create index-to-port hash table");
+ return NULL;
+ }
/* Create the ports. */
STAILQ_FOREACH(iface, &config->interfaces, list) {
if (clock_add_port(c, phc_index, timestamping, iface)) {
--
2.1.4


------------------------------------------------------------------------------
Richard Cochran
2016-07-31 21:48:39 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
port.c | 13 +++++++++++++
port.h | 14 ++++++++++++++
2 files changed, 27 insertions(+)

diff --git a/port.c b/port.c
index 9f07cea..666fed0 100644
--- a/port.c
+++ b/port.c
@@ -117,6 +117,7 @@ struct port {
int path_trace_enabled;
int rx_timestamp_offset;
int tx_timestamp_offset;
+ int link_status;
struct fault_interval flt_interval_pertype[FT_CNT];
enum fault_type last_fault_type;
unsigned int versionNumber; /*UInteger4*/
@@ -2357,6 +2358,17 @@ int port_number(struct port *p)
return portnum(p);
}

+int port_link_status_get(struct port *p)
+{
+ return p->link_status;
+}
+
+void port_link_status_set(struct port *p, int up)
+{
+ p->link_status = up ? 1 : 0;
+ pr_notice("port %hu: link %s", portnum(p), up ? "up" : "down");
+}
+
int port_manage(struct port *p, struct port *ingress, struct ptp_message *msg)
{
struct management_tlv *mgt;
@@ -2573,6 +2585,7 @@ struct port *port_open(int phc_index,
p->path_trace_enabled = config_get_int(cfg, p->name, "path_trace_enabled");
p->rx_timestamp_offset = config_get_int(cfg, p->name, "ingressLatency");
p->tx_timestamp_offset = config_get_int(cfg, p->name, "egressLatency");
+ p->link_status = 1;
p->clock = clock;
p->trp = transport_create(cfg, transport);
if (!p->trp)
diff --git a/port.h b/port.h
index 662eaef..19dec4a 100644
--- a/port.h
+++ b/port.h
@@ -127,6 +127,20 @@ struct PortIdentity port_identity(struct port *p);
int port_number(struct port *p);

/**
+ * Obtain the link status of a port.
+ * @param p A port instance.
+ * @return One (1) if the link is up, zero otherwise.
+ */
+int port_link_status_get(struct port *p);
+
+/**
+ * Sets the link status for a port.
+ * @param p A port instance.
+ * @param up Pass one (1) if the link is up and zero if down.
+ */
+void port_link_status_set(struct port *p, int up);
+
+/**
* Manage a port according to a given message.
* @param p A pointer previously obtained via port_open().
* @param ingress The port on which 'msg' was received.
--
2.1.4


------------------------------------------------------------------------------
Richard Cochran
2016-07-31 21:48:40 UTC
Permalink
Poll for link up/down events. When a link goes down, the port becomes
faulty until the link goes up again. Because the netlink notifications
race with our existing fault detection, we keep the fault timer from
before, but a downed link prevents clear the fault.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/clock.c b/clock.c
index 7bfc053..f36f526 100644
--- a/clock.c
+++ b/clock.c
@@ -39,6 +39,7 @@
#include "servo.h"
#include "stats.h"
#include "print.h"
+#include "rtnl.h"
#include "sk.h"
#include "tlv.h"
#include "tsproc.h"
@@ -271,6 +272,7 @@ void clock_destroy(struct clock *c)
LIST_FOREACH_SAFE(p, &c->ports, list, tmp) {
clock_remove_port(c, p);
}
+ rtnl_close(c->pollfd[0].fd);
port_close(c->uds_port);
free(c->pollfd);
hash_destroy(c->index2port, NULL);
@@ -320,6 +322,25 @@ static void clock_freq_est_reset(struct clock *c)
c->fest.count = 0;
}

+static void clock_link_status(void *ctx, int index, int linkup)
+{
+ struct clock *c = ctx;
+ struct port *p;
+ char key[16];
+
+ snprintf(key, sizeof(key), "%d", index);
+ p = hash_lookup(c->index2port, key);
+ if (!p) {
+ return;
+ }
+ port_link_status_set(p, linkup);
+ if (linkup) {
+ port_dispatch(p, EV_FAULT_CLEARED, 0);
+ } else {
+ port_dispatch(p, EV_FAULT_DETECTED, 0);
+ }
+}
+
static void clock_management_send_error(struct port *p,
struct ptp_message *msg, int error_id)
{
@@ -1096,13 +1117,19 @@ struct clock *clock_create(enum clock_type type, struct config *config,
LIST_INIT(&c->ports);
c->last_port_number = 0;

- /*
- * Create the UDS interface.
- */
if (clock_resize_pollfd(c, 0)) {
pr_err("failed to allocate pollfd");
return NULL;
}
+
+ /* Open a RT netlink socket. */
+ c->pollfd[0].fd = rtnl_open();
+ if (c->pollfd[0].fd < 0) {
+ return NULL;
+ }
+ c->pollfd[0].events = POLLIN|POLLPRI;
+
+ /* Create the UDS interface. */
c->uds_port = port_open(phc_index, timestamping, 0, udsif, c);
if (!c->uds_port) {
pr_err("failed to open the UDS port");
@@ -1129,6 +1156,7 @@ struct clock *clock_create(enum clock_type type, struct config *config,
port_dispatch(p, EV_INITIALIZE, 0);
}
port_dispatch(c->uds_port, EV_INITIALIZE, 0);
+ rtnl_link_query(c->pollfd[0].fd);

return c;
}
@@ -1194,9 +1222,12 @@ static int clock_resize_pollfd(struct clock *c, int new_nports)
{
struct pollfd *new_pollfd;

- /* Need to allocate one extra block of fds for uds */
+ /*
+ * Need to allocate one descriptor for RT netlink and one
+ * whole extra block of fds for UDS.
+ */
new_pollfd = realloc(c->pollfd,
- (new_nports + 1) * N_CLOCK_PFD *
+ 1 + (new_nports + 1) * N_CLOCK_PFD *
sizeof(struct pollfd));
if (!new_pollfd)
return -1;
@@ -1221,7 +1252,7 @@ static void clock_fill_pollfd(struct pollfd *dest, struct port *p)
static void clock_check_pollfd(struct clock *c)
{
struct port *p;
- struct pollfd *dest = c->pollfd;
+ struct pollfd *dest = c->pollfd + 1;

if (c->pollfd_valid)
return;
@@ -1437,7 +1468,7 @@ int clock_poll(struct clock *c)
struct port *p;

clock_check_pollfd(c);
- cnt = poll(c->pollfd, (c->nports + 1) * N_CLOCK_PFD, -1);
+ cnt = poll(c->pollfd, 1 + (c->nports + 1) * N_CLOCK_PFD, -1);
if (cnt < 0) {
if (EINTR == errno) {
return 0;
@@ -1449,7 +1480,13 @@ int clock_poll(struct clock *c)
return 0;
}

+ /* Check the RT netlink. */
cur = c->pollfd;
+ if (cur->revents & (POLLIN|POLLPRI)) {
+ rtnl_link_status(cur->fd, clock_link_status, c);
+ }
+
+ cur++;
LIST_FOREACH(p, &c->ports, list) {
/* Let the ports handle their events. */
for (i = err = 0; i < N_POLLFD && !err; i++) {
@@ -1468,10 +1505,15 @@ int clock_poll(struct clock *c)
}
}

- /* Check the fault timer. */
+ /*
+ * When the fault timer expires we clear the fault,
+ * but only if the link is up.
+ */
if (cur[N_POLLFD].revents & (POLLIN|POLLPRI)) {
clock_fault_timeout(p, 0);
- port_dispatch(p, EV_FAULT_CLEARED, 0);
+ if (port_link_status_get(p)) {
+ port_dispatch(p, EV_FAULT_CLEARED, 0);
+ }
}

cur += N_CLOCK_PFD;
--
2.1.4


------------------------------------------------------------------------------
Keller, Jacob E
2016-08-02 22:08:04 UTC
Permalink
Poll for link up/down events.  When a link goes down, the port
becomes
faulty until the link goes up again.  Because the netlink
notifications
race with our existing fault detection, we keep the fault timer from
before, but a downed link prevents clear the fault.
---
<snip>
+static void clock_link_status(void *ctx, int index, int linkup)
+{
+ struct clock *c = ctx;
+ struct port *p;
+ char key[16];
+
+ snprintf(key, sizeof(key), "%d", index);
+ p = hash_lookup(c->index2port, key);
+ if (!p) {
+ return;
+ }
+ port_link_status_set(p, linkup);
+ if (linkup) {
+ port_dispatch(p, EV_FAULT_CLEARED, 0);
+ } else {
+ port_dispatch(p, EV_FAULT_DETECTED, 0);
+ }
Wouldn't these port dispatch messages interfer with the current ones?
Shouldn't we only set the link status here and not actually send the
message? I thought that was from the patch description. Or am I missing
something?
- /* Check the fault timer. */
+ /*
+  * When the fault timer expires we clear the fault,
+  * but only if the link is up.
+  */
  if (cur[N_POLLFD].revents & (POLLIN|POLLPRI)) {
  clock_fault_timeout(p, 0);
- port_dispatch(p, EV_FAULT_CLEARED, 0);
+ if (port_link_status_get(p)) {
+ port_dispatch(p, EV_FAULT_CLEARED,
0);
+ }
 
Here? Wouldn't these two port dispatch things be racing with the ones
above?

Thanks,
Jake
------------------------------------------------------------------------------
Richard Cochran
2016-08-03 21:04:53 UTC
Permalink
Post by Keller, Jacob E
Here? Wouldn't these two port dispatch things be racing with the ones
above?
I don't think the link events will spoil the existing timeouts. Here
are the currently defined fault types.

FT_UNSPECIFIED - We use this for any kind of socket IO error and
also in case of low memory. It very non-specific. The most likely
cause of socket IO error is a downed link. This fault also covers
missing time stamps from flakey or limited HW or drivers.
Regardless of the specific cause, we simply wait for a defined power
of 2 number of seconds and hope the problem goes away.

FT_BAD_PEER_NETWORK - This was added to support gPTP which requires
a linear number of seconds (not 2^N) for this specific fault.

FT_SWITCH_PHC - This is used to distinguish a possible fault when
switching clocks in the "jbod" mode. The value is hard coded at 16
seconds, without any config option at all. The purpose of this
fault is really debugging. In case the jbod mode every explodes in
the wild, we will be able to see the cause.

Here is the new state machine (ascii art!):

+--------+ Fault +---------+
| |------------>| |
| UP | | FAULT |
| |<------------| |
+--+--+--+ Timeout +---------+
A | /
| | /
Link-Up | | Link-Down /
| | /
| V /
+--+--+--+ / Link-Down
| | /
| DOWN |<--------/
| |
+--------+

If the fault timer occurs in the DOWN state, we simply ignore it.
After all, without the link the port is useless.

There is one case where the new code changes the existing behavior.
If the link quickly does down and then up again while another fault
(and its timer) are active, then we will enter the UP state without
waiting for the fault timer expiration. However, I consider this
behavior acceptable, because when a link goes up, you are starting
from with a clean slate. Even the network might have changed, so the
FT_BAD_PEER_NETWORK fault is then dubious.

I would welcome even more comprehensive fault handling, if some one
wants to code it. But having this more intelligent link handling is
an improvement and at least a step in the right direction, IMHO.

Thoughts?

Thanks,
Richard


------------------------------------------------------------------------------
Keller, Jacob E
2016-08-03 21:49:35 UTC
Permalink
Post by Keller, Jacob E
Here? Wouldn't these two port dispatch things be racing with the ones
above?
I don't think the link events will spoil the existing timeouts.  Here
are the currently defined fault types.
Hmm. Ok, it just seems like you call port_dispatch with FAULT_CLEARED
when link goes up. The commit message says "we will keep the old fault
handling and won't clear the fault while link is down." but this new
flow seems to me to actually prevent the full wait because we'll clear
the fault once we go back up. I guess it's just a mis-read of the
commit message. For sure, we will *stay* in fault as long as the link
is still down (good). But if the link goes up before the fault timeout
ends, we'll just clear the fault and reset. I think this behavior is an
improvement since it will allow a link-down/link-up to clear faster. It
just wasn't clear from the commit mesage to me.
There is one case where the new code changes the existing behavior.
If the link quickly does down and then up again while another fault
(and its timer) are active, then we will enter the UP state without
waiting for the fault timer expiration.  However, I consider this
behavior acceptable, because when a link goes up, you are starting
from with a clean slate.  Even the network might have changed, so the
FT_BAD_PEER_NETWORK fault is then dubious.
Hmm. I think the commit message seems to indicate that we wouldn't
clear faults in the link up case. But I think I agree with the logic
here, this is definitely an improvement over the previous situation and
clearing the state and going up makes sense to me.
I would welcome even more comprehensive fault handling, if some one
wants to code it.  But having this more intelligent link handling is
an improvement and at least a step in the right direction, IMHO.
Thoughts?
It's worth thinking about for the future, but what we have now should
be an improvement.

Regards,
Jake
Thanks,
Richard
------------------------------------------------------------------------------
Richard Cochran
2016-08-04 10:16:36 UTC
Permalink
Post by Keller, Jacob E
Hmm. Ok, it just seems like you call port_dispatch with FAULT_CLEARED
when link goes up. The commit message says "we will keep the old fault
handling and won't clear the fault while link is down." but this new
flow seems to me to actually prevent the full wait because we'll clear
the fault once we go back up. I guess it's just a mis-read of the
commit message. For sure, we will *stay* in fault as long as the link
is still down (good). But if the link goes up before the fault timeout
ends, we'll just clear the fault and reset. I think this behavior is an
improvement since it will allow a link-down/link-up to clear faster. It
just wasn't clear from the commit mesage to me.
I'll try to improve the message...

Thanks,
Richard

------------------------------------------------------------------------------
Jesuiter, Henry (ALC NetworX GmbH)
2016-08-09 08:45:44 UTC
Permalink
-----Ursprüngliche Nachricht-----
Von: Richard Cochran [mailto:***@gmail.com]
Gesendet: Sonntag, 31. Juli 2016 23:49
An: linuxptp-***@lists.sourceforge.net
Cc: Jesuiter, Henry (ALC NetworX GmbH) <***@alcnetworx.de>
Betreff: [PATCH RFC 7/7] clock: Monitor the link status using a RT netlink socket.

@@ -1194,9 +1222,12 @@ static int clock_resize_pollfd(struct clock *c, int new_nports)
{
struct pollfd *new_pollfd;

- /* Need to allocate one extra block of fds for uds */
+ /*
+ * Need to allocate one descriptor for RT netlink and one
+ * whole extra block of fds for UDS.
+ */
new_pollfd = realloc(c->pollfd,
- (new_nports + 1) * N_CLOCK_PFD *
+ 1 + (new_nports + 1) * N_CLOCK_PFD *
sizeof(struct pollfd));

---

Hi,

sorry for the late answer, but I was in vacation last week.

After trying out your patches, I've only one thing to mention.
Since you are using c->pollfd[0] for the netlink fd, the pollfd
resize() should use 'sizeof(struct pollfd)' instead of '1' for this
descriptor. Otherwise, you will get some memory corruption.

Best regards
Henry
Richard Cochran
2016-08-09 11:20:32 UTC
Permalink
Post by Jesuiter, Henry (ALC NetworX GmbH)
After trying out your patches, I've only one thing to mention.
Since you are using c->pollfd[0] for the netlink fd, the pollfd
resize() should use 'sizeof(struct pollfd)' instead of '1' for this
descriptor. Otherwise, you will get some memory corruption.
Putting that bug aside, does this series otherwise do what you need?

Thanks for the careful review,

Richard
Jesuiter, Henry (ALC NetworX GmbH)
2016-08-09 11:35:02 UTC
Permalink
Until now I've tested your patch on a two-port device only. But it works fine here,
as one would expect. ;-)

There is one message I'm still not sure about, but may be you can explain it shortly.
If I get a port up (except it is already up if ptp4l starts), I get the following message:
ptp4l[294.884]: eth1: SO_SELECT_ERR_QUEUE: Protocol not available

After that the port changes correctly from fault-state to LISTENING and all seem to
work fine.

Best regards
Henry

-----Ursprüngliche Nachricht-----
Von: Richard Cochran [mailto:***@gmail.com]
Gesendet: Dienstag, 9. August 2016 13:21
An: Jesuiter, Henry (ALC NetworX GmbH) <***@alcnetworx.de>
Cc: linuxptp-***@lists.sourceforge.net
Betreff: Re: [PATCH RFC 7/7] clock: Monitor the link status using a RT netlink socket.
Post by Jesuiter, Henry (ALC NetworX GmbH)
After trying out your patches, I've only one thing to mention.
Since you are using c->pollfd[0] for the netlink fd, the pollfd
resize() should use 'sizeof(struct pollfd)' instead of '1' for this
descriptor. Otherwise, you will get some memory corruption.
Putting that bug aside, does this series otherwise do what you need?

Thanks for the careful review,

Richard
Richard Cochran
2016-08-09 12:40:00 UTC
Permalink
Post by Jesuiter, Henry (ALC NetworX GmbH)
There is one message I'm still not sure about, but may be you can explain it shortly.
ptp4l[294.884]: eth1: SO_SELECT_ERR_QUEUE: Protocol not available
That is just a warning. If you upgrade your kernel, then you will get
more efficient transmit time stamp polling, and the warning goes away
too!

Thanks,
Richard
Jesuiter, Henry (ALC NetworX GmbH)
2016-08-09 12:49:22 UTC
Permalink
We have to thank you.

Together with your other last patches (regarding port identity usage on announce messages),
we are finally able to run ptp4l on our devices on all available ports (before the second port
sometimes chooses master if the device didn't sit behind a PTP BC switch. That helps us much.

Best regards
Henry

-----Ursprüngliche Nachricht-----
Von: Richard Cochran [mailto:***@gmail.com]
Gesendet: Dienstag, 9. August 2016 14:40
An: Jesuiter, Henry (ALC NetworX GmbH) <***@alcnetworx.de>
Cc: linuxptp-***@lists.sourceforge.net
Betreff: Re: [PATCH RFC 7/7] clock: Monitor the link status using a RT netlink socket.
Post by Jesuiter, Henry (ALC NetworX GmbH)
There is one message I'm still not sure about, but may be you can explain it shortly.
ptp4l[294.884]: eth1: SO_SELECT_ERR_QUEUE: Protocol not available
That is just a warning. If you upgrade your kernel, then you will get
more efficient transmit time stamp polling, and the warning goes away
too!

Thanks,
Richard

Loading...