[Linuxptp-devel] [PATCH v2] ptp4l: use poll to wait for tx timestamps instead of a retry loop

Jacob Keller

2013-04-04 20:10:46 UTC

This patch modifies the behavior of sk_receive's retry loop to use poll. It
supersedes the previous RFC which required kernel modification, as it is
possible to use poll without any particular event. In this case poll will wait
until the timeout or a POLLERR event. This means we can avoid the use of a retry
loop, and can use much cleaner logic.

This patch also breaks out sk_timestamp from the sk_receive logic as a seperate
function with seperate logic. It also cleans up the call to sk_timestamp as it
does not need to be passed a buffer.

The old tx_timestamp_retries has been replaced with tx_timestamp_timeout
specified in milliseconds (the smallest unit poll() accepts).

The patch simplifies and cleans up the sk_timestamp code, and reduces errors due
to missing timestamps as we are no longer using a polling method that fails too
often on some parts.

-v2-
* Removed the tx_timestamp_retries from the manpage

Signed-off-by: Jacob Keller <***@intel.com>
---
config.c | 6 ++--
config.h | 2 +
default.cfg | 2 +
gPTP.cfg | 2 +
ptp4l.8 | 8 ++---
ptp4l.c | 2 +
raw.c | 6 ++--
sk.c | 95 +++++++++++++++++++++++++++++++++++++++++++++--------------
sk.h | 17 +++++++----
udp.c | 5 +--
udp6.c | 5 +--
11 files changed, 103 insertions(+), 47 deletions(-)

diff --git a/config.c b/config.c
index 4727942..7318046 100644
--- a/config.c
+++ b/config.c
@@ -270,10 +270,10 @@ static enum parser_result parse_global_setting(const char *option,
return BAD_VALUE;
*cfg->assume_two_step = val ? 1 : 0;

- } else if (!strcmp(option, "tx_timestamp_retries")) {
- if (1 != sscanf(value, "%u", &val) || !(val > 0))
+ } else if (!strcmp(option, "tx_timestamp_timeout")) {
+ if (1 != sscanf(value, "%u", &val) || (!val > 0))
return BAD_VALUE;
- *cfg->tx_timestamp_retries = val;
+ *cfg->tx_timestamp_timeout = val;

} else if (!strcmp(option, "pi_proportional_const")) {
if (1 != sscanf(value, "%lf", &df) || !(df >= 0.0 && df < 1.0))
diff --git a/config.h b/config.h
index 497d683..e7f1f50 100644
--- a/config.h
+++ b/config.h
@@ -61,7 +61,7 @@ struct config {
struct default_ds dds;
struct port_defaults pod;
int *assume_two_step;
- int *tx_timestamp_retries;
+ int *tx_timestamp_timeout;

enum servo_type clock_servo;

diff --git a/default.cfg b/default.cfg
index 84cc0df..4694e32 100644
--- a/default.cfg
+++ b/default.cfg
@@ -30,7 +30,7 @@ assume_two_step 0
logging_level 6
path_trace_enabled 0
follow_up_info 0
-tx_timestamp_retries 100
+tx_timestamp_timeout 1
use_syslog 1
verbose 0
summary_interval 0
diff --git a/gPTP.cfg b/gPTP.cfg
index f14990b..83a44bf 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -29,7 +29,7 @@ assume_two_step 1
logging_level 6
path_trace_enabled 1
follow_up_info 1
-tx_timestamp_retries 100
+tx_timestamp_timeout 1
use_syslog 1
verbose 0
summary_interval 0
diff --git a/ptp4l.8 b/ptp4l.8
index 45ef6f3..c3c5fa9 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -264,10 +264,10 @@ Treat one-step responses as two-step if enabled. It is used to work around
buggy 802.1AS switches.
The default is 0 (disabled).
.TP
-.B tx_timestamp_retries
-The number of retries to fetch the tx time stamp from the kernel when a message
-is sent.
-The default is 100.
+.B tx_timestamp_timeout
+The number of milliseconds to wait while polling for a tx timestamp when a
+message has just been sent.
+The default is 1.
.TP
.B clock_servo
The servo which is used to synchronize the local clock. Currently only one
diff --git a/ptp4l.c b/ptp4l.c
index 1312cd6..3bf1f86 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -89,7 +89,7 @@ static struct config cfg_settings = {
.transport = TRANS_UDP_IPV4,

.assume_two_step = &assume_two_step,
- .tx_timestamp_retries = &sk_tx_retries,
+ .tx_timestamp_timeout = &sk_tx_timeout,

.clock_servo = CLOCK_SERVO_PI,

diff --git a/raw.c b/raw.c
index 326ccd2..8c0c3b0 100644
--- a/raw.c
+++ b/raw.c
@@ -239,7 +239,7 @@ static int raw_recv(struct transport *t, int fd, void *buf, int buflen,
buflen += hlen;
hdr = (struct eth_hdr *) ptr;

- cnt = sk_receive(fd, ptr, buflen, hwts, 0);
+ cnt = sk_receive(fd, ptr, buflen, hwts);

if (raw->vlan) {
if (ETH_P_1588 == ntohs(hdr->type)) {
@@ -264,7 +264,7 @@ static int raw_send(struct transport *t, struct fdarray *fda, int event, int pee
struct raw *raw = container_of(t, struct raw, t);
ssize_t cnt;
int fd = event ? fda->fd[FD_EVENT] : fda->fd[FD_GENERAL];
- unsigned char pkt[1600], *ptr = buf;
+ unsigned char *ptr = buf;
struct eth_hdr *hdr;

ptr -= sizeof(*hdr);
@@ -286,7 +286,7 @@ static int raw_send(struct transport *t, struct fdarray *fda, int event, int pee
/*
* Get the time stamp right away.
*/
- return event == TRANS_EVENT ? sk_receive(fd, pkt, len, hwts, MSG_ERRQUEUE) : cnt;
+ return event == TRANS_EVENT ? sk_timestamp(fd, hwts) : cnt;
}

static void raw_release(struct transport *t)
diff --git a/sk.c b/sk.c
index e09d459..7fee0c4 100644
--- a/sk.c
+++ b/sk.c
@@ -28,13 +28,14 @@
#include <unistd.h>
#include <ifaddrs.h>
#include <stdlib.h>
+#include <poll.h>

#include "print.h"
#include "sk.h"
+#include "missing.h"

/* globals */
-
-int sk_tx_retries = 100;
+int sk_tx_timeout = 1;

/* private methods */

@@ -196,15 +197,16 @@ int sk_interface_addr(char *name, int family, uint8_t *addr, int len)
return result;
}

-int sk_receive(int fd, void *buf, int buflen,
- struct hw_timestamp *hwts, int flags)
+int sk_timestamp(int fd, struct hw_timestamp *hwts)
{
char control[256];
- int cnt, level, try_again, type;
+ char junk[1600];
+ int res, cnt, level, type;
struct cmsghdr *cm;
- struct iovec iov = { buf, buflen };
+ struct iovec iov = { junk, 1600 };
struct msghdr msg;
struct timespec *ts = NULL;
+ struct pollfd pfd;

memset(control, 0, sizeof(control));
memset(&msg, 0, sizeof(msg));
@@ -213,28 +215,77 @@ int sk_receive(int fd, void *buf, int buflen,
msg.msg_control = control;
msg.msg_controllen = sizeof(control);

- try_again = flags == MSG_ERRQUEUE ? sk_tx_retries : 1;
+ pfd.fd = fd;
+ pfd.events = 0;
+ res = poll(&pfd, 1, sk_tx_timeout);
+ if (res < 1) {
+ pr_err("poll tx timestamp failed: %m");
+ return res;
+ } else if (!(pfd.revents & POLLERR)) {
+ pr_err("socket woke up but POLLERR was not set");
+ return -1;
+ }

- for ( ; try_again; try_again--) {
- cnt = recvmsg(fd, &msg, flags);
- if (cnt >= 0) {
- break;
- }
- if (errno == EINTR) {
- try_again++;
- } else if (errno == EAGAIN) {
- usleep(1);
- } else {
+ cnt = recvmsg(fd, &msg, MSG_ERRQUEUE);
+ if (cnt < 1) {
+ pr_err("recvmsg tx timestamp failed: %m");
+ return cnt;
+ }
+
+ for (cm = CMSG_FIRSTHDR(&msg); cm != NULL; cm = CMSG_NXTHDR(&msg, cm)) {
+ level = cm->cmsg_level;
+ type = cm->cmsg_type;
+ if (SOL_SOCKET == level && SO_TIMESTAMPING == type) {
+ if (cm->cmsg_len < sizeof(*ts) * 3) {
+ pr_warning("short SO_TIMESTAMPING message");
+ return -1;
+ }
+ ts = (struct timespec *) CMSG_DATA(cm);
break;
}
}

- if (cnt < 1) {
- if (flags == MSG_ERRQUEUE)
- pr_err("recvmsg tx timestamp failed: %m");
- else
- pr_err("recvmsg failed: %m");
+ if (!ts) {
+ memset(&hwts->ts, 0, sizeof(hwts->ts));
+ return cnt;
+ }
+
+ switch (hwts->type) {
+ case TS_SOFTWARE:
+ hwts->ts = ts[0];
+ break;
+ case TS_HARDWARE:
+ case TS_ONESTEP:
+ hwts->ts = ts[2];
+ break;
+ case TS_LEGACY_HW:
+ hwts->ts = ts[1];
+ break;
}
+ return cnt;
+}
+
+
+int sk_receive(int fd, void *buf, int buflen,
+ struct hw_timestamp *hwts)
+{
+ char control[256];
+ int cnt, level, type;
+ struct cmsghdr *cm;
+ struct iovec iov = { buf, buflen };
+ struct msghdr msg;
+ struct timespec *ts = NULL;
+
+ memset(control, 0, sizeof(control));
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ msg.msg_control = control;
+ msg.msg_controllen = sizeof(control);
+
+ cnt = recvmsg(fd, &msg, 0);
+ if (cnt < 1)
+ pr_err("recvmsg failed: %m");

for (cm = CMSG_FIRSTHDR(&msg); cm != NULL; cm = CMSG_NXTHDR(&msg, cm)) {
level = cm->cmsg_level;
diff --git a/sk.h b/sk.h
index 1923742..a836cd1 100644
--- a/sk.h
+++ b/sk.h
@@ -74,16 +74,23 @@ int sk_interface_macaddr(char *name, unsigned char *mac, int len);
int sk_interface_addr(char *name, int family, uint8_t *addr, int len);

/**
+ * Read a timestamp from a socket.
+ * @param fd An open socket.
+ * @param hwts Pointer to a buffer to receive the message's time stamp.
+ * @return
+ */
+int sk_timestamp(int fd, struct hw_timestamp *hwts);
+
+/**
* Read a message from a socket.
* @param fd An open socket.
* @param buf Buffer to receive the message.
* @param buflen Size of 'buf' in bytes.
* @param hwts Pointer to a buffer to receive the message's time stamp.
- * @param flags Flags to pass to RECV(2).
* @return
*/
int sk_receive(int fd, void *buf, int buflen,
- struct hw_timestamp *hwts, int flags);
+ struct hw_timestamp *hwts);

/**
* Enable time stamping on a given network interface.
@@ -97,9 +104,9 @@ int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
enum transport_type transport);

/**
- * Limits the number of RECVMSG(2) calls when attempting to obtain a
- * transmit time stamp on an event message.
+ * Limits the length of time that poll will be used to wait for a socket
+ * timestamp to be returned. (specified in milliseconds)
*/
-extern int sk_tx_retries;
+extern int sk_tx_timeout;

#endif
diff --git a/udp.c b/udp.c
index fea0122..403cbc5 100644
--- a/udp.c
+++ b/udp.c
@@ -194,7 +194,7 @@ no_event:
static int udp_recv(struct transport *t, int fd, void *buf, int buflen,
struct hw_timestamp *hwts)
{
- return sk_receive(fd, buf, buflen, hwts, 0);
+ return sk_receive(fd, buf, buflen, hwts);
}

static int udp_send(struct transport *t, struct fdarray *fda, int event, int peer,
@@ -203,7 +203,6 @@ static int udp_send(struct transport *t, struct fdarray *fda, int event, int pee
ssize_t cnt;
int fd = event ? fda->fd[FD_EVENT] : fda->fd[FD_GENERAL];
struct sockaddr_in addr;
- unsigned char junk[1600];

memset(&addr, 0, sizeof(addr));
addr.sin_family = AF_INET;
@@ -226,7 +225,7 @@ static int udp_send(struct transport *t, struct fdarray *fda, int event, int pee
/*
* Get the time stamp right away.
*/
- return event == TRANS_EVENT ? sk_receive(fd, junk, len, hwts, MSG_ERRQUEUE) : cnt;
+ return event == TRANS_EVENT ? sk_timestamp(fd, hwts) : cnt;
}

static void udp_release(struct transport *t)
diff --git a/udp6.c b/udp6.c
index 2cb0179..edf6f75 100644
--- a/udp6.c
+++ b/udp6.c
@@ -206,7 +206,7 @@ no_event:
static int udp6_recv(struct transport *t, int fd, void *buf, int buflen,
struct hw_timestamp *hwts)
{
- return sk_receive(fd, buf, buflen, hwts, 0);
+ return sk_receive(fd, buf, buflen, hwts);
}

static int udp6_send(struct transport *t, struct fdarray *fda, int event, int peer,
@@ -216,7 +216,6 @@ static int udp6_send(struct transport *t, struct fdarray *fda, int event, int pe
ssize_t cnt;
int fd = event ? fda->fd[FD_EVENT] : fda->fd[FD_GENERAL];
struct sockaddr_in6 addr;
- unsigned char junk[1600];

memset(&addr, 0, sizeof(addr));
addr.sin6_family = AF_INET6;
@@ -237,7 +236,7 @@ static int udp6_send(struct transport *t, struct fdarray *fda, int event, int pe
/*
* Get the time stamp right away.
*/
- return event == TRANS_EVENT ? sk_receive(fd, junk, len, hwts, MSG_ERRQUEUE) : cnt;
+ return event == TRANS_EVENT ? sk_timestamp(fd, hwts) : cnt;
}

static void udp6_release(struct transport *t)