Discussion:
[Linuxptp-devel] Setting DSCP values in PTP messages
Jesuiter, Henry (ALC NetworX GmbH)
2016-07-06 09:11:23 UTC
Permalink
Hi,

I would like to propose a patch for linuxptp v1.6, that will add the possibility to set DSCP values into PTP messages.

This patch adds the global configuration options "tosEventMessages" and "tosGeneralMessages" to be able to set different DSCP values for those kinds of messages. These options are especially important if one is using linuxptp on media streaming networks (f.e. AES67, RAVENNA, etc.).

It's a first-shot-patch that fulfills our needs here, so feel free to suggest changes or improvements.

Best regards
Henry Jesuiter
Jesuiter, Henry (ALC NetworX GmbH)
2016-07-06 09:21:45 UTC
Permalink
Hi,

sorry, the last patch contained a bug, on setting the DSCP value on the correct sockets. So please use the newly attached one here.

Best regards
Henry Jesuiter

Von: Jesuiter, Henry (ALC NetworX GmbH)
Gesendet: Mittwoch, 6. Juli 2016 11:11
An: 'linuxptp-***@lists.sourceforge.net' <linuxptp-***@lists.sourceforge.net>
Betreff: Setting DSCP values in PTP messages

Hi,

I would like to propose a patch for linuxptp v1.6, that will add the possibility to set DSCP values into PTP messages.

This patch adds the global configuration options "tosEventMessages" and "tosGeneralMessages" to be able to set different DSCP values for those kinds of messages. These options are especially important if one is using linuxptp on media streaming networks (f.e. AES67, RAVENNA, etc.).

It's a first-shot-patch that fulfills our needs here, so feel free to suggest changes or improvements.

Best regards
Henry Jesuiter
Richard Cochran
2016-07-06 11:59:55 UTC
Permalink
Post by Jesuiter, Henry (ALC NetworX GmbH)
sorry, the last patch contained a bug, on setting the DSCP value on the correct sockets. So please use the newly attached one here.
Please resend with the patch in line (not as an attachment).

Thanks,
Richard
Jesuiter, Henry (ALC NetworX GmbH)
2016-07-06 13:32:26 UTC
Permalink
Oh yes, my fault. Please find it below:

---
Index: config.c
===================================================================
--- config.c (Revision 827)
+++ config.c (Revision 830)
@@ -236,6 +236,8 @@
GLOB_ITEM_INT("use_syslog", 1, 0, 1),
GLOB_ITEM_STR("userDescription", ""),
GLOB_ITEM_INT("verbose", 0, 0, 1),
+ GLOB_ITEM_INT("tosEventMessage", 56, 0, 63),
+ GLOB_ITEM_INT("tosGeneralMessage", 46, 0, 46)
};

static enum parser_result
Index: udp.c
===================================================================
--- udp.c (Revision 827)
+++ udp.c (Revision 830)
@@ -149,6 +149,29 @@
return -1;
}

+// setting IP DSCP value here
+static int set_priority(int sock_fd, uint8_t dscp) {
+ int tos;
+ socklen_t tos_len;
+
+ if (!sock_fd) {
+ return 0;
+ }
+
+ tos_len = sizeof(tos);
+ if (getsockopt(sock_fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
+ tos = 0;
+ }
+
+ tos |= dscp<<2;
+ tos_len = sizeof(tos);
+ if (setsockopt(sock_fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
enum { MC_PRIMARY, MC_PDELAY };

static struct in_addr mcast_addr[2];
@@ -176,11 +199,23 @@
if (efd < 0)
goto no_event;

+ // set DSCP priority for event messages
+ uint8_t event_dscp = config_get_int(t->cfg, NULL, "tosEventMessage");
+ if(set_priority(efd, event_dscp) < 0) {
+ pr_warning("Could not set COS value for PTP event messages.");
+ }
+
gfd = open_socket(name, mcast_addr, GENERAL_PORT, ttl);
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV4))
+ // set DSCP priority for general messages
+ uint8_t general_dscp = config_get_int(t->cfg, NULL, "tosGeneralMessage");
+ if(set_priority(gfd, general_dscp) < 0) {
+ pr_warning("Could not set DHCP value for PTP general messages.");
+ }
+
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV4))
goto no_timestamping;

if (sk_general_init(gfd))

Index: udp6.c
===================================================================
--- udp6.c (Revision 827)
+++ udp6.c (Revision 830)
@@ -157,6 +157,29 @@
return -1;
}

+// setting IP DSCP value here
+static int set_priority(int sock_fd, uint8_t dscp) {
+ int tos;
+ socklen_t tos_len;
+
+ if (!sock_fd) {
+ return 0;
+ }
+
+ tos_len = sizeof(tos);
+ if (getsockopt(sock_fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
+ tos = 0;
+ }
+
+ tos |= dscp<<2;
+ tos_len = sizeof(tos);
+ if (setsockopt(sock_fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
enum { MC_PRIMARY, MC_PDELAY };

static struct in6_addr mc6_addr[2];
@@ -186,11 +209,23 @@
if (efd < 0)
goto no_event;

+ // set DSCP priority for event messages
+ uint8_t event_dscp = config_get_int(t->cfg, NULL, "tosEventMessage");
+ if(set_priority(efd, event_dscp) < 0) {
+ pr_warning("Could not set COS value for PTP event messages.");
+ }
+
gfd = open_socket_ipv6(name, mc6_addr, GENERAL_PORT, &udp6->index, hop_limit);
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV6))
+ // set DSCP priority for general messages
+ uint8_t general_dscp = config_get_int(t->cfg, NULL, "tosGeneralMessage");
+ if(set_priority(gfd, general_dscp) < 0) {
+ pr_warning("Could not set DHCP value for PTP general messages.");
+ }
+
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV6))
goto no_timestamping;

if (sk_general_init(gfd))
---

Best regards
Henry Jesuiter
Richard Cochran
2016-07-06 13:58:59 UTC
Permalink
Post by Jesuiter, Henry (ALC NetworX GmbH)
---
Index: config.c
===================================================================
--- config.c (Revision 827)
+++ config.c (Revision 830)
@@ -236,6 +236,8 @@
GLOB_ITEM_INT("use_syslog", 1, 0, 1),
GLOB_ITEM_STR("userDescription", ""),
GLOB_ITEM_INT("verbose", 0, 0, 1),
+ GLOB_ITEM_INT("tosEventMessage", 56, 0, 63),
+ GLOB_ITEM_INT("tosGeneralMessage", 46, 0, 46)
Default should be zero.

Since this is a bit field (isn't it?), max would make more sense as
hexadecimal value.

Keep the table in alphabetical order please.

The options should be all lower case, like 'tos_event'. (We only use
lowerCamelCase when the option comes from a published standard.)
Post by Jesuiter, Henry (ALC NetworX GmbH)
};
static enum parser_result
Index: udp.c
===================================================================
--- udp.c (Revision 827)
+++ udp.c (Revision 830)
@@ -149,6 +149,29 @@
return -1;
}
+// setting IP DSCP value here
+static int set_priority(int sock_fd, uint8_t dscp) {
No C++ comments, K&R style. Please check CODING_STYLE.org.
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ int tos;
+ socklen_t tos_len;
+
+ if (!sock_fd) {
This test is useless, because the caller ensures that the FD is valid.
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ return 0;
+ }
+
+ tos_len = sizeof(tos);
+ if (getsockopt(sock_fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
+ tos = 0;
+ }
+
+ tos |= dscp<<2;
You need to clear the bit field first.
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ tos_len = sizeof(tos);
+ if (setsockopt(sock_fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
enum { MC_PRIMARY, MC_PDELAY };
static struct in_addr mcast_addr[2];
@@ -176,11 +199,23 @@
if (efd < 0)
goto no_event;
+ // set DSCP priority for event messages
+ uint8_t event_dscp = config_get_int(t->cfg, NULL, "tosEventMessage");
+ if(set_priority(efd, event_dscp) < 0) {
^
Coding style.

Only call set_priority if dscp != 0.

Both of these calls should happen *after* sk_general_init. (It not
wrong as is, but it would fit better to the existing code which does
first opens the sockets, then configures them.)
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ pr_warning("Could not set COS value for PTP event messages.");
+ }
+
gfd = open_socket(name, mcast_addr, GENERAL_PORT, ttl);
if (gfd < 0)
goto no_general;
- if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV4))
+ // set DSCP priority for general messages
+ uint8_t general_dscp = config_get_int(t->cfg, NULL, "tosGeneralMessage");
+ if(set_priority(gfd, general_dscp) < 0) {
+ pr_warning("Could not set DHCP value for PTP general messages.");
DSCP.
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ }
+
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV4))
goto no_timestamping;
if (sk_general_init(gfd))
Index: udp6.c
===================================================================
--- udp6.c (Revision 827)
+++ udp6.c (Revision 830)
@@ -157,6 +157,29 @@
return -1;
}
+// setting IP DSCP value here
+static int set_priority(int sock_fd, uint8_t dscp) {
This function is a duplicate of the one above, so it should go into
sk.c with the other shared code.
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ int tos;
+ socklen_t tos_len;
+
+ if (!sock_fd) {
+ return 0;
+ }
+
+ tos_len = sizeof(tos);
+ if (getsockopt(sock_fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
+ tos = 0;
+ }
+
+ tos |= dscp<<2;
+ tos_len = sizeof(tos);
+ if (setsockopt(sock_fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
enum { MC_PRIMARY, MC_PDELAY };
static struct in6_addr mc6_addr[2];
@@ -186,11 +209,23 @@
if (efd < 0)
goto no_event;
+ // set DSCP priority for event messages
+ uint8_t event_dscp = config_get_int(t->cfg, NULL, "tosEventMessage");
+ if(set_priority(efd, event_dscp) < 0) {
+ pr_warning("Could not set COS value for PTP event messages.");
+ }
+
gfd = open_socket_ipv6(name, mc6_addr, GENERAL_PORT, &udp6->index, hop_limit);
if (gfd < 0)
goto no_general;
- if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV6))
+ // set DSCP priority for general messages
+ uint8_t general_dscp = config_get_int(t->cfg, NULL, "tosGeneralMessage");
+ if(set_priority(gfd, general_dscp) < 0) {
+ pr_warning("Could not set DHCP value for PTP general messages.");
+ }
+
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV6))
goto no_timestamping;
if (sk_general_init(gfd))
This would look better if you had a helper in sk.c:

int sk_set_priority(int efd, int gfd);

Then you would have in udp.c/udp6.c:

if (sk_set_priority(efd, gfd)) {
pr_warning("Failed to set the DSCP priority.");
}

Thanks,
Richard
Richard Cochran
2016-07-06 14:42:12 UTC
Permalink
Post by Jesuiter, Henry (ALC NetworX GmbH)
---
Index: config.c
===================================================================
--- config.c (Revision 827)
+++ config.c (Revision 830)
@@ -236,6 +236,8 @@
GLOB_ITEM_INT("use_syslog", 1, 0, 1),
GLOB_ITEM_STR("userDescription", ""),
GLOB_ITEM_INT("verbose", 0, 0, 1),
+ GLOB_ITEM_INT("tosEventMessage", 56, 0, 63),
+ GLOB_ITEM_INT("tosGeneralMessage", 46, 0, 46)
Also, please update the man page and the .cfg files, too.

Thanks,
Richard
Jesuiter, Henry (ALC NetworX GmbH)
2016-07-06 15:15:29 UTC
Permalink
Thank you very much for your fast reply and the notes. Please find below a modified patch according to your annotations:
---
diff -rwbBu a/config.c b/config.c
--- a/config.c 2015-09-19 16:25:11.000000000 +0200
+++ b/config.c 2016-07-06 17:07:38.176449373 +0200
@@ -226,6 +226,8 @@
PORT_ITEM_INT("syncReceiptTimeout", 0, 0, UINT8_MAX),
GLOB_ITEM_INT("timeSource", INTERNAL_OSCILLATOR, 0x10, 0xfe),
GLOB_ITEM_ENU("time_stamping", TS_HARDWARE, timestamping_enu),
+ GLOB_ITEM_INT("tos_event", 0, 0, 0x3F),
+ GLOB_ITEM_INT("tos_general", 0, 0, 0x3F),
PORT_ITEM_INT("transportSpecific", 0, 0, 0x0F),
PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
GLOB_ITEM_INT("twoStepFlag", 1, 0, 1),
@@ -235,7 +237,7 @@
GLOB_ITEM_STR("uds_address", "/var/run/ptp4l"),
GLOB_ITEM_INT("use_syslog", 1, 0, 1),
GLOB_ITEM_STR("userDescription", ""),
- GLOB_ITEM_INT("verbose", 0, 0, 1),
+ GLOB_ITEM_INT("verbose", 0, 0, 1)
};

static enum parser_result
diff -rwbBu a/default.cfg b/default.cfg
--- a/default.cfg 2015-09-19 16:25:11.000000000 +0200
+++ b/default.cfg 2016-07-06 16:49:36.853140183 +0200
@@ -12,6 +12,8 @@
offsetScaledLogVariance 0xFFFF
free_running 0
freq_est_interval 1
+tos_event 0
+tos_general 0
#
# Port Data Set
#
diff -rwbBu a/ptp4l.8 b/ptp4l.8
--- a/ptp4l.8 2015-09-19 16:25:11.000000000 +0200
+++ b/ptp4l.8 2016-07-06 16:58:23.722698053 +0200
@@ -446,6 +446,14 @@
Specifies the address of the UNIX domain socket for receiving local
management messages. The default is /var/run/ptp4l.
.TP
+.B tos_event
+Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
+event messages. Must be a value between 0 and 63. The default is 0.
+.TP
+.B tos_general
+Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
+general messages. Must be a value between 0 and 63. The default is 0.
+.TP
.B logging_level
The maximum logging level of messages which should be printed.
The default is 6 (LOG_INFO).
diff -rwbBu a/sk.c b/sk.c
--- a/sk.c 2015-09-19 16:25:11.000000000 +0200
+++ b/sk.c 2016-07-06 16:40:27.000000000 +0200
@@ -35,6 +35,7 @@
#include "missing.h"
#include "print.h"
#include "sk.h"
+#include "config.h"

/* globals */

@@ -78,6 +79,28 @@
return 0;
}

+static int set_priority(int fd, uint8_t dscp) {
+ int tos;
+ socklen_t tos_len;
+
+ tos_len = sizeof(tos);
+ if (getsockopt(fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
+ tos = 0;
+ }
+
+ /* clear old DSCP value */
+ tos &= ~0xFC;
+
+ /* set new DSCP value */
+ tos |= dscp<<2;
+ tos_len = sizeof(tos);
+ if (setsockopt(fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
/* public methods */

int sk_interface_index(int fd, const char *name)
@@ -105,6 +128,24 @@
return 0;
}

+int sk_set_priority(struct config *c, int event_fd, int general_fd) {
+ uint8_t event_dscp = config_get_int(c, NULL, "tos_event");
+ uint8_t general_dscp = config_get_int(c, NULL, "tos_general");
+
+ int err = 0;
+
+ if(event_dscp != 0) {
+ err = set_priority(event_fd, event_dscp);
+ }
+
+ if(!err && general_dscp) {
+ err = set_priority(general_fd, general_dscp);
+ }
+
+ return err;
+}
+
+
int sk_get_ts_info(const char *name, struct sk_ts_info *sk_info)
{
#ifdef ETHTOOL_GET_TS_INFO
diff -rwbBu a/sk.h b/sk.h
--- a/sk.h 2015-09-19 16:25:11.000000000 +0200
+++ b/sk.h 2016-07-06 16:38:36.000000000 +0200
@@ -55,6 +55,16 @@
int sk_general_init(int fd);

/**
+ * Set DSCP value for socket.
+ * @param c Current linuxptp configuration.
+ * @param event_fd Open socket for ptp event messages.
+ * @param general_fd Open socket for ptp general messages.
+ * @return Zero in success, negative on failure
+ *
+ */
+int sk_set_priority(struct config *c, int event_fd, int general_fd);
+
+/**
* Obtain supported timestamping information
* @param name The name of the interface
* @param info Struct containing obtained timestamping information.
diff -rwbBu a/udp6.c b/udp6.c
--- a/udp6.c 2015-09-19 16:25:11.000000000 +0200
+++ b/udp6.c 2016-07-06 16:39:48.000000000 +0200
@@ -196,6 +196,10 @@
if (sk_general_init(gfd))
goto no_timestamping;

+ if(sk_set_priority(t->cfg, efd, gfd) < 0) {
+ pr_warning("Failure on setting DSCP priority.");
+ }
+
fda->fd[FD_EVENT] = efd;
fda->fd[FD_GENERAL] = gfd;
return 0;
diff -rwbBu a/udp.c b/udp.c
--- a/udp.c 2015-09-19 16:25:11.000000000 +0200
+++ b/udp.c 2016-07-06 16:39:27.000000000 +0200
@@ -186,6 +186,10 @@
if (sk_general_init(gfd))
goto no_timestamping;

+ if(sk_set_priority(t->cfg, efd, gfd) < 0) {
+ pr_warning("Failure on setting DSCP priority.");
+ }
+
fda->fd[FD_EVENT] = efd;
fda->fd[FD_GENERAL] = gfd;
return 0;
---

Best regards
Henry Jesuiter
Richard Cochran
2016-07-07 09:10:22 UTC
Permalink
The patch also needs a proper commit message explaining how these new
options are useful and a SoB tag. Except for a few minor nits, this
patch looks good to me.
Post by Jesuiter, Henry (ALC NetworX GmbH)
@@ -235,7 +237,7 @@
GLOB_ITEM_STR("uds_address", "/var/run/ptp4l"),
GLOB_ITEM_INT("use_syslog", 1, 0, 1),
GLOB_ITEM_STR("userDescription", ""),
- GLOB_ITEM_INT("verbose", 0, 0, 1),
+ GLOB_ITEM_INT("verbose", 0, 0, 1)
Please leave that comma alone.
Post by Jesuiter, Henry (ALC NetworX GmbH)
};
diff -rwbBu a/ptp4l.8 b/ptp4l.8
--- a/ptp4l.8 2015-09-19 16:25:11.000000000 +0200
+++ b/ptp4l.8 2016-07-06 16:58:23.722698053 +0200
@@ -446,6 +446,14 @@
Specifies the address of the UNIX domain socket for receiving local
management messages. The default is /var/run/ptp4l.
.TP
+.B tos_event
+Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
+event messages. Must be a value between 0 and 63. The default is 0.
+.TP
+.B tos_general
+Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
+general messages. Must be a value between 0 and 63. The default is 0.
+.TP
It would be nice to keep the numbers consistent, either always decimal
or always hexidecimal. I guess hexidecimal makes more sense (but you
know that better than me).

Also, the man page should give at least one example. What values are
most commonly used here?
Post by Jesuiter, Henry (ALC NetworX GmbH)
diff -rwbBu a/sk.c b/sk.c
--- a/sk.c 2015-09-19 16:25:11.000000000 +0200
+++ b/sk.c 2016-07-06 16:40:27.000000000 +0200
@@ -35,6 +35,7 @@
#include "missing.h"
#include "print.h"
#include "sk.h"
+#include "config.h"
/* globals */
@@ -78,6 +79,28 @@
return 0;
}
+static int set_priority(int fd, uint8_t dscp) {
Opening brace goes on a line all by itself ------^
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ int tos;
+ socklen_t tos_len;
+
+ tos_len = sizeof(tos);
+ if (getsockopt(fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
+ tos = 0;
+ }
+
+ /* clear old DSCP value */
+ tos &= ~0xFC;
+
+ /* set new DSCP value */
+ tos |= dscp<<2;
+ tos_len = sizeof(tos);
+ if (setsockopt(fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
/* public methods */
int sk_interface_index(int fd, const char *name)
@@ -105,6 +128,24 @@
return 0;
}
+int sk_set_priority(struct config *c, int event_fd, int general_fd) {
same here
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ uint8_t event_dscp = config_get_int(c, NULL, "tos_event");
+ uint8_t general_dscp = config_get_int(c, NULL, "tos_general");
Hm, does it make sense to let this option be per-port?

In that case, you want to pass the interface name instead of NULL in
argument #2.
Post by Jesuiter, Henry (ALC NetworX GmbH)
+
+ int err = 0;
+
+ if(event_dscp != 0) {
space before that (
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ err = set_priority(event_fd, event_dscp);
+ }
+
+ if(!err && general_dscp) {
ditto
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ err = set_priority(general_fd, general_dscp);
+ }
+
+ return err;
+}
diff -rwbBu a/udp6.c b/udp6.c
--- a/udp6.c 2015-09-19 16:25:11.000000000 +0200
+++ b/udp6.c 2016-07-06 16:39:48.000000000 +0200
@@ -196,6 +196,10 @@
if (sk_general_init(gfd))
goto no_timestamping;
+ if(sk_set_priority(t->cfg, efd, gfd) < 0) {
ditto
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ pr_warning("Failure on setting DSCP priority.");
+ }
+
fda->fd[FD_EVENT] = efd;
fda->fd[FD_GENERAL] = gfd;
return 0;
diff -rwbBu a/udp.c b/udp.c
--- a/udp.c 2015-09-19 16:25:11.000000000 +0200
+++ b/udp.c 2016-07-06 16:39:27.000000000 +0200
@@ -186,6 +186,10 @@
if (sk_general_init(gfd))
goto no_timestamping;
+ if(sk_set_priority(t->cfg, efd, gfd) < 0) {
ditto
Post by Jesuiter, Henry (ALC NetworX GmbH)
+ pr_warning("Failure on setting DSCP priority.");
+ }
+
fda->fd[FD_EVENT] = efd;
fda->fd[FD_GENERAL] = gfd;
return 0;
Thanks,
Richard
Jesuiter, Henry (ALC NetworX GmbH)
2016-07-07 11:42:26 UTC
Permalink
In the last years there are several media standards evolving that are relying on PTP. These standards make requirements about the DSCP priority of PTP messages. This patch introduces two new configuration options 'tos_event' and 'tos_general' to address that issue and to be able to set the DSCP priority separately for PTP event messages and PTP general messages.

Signed-off-by: Henry Jesuiter <***@alcnetworx.de>
---
diff -rwbBu a/config.c b/config.c
--- a/config.c 2015-09-19 16:25:11.000000000 +0200
+++ b/config.c 2016-07-07 13:13:44.465579808 +0200
@@ -226,6 +226,8 @@
PORT_ITEM_INT("syncReceiptTimeout", 0, 0, UINT8_MAX),
GLOB_ITEM_INT("timeSource", INTERNAL_OSCILLATOR, 0x10, 0xfe),
GLOB_ITEM_ENU("time_stamping", TS_HARDWARE, timestamping_enu),
+ GLOB_ITEM_INT("tos_event", 0, 0, 63),
+ GLOB_ITEM_INT("tos_general", 0, 0, 63),
PORT_ITEM_INT("transportSpecific", 0, 0, 0x0F),
PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
GLOB_ITEM_INT("twoStepFlag", 1, 0, 1),
diff -rwbBu a/default.cfg b/default.cfg
--- a/default.cfg 2015-09-19 16:25:11.000000000 +0200
+++ b/default.cfg 2016-07-07 13:14:21.333731697 +0200
@@ -12,6 +12,8 @@
offsetScaledLogVariance 0xFFFF
free_running 0
freq_est_interval 1
+tos_event 0
+tos_general 0
#
# Port Data Set
#
diff -rwbBu a/ptp4l.8 b/ptp4l.8
--- a/ptp4l.8 2015-09-19 16:25:11.000000000 +0200
+++ b/ptp4l.8 2016-07-07 13:04:30.601314267 +0200
@@ -446,6 +446,20 @@
Specifies the address of the UNIX domain socket for receiving local
management messages. The default is /var/run/ptp4l.
.TP
+.B tos_event
+Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
+event messages. Must be a value between 0 and 63. There are several media
+streaming standards out there that require specific values for this option.
+For example 46 (EF PHB) in AES67 or 48 (CS6 PHB) in RAVENNA. The default
+is 0.
+.TP
+.B tos_general
+Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
+general messages. Must be a value between 0 and 63. There are several media
+streaming standards out there that recommend specific values for this option.
+For example 34 (AF41 PHB) in AES67 or 46 (EF PHB) in RAVENNA. The default
+is 0.
+.TP
.B logging_level
The maximum logging level of messages which should be printed.
The default is 6 (LOG_INFO).
diff -rwbBu a/sk.c b/sk.c
--- a/sk.c 2015-09-19 16:25:11.000000000 +0200
+++ b/sk.c 2016-07-07 13:15:24.933969306 +0200
@@ -35,6 +35,7 @@
#include "missing.h"
#include "print.h"
#include "sk.h"
+#include "config.h"

/* globals */

@@ -78,6 +79,29 @@
return 0;
}

+static int set_priority(int fd, uint8_t dscp)
+{
+ int tos;
+ socklen_t tos_len;
+
+ tos_len = sizeof(tos);
+ if (getsockopt(fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
+ tos = 0;
+ }
+
+ /* clear old DSCP value */
+ tos &= ~0xFC;
+
+ /* set new DSCP value */
+ tos |= dscp<<2;
+ tos_len = sizeof(tos);
+ if (setsockopt(fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
/* public methods */

int sk_interface_index(int fd, const char *name)
@@ -105,6 +129,25 @@
return 0;
}

+int sk_set_priority(struct config *c, int event_fd, int general_fd)
+{
+ uint8_t event_dscp = config_get_int(c, NULL, "tos_event");
+ uint8_t general_dscp = config_get_int(c, NULL, "tos_general");
+
+ int err = 0;
+
+ if (event_dscp) {
+ err = set_priority(event_fd, event_dscp);
+ }
+
+ if (!err && general_dscp) {
+ err = set_priority(general_fd, general_dscp);
+ }
+
+ return err;
+}
+
+
int sk_get_ts_info(const char *name, struct sk_ts_info *sk_info)
{
#ifdef ETHTOOL_GET_TS_INFO
diff -rwbBu a/sk.h b/sk.h
--- a/sk.h 2015-09-19 16:25:11.000000000 +0200
+++ b/sk.h 2016-07-06 17:16:19.000000000 +0200
@@ -55,6 +55,16 @@
int sk_general_init(int fd);

/**
+ * Set DSCP value for socket.
+ * @param c Current linuxptp configuration.
+ * @param event_fd Open socket for ptp event messages.
+ * @param general_fd Open socket for ptp general messages.
+ * @return Zero on success, negative on failure
+ *
+ */
+int sk_set_priority(struct config *c, int event_fd, int general_fd);
+
+/**
* Obtain supported timestamping information
* @param name The name of the interface
* @param info Struct containing obtained timestamping information.
diff -rwbBu a/udp6.c b/udp6.c
--- a/udp6.c 2015-09-19 16:25:11.000000000 +0200
+++ b/udp6.c 2016-07-07 13:16:00.134088652 +0200
@@ -196,6 +196,10 @@
if (sk_general_init(gfd))
goto no_timestamping;

+ if (sk_set_priority(t->cfg, efd, gfd) < 0) {
+ pr_warning("Failure on setting DSCP priority.");
+ }
+
fda->fd[FD_EVENT] = efd;
fda->fd[FD_GENERAL] = gfd;
return 0;
diff -rwbBu a/udp.c b/udp.c
--- a/udp.c 2015-09-19 16:25:11.000000000 +0200
+++ b/udp.c 2016-07-07 13:15:48.358049636 +0200
@@ -186,6 +186,10 @@
if (sk_general_init(gfd))
goto no_timestamping;

+ if (sk_set_priority(t->cfg, efd, gfd) < 0) {
+ pr_warning("Failure on setting DSCP priority.");
+ }
+
fda->fd[FD_EVENT] = efd;
fda->fd[FD_GENERAL] = gfd;
return 0;

---

Some notes to the patch above:
1. I finally decided to use the decimal DSCP value. The RFCs are always using the PHB name or binary representation. On the other hand in my experience you will find more often a decimal than a hexadecimal representation. At least in the audio community - the foremost target audience of this patch.
2. Actually that config options could be done on per port basis. Currently we decided explicitly against this, since a device in a streaming network should not be able to use different priorities for pTP messages on different ports to avoid misconfiguration. It would be dangerous, if in a streaming network by some misconfiguration there is the possibility to suppress PTP event messages by media streams, which finally could lead to media drop outs. Of course, if you prefer a per port option we will implement it that way.

Best regards
Henry Jesuiter
Jesuiter, Henry (ALC NetworX GmbH)
2016-07-07 12:01:13 UTC
Permalink
In the last years there are several media streaming standards evolving that are relying on PTP. These standards make requirements about the DSCP priority of PTP messages. This patch introduces two new configuration options 'tos_event' and 'tos_general' to address that issue and to be able to set the DSCP priority separately for PTP event messages and PTP general messages.

Signed-off-by: Henry Jesuiter <***@alcnetworx.de>
---
diff -rwbBu a/config.c b/config.c
--- a/config.c 2015-09-19 16:25:11.000000000 +0200
+++ b/config.c 2016-07-07 13:13:44.465579808 +0200
@@ -226,6 +226,8 @@
PORT_ITEM_INT("syncReceiptTimeout", 0, 0, UINT8_MAX),
GLOB_ITEM_INT("timeSource", INTERNAL_OSCILLATOR, 0x10, 0xfe),
GLOB_ITEM_ENU("time_stamping", TS_HARDWARE, timestamping_enu),
+ GLOB_ITEM_INT("tos_event", 0, 0, 63),
+ GLOB_ITEM_INT("tos_general", 0, 0, 63),
PORT_ITEM_INT("transportSpecific", 0, 0, 0x0F),
PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
GLOB_ITEM_INT("twoStepFlag", 1, 0, 1),
diff -rwbBu a/default.cfg b/default.cfg
--- a/default.cfg 2015-09-19 16:25:11.000000000 +0200
+++ b/default.cfg 2016-07-07 13:14:21.333731697 +0200
@@ -12,6 +12,8 @@
offsetScaledLogVariance 0xFFFF
free_running 0
freq_est_interval 1
+tos_event 0
+tos_general 0
#
# Port Data Set
#
diff -rwbBu a/ptp4l.8 b/ptp4l.8
--- a/ptp4l.8 2015-09-19 16:25:11.000000000 +0200
+++ b/ptp4l.8 2016-07-07 13:04:30.601314267 +0200
@@ -446,6 +446,20 @@
Specifies the address of the UNIX domain socket for receiving local
management messages. The default is /var/run/ptp4l.
.TP
+.B tos_event
+Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
+event messages. Must be a value between 0 and 63. There are several media
+streaming standards out there that require specific values for this option.
+For example 46 (EF PHB) in AES67 or 48 (CS6 PHB) in RAVENNA. The default
+is 0.
+.TP
+.B tos_general
+Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
+general messages. Must be a value between 0 and 63. There are several media
+streaming standards out there that recommend specific values for this option.
+For example 34 (AF41 PHB) in AES67 or 46 (EF PHB) in RAVENNA. The default
+is 0.
+.TP
.B logging_level
The maximum logging level of messages which should be printed.
The default is 6 (LOG_INFO).
diff -rwbBu a/sk.c b/sk.c
--- a/sk.c 2015-09-19 16:25:11.000000000 +0200
+++ b/sk.c 2016-07-07 13:15:24.933969306 +0200
@@ -35,6 +35,7 @@
#include "missing.h"
#include "print.h"
#include "sk.h"
+#include "config.h"

/* globals */

@@ -78,6 +79,29 @@
return 0;
}

+static int set_priority(int fd, uint8_t dscp)
+{
+ int tos;
+ socklen_t tos_len;
+
+ tos_len = sizeof(tos);
+ if (getsockopt(fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
+ tos = 0;
+ }
+
+ /* clear old DSCP value */
+ tos &= ~0xFC;
+
+ /* set new DSCP value */
+ tos |= dscp<<2;
+ tos_len = sizeof(tos);
+ if (setsockopt(fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
/* public methods */

int sk_interface_index(int fd, const char *name)
@@ -105,6 +129,25 @@
return 0;
}

+int sk_set_priority(struct config *c, int event_fd, int general_fd)
+{
+ uint8_t event_dscp = config_get_int(c, NULL, "tos_event");
+ uint8_t general_dscp = config_get_int(c, NULL, "tos_general");
+
+ int err = 0;
+
+ if (event_dscp) {
+ err = set_priority(event_fd, event_dscp);
+ }
+
+ if (!err && general_dscp) {
+ err = set_priority(general_fd, general_dscp);
+ }
+
+ return err;
+}
+
+
int sk_get_ts_info(const char *name, struct sk_ts_info *sk_info)
{
#ifdef ETHTOOL_GET_TS_INFO
diff -rwbBu a/sk.h b/sk.h
--- a/sk.h 2015-09-19 16:25:11.000000000 +0200
+++ b/sk.h 2016-07-06 17:16:19.000000000 +0200
@@ -55,6 +55,16 @@
int sk_general_init(int fd);

/**
+ * Set DSCP value for socket.
+ * @param c Current linuxptp configuration.
+ * @param event_fd Open socket for ptp event messages.
+ * @param general_fd Open socket for ptp general messages.
+ * @return Zero on success, negative on failure
+ *
+ */
+int sk_set_priority(struct config *c, int event_fd, int general_fd);
+
+/**
* Obtain supported timestamping information
* @param name The name of the interface
* @param info Struct containing obtained timestamping information.
diff -rwbBu a/udp6.c b/udp6.c
--- a/udp6.c 2015-09-19 16:25:11.000000000 +0200
+++ b/udp6.c 2016-07-07 13:16:00.134088652 +0200
@@ -196,6 +196,10 @@
if (sk_general_init(gfd))
goto no_timestamping;

+ if (sk_set_priority(t->cfg, efd, gfd) < 0) {
+ pr_warning("Failure on setting DSCP priority.");
+ }
+
fda->fd[FD_EVENT] = efd;
fda->fd[FD_GENERAL] = gfd;
return 0;
diff -rwbBu a/udp.c b/udp.c
--- a/udp.c 2015-09-19 16:25:11.000000000 +0200
+++ b/udp.c 2016-07-07 13:15:48.358049636 +0200
@@ -186,6 +186,10 @@
if (sk_general_init(gfd))
goto no_timestamping;

+ if (sk_set_priority(t->cfg, efd, gfd) < 0) {
+ pr_warning("Failure on setting DSCP priority.");
+ }
+
fda->fd[FD_EVENT] = efd;
fda->fd[FD_GENERAL] = gfd;
return 0;

---

Some notes to the patch above:
1. I finally decided to use the decimal DSCP value. The RFCs are always using the PHB name or binary representation. On the other hand in my experience you will find more often a decimal than a hexadecimal representation. At least in the audio community - the foremost target audience of this patch.
2. Actually that config options could be done on per port basis. Currently we decided explicitly against this, since a device in a streaming network should not be able to use different priorities for pTP messages on different ports to avoid misconfiguration. It would be dangerous, if in a streaming network by some misconfiguration there is the possibility to suppress PTP event messages by media streams, which finally could lead to media drop outs. Of course, if you prefer a per port option we will implement it that way.

Best regards
Henry Jesuiter

PS: Sorry, for some reason, the last mail contained an incomplete patch, so please use the one in this mail.
Richard Cochran
2016-07-07 17:41:47 UTC
Permalink
Henry,

This is patch is *almost* ready for merging...

This patch causes a build failure:

gcc phc_ctl.o phc.o sk.o util.o clockadj.o sysoff.o print.o version.o -lm -lrt -o phc_ctl
sk.o: In function `sk_set_priority':
sk.c:(.text+0x26e): undefined reference to `config_get_int'
sk.c:(.text+0x286): undefined reference to `config_get_int'
collect2: error: ld returned 1 exit status
<builtin>: recipe for target 'phc_ctl' failed
make: *** [phc_ctl] Error 1

(This is easy to fix in the makefile.)

The subject line should look like this:

Subject: [PATCH] Introduce options to set DSCP values in PTP messages.

Then, the description should be formatted as a paragraph, not all on
Post by Jesuiter, Henry (ALC NetworX GmbH)
In the last years there are several media streaming standards evolving that are relying on PTP. These standards make requirements about the DSCP priority of PTP messages. This patch introduces two new configuration options 'tos_event' and 'tos_general' to address that issue and to be able to set the DSCP priority separately for PTP event messages and PTP general messages.
but rather like this:
---
In the last years there are several media streaming standards
evolving that are relying on PTP. These standards make requirements
about the DSCP priority of PTP messages. This patch introduces two
new configuration options 'tos_event' and 'tos_general' to address
that issue and to be able to set the DSCP priority separately for
PTP event messages and PTP general messages.
---
Post by Jesuiter, Henry (ALC NetworX GmbH)
diff -rwbBu a/ptp4l.8 b/ptp4l.8
--- a/ptp4l.8 2015-09-19 16:25:11.000000000 +0200
+++ b/ptp4l.8 2016-07-07 13:04:30.601314267 +0200
@@ -446,6 +446,20 @@
Specifies the address of the UNIX domain socket for receiving local
management messages. The default is /var/run/ptp4l.
.TP
+.B tos_event
+Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
+event messages. Must be a value between 0 and 63. There are several media
+streaming standards out there that require specific values for this option.
+For example 46 (EF PHB) in AES67 or 48 (CS6 PHB) in RAVENNA. The default
+is 0.
+.TP
+.B tos_general
+Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
+general messages. Must be a value between 0 and 63. There are several media
+streaming standards out there that recommend specific values for this option.
+For example 34 (AF41 PHB) in AES67 or 46 (EF PHB) in RAVENNA. The default
+is 0.
+.TP
The descriptions in the man page are good now, thanks. Please avoid
adding trailing white space.
Post by Jesuiter, Henry (ALC NetworX GmbH)
diff -rwbBu a/sk.c b/sk.c
--- a/sk.c 2015-09-19 16:25:11.000000000 +0200
+++ b/sk.c 2016-07-07 13:15:24.933969306 +0200
@@ -35,6 +35,7 @@
#include "missing.h"
#include "print.h"
#include "sk.h"
+#include "config.h"
Here, please keep the #include directives in alphabetical order.
Post by Jesuiter, Henry (ALC NetworX GmbH)
2. Actually that config options could be done on per port
basis. Currently we decided explicitly against this, since a device
in a streaming network should not be able to use different
priorities for pTP messages on different ports to avoid
misconfiguration. It would be dangerous, if in a streaming network
by some misconfiguration there is the possibility to suppress PTP
event messages by media streams, which finally could lead to media
drop outs. Of course, if you prefer a per port option we will
implement it that way.
What about the following use case? Maybe someone wants to run ptp4l
as a BC between a normal 1588 network and an audio network. Several
of the other options are per-port in order to allow this kind of
bridging. For example, you can have a BC with one port UDP/IPv4 and
another port Layer-2. Or you can connect E2E and P2P networks
together.

I don't have a strong opinion, and I think the patch is ok as is. We
can always change to a port option later on...

Thanks,
Richard

Loading...