Re: [Linuxptp-devel] [PATCH 2/2] pmc: Support querying TLV_PORT_PROPERTIES_NP
Jiri Benc writes: > On Mon, 16 Sep 2019 09:56:29 +0000, Petr Machata wrote: >> Inside the namespace you can do "ip netns id" to get the namespace that >> you are in. > > Beware that this netnsid is only valid in the net name space that the > 'ip' command ran in. It is not a global identifier and it is not valid > in another net name space. > > In other words, ptp4l can't return a netnsid valid in the pmc net > name space (without doing gross and unreliable hacks). I was actually thinking about /var/run/netns being mounted elsewhere or some such, which would prevent "ip netns id" from working. The symbolic names seem to be accessible from other namespaces just fine --"ip netns exec foo ip netns exec bar bash" works-- but renaming /var/run breaks it. I'm probably just missing something, I haven't worked with network namespaces much. >> ptp4l already has a public API to obtain the device name: the VLA >> itself. There just isn't an easy way to invoke that interface. So we >> don't have to concern ourselves with whether to hide the interface name >> itself, that ship has sailed. > > If the API is so hard that it's not in use in practice, it makes a > difference. Yeah, conceded, random admins probably wouldn't do this. >> About the namespace--as an unprivileged process you can't just ask about >> netns ids of random processes. So whether to allow ptp4l to give up this >> information is a good question, because that might go against the >> intentions of the admin of the machine. > > It also does not work, see above :-) Agreed. >> I'm not sure I agree the process inside the namespace should be >> disallowed from knowing it is. I'm not really well versed in network >> namespaces, but looking at what "ip netns id" is doing under the hood it >> looks like the kernel has interfaces to just tell you (RTM_GETNSID). > > Since every process is in a net name space (and exactly one net name > space), it already knows by definition that it is in a name space :-) > > Whether referencing into other net name spaces makes sense or not, > depends on the use. I'd argue that a daemon (such as ptp4l) should not > mess with name spaces, as it needs the administrator to be able to run > it in whatever net name space they chooses. But it can certainly query > it if it is needed for proper interoperability. > >> Maybe just documenting the issue is all that can be done. > > Could be. I don't have other ideas than this. I also think that having an easy to use way to access the interface outweighs the risks. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] pmc: Support querying TLV_PORT_PROPERTIES_NP
Jiri Benc writes: > On Mon, 16 Sep 2019 14:04:00 +0000, Petr Machata wrote: >> I was actually thinking about /var/run/netns being mounted elsewhere or >> some such, which would prevent "ip netns id" from working. The symbolic >> names seem to be accessible from other namespaces just fine --"ip netns >> exec foo ip netns exec bar bash" works-- but renaming /var/run breaks >> it. > > /var/run/netns is a convention introduced by iproute2 and respected by > many, but sadly not all, other net management systems. There is no > concept of netns names in the kernel. > > And it is a completely different thing than netnsids. The netns mounts > allow you to get a file descriptor referencing the target net name > space unambiguously. Such fd can be used for many operations, most > importantly to switch yourself into that net name space (see setns(2)). > You can obtain the fd by other means, too, for example by > opening /proc//ns/net. > > netnsid is a different integer. It is valid only in the net name space > it was created in and references a target net name space. It can be > used only in netlink operations. It is completely managed by the kernel > and does not require the netns to be mounted nor /proc to be mounted. Thanks for clearing this up for me. I saw the netnsid thing in the man but didn't know what it was for. >> I don't have other ideas than this. I also think that having an easy to >> use way to access the interface outweighs the risks. > > I think proper documentation might help. Add a warning that this > doesn't work across net name spaces and ensure that the API is flexible > enough to allow a new field to be added in the future without breaking > backwards compatibility if we ever need to start communicating also a > netns identifier. So you want to change the current TLV? I guess that's reasonable, since it is essentially an internal API. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] pmc: Support querying TLV_PORT_PROPERTIES_NP
Keller, Jacob E writes: >> From: Petr Machata [mailto:pe...@mellanox.com] >> Sent: Friday, September 13, 2019 7:58 AM >> To: Jiri Benc >> Cc: linuxptp-devel@lists.sourceforge.net >> Subject: Re: [Linuxptp-devel] [PATCH 2/2] pmc: Support querying >> TLV_PORT_PROPERTIES_NP >> >> Jiri Benc writes: >> >> > pmc is different. It's not tied to the service start time, it's used by >> > administrators and by various scripts from varying environment. While >> > you can reasonably expect that ptp4l and phc2sys will be run in the >> > same name space, it's not necessarily the case for pmc. >> >> Hmm, and as an end user I can't do `ip netns identify $(pidof ptp4l)` >> even if I know about this issue. >> >> Maybe ptp4l could include the namespace name in the VLA response. Are >> the messages considered a stable API? >> > > Is the namespacing information supposed to be kept secure from > processes inside the namespacing? I would want to prevent any sort of > leak of namespace info in that case. ptp4l won't be able to tell the > request came from within a namespace either. Inside the namespace you can do "ip netns id" to get the namespace that you are in. > Hmm. It's plausible the pmc instance wouldn't even know about the > device, but ptp4l itself doesn't know whether to hide that info or > not. Other way around could be that ptp4l is inside a namespace while > pmc is not, but again it's not obvious. ptp4l already has a public API to obtain the device name: the VLA itself. There just isn't an easy way to invoke that interface. So we don't have to concern ourselves with whether to hide the interface name itself, that ship has sailed. About the namespace--as an unprivileged process you can't just ask about netns ids of random processes. So whether to allow ptp4l to give up this information is a good question, because that might go against the intentions of the admin of the machine. Maybe just documenting the issue is all that can be done. > I'm not sure what the best practice here is. Yeah, I didn't even think about this until Jiri brought it up. >> Alternatively I might mention the issue in the man page. >> >> Not being able to tell which of the PTP port identifiers represents >> which actual interface is something of a problem if you have more than >> about two of them. > > Right, but part of namespacing is that processes inside the namespace > should generally not know they are, and not be aware of anything > outside the namespace... I'm not sure I agree the process inside the namespace should be disallowed from knowing it is. I'm not really well versed in network namespaces, but looking at what "ip netns id" is doing under the hood it looks like the kernel has interfaces to just tell you (RTM_GETNSID). Thanks, Petr ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] pmc: Support querying TLV_PORT_PROPERTIES_NP
Jiri Benc writes: > pmc is different. It's not tied to the service start time, it's used by > administrators and by various scripts from varying environment. While > you can reasonably expect that ptp4l and phc2sys will be run in the > same name space, it's not necessarily the case for pmc. Hmm, and as an end user I can't do `ip netns identify $(pidof ptp4l)` even if I know about this issue. Maybe ptp4l could include the namespace name in the VLA response. Are the messages considered a stable API? Alternatively I might mention the issue in the man page. Not being able to tell which of the PTP port identifiers represents which actual interface is something of a problem if you have more than about two of them. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] pmc: Support querying TLV_PORT_PROPERTIES_NP
Jiri Benc writes: > On Thu, 12 Sep 2019 11:06:30 +0000, Petr Machata wrote: >> +text2str(>interface)); > > How does this work when net name spaces are in use? There's no guarantee > that pmc is run in the same net name space where ptp4l is running. The > returned interface name may correspond to a completely different > interface in the pmc net name space. I would guess the same problem applies when phc2sys uses the interface. Why is it not an issue there? ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH 2/2] pmc: Support querying TLV_PORT_PROPERTIES_NP
TLV_PORT_PROPERTIES_NP messages serve for querying of port properties, such
as timestamp type and, prominently, netdevice name associated with the
port. pmc however does not support this query, which makes it difficult to
access this information e.g. from scripts. Add this support to pmc.
Signed-off-by: Mykola Zhuravel
Signed-off-by: Petr Machata
---
pmc.8| 2 ++
pmc.c| 16
pmc_common.c | 1 +
3 files changed, 19 insertions(+)
diff --git a/pmc.8 b/pmc.8
index acf2d90..e0ab5ac 100644
--- a/pmc.8
+++ b/pmc.8
@@ -193,6 +193,8 @@ The MAC address to which PTP management messages should be
sent. Relevant only w
.TP
.B PORT_DATA_SET_NP
.TP
+.B PORT_PROPERTIES_NP
+.TP
.B PORT_STATS_NP
.TP
.B PRIORITY1
diff --git a/pmc.c b/pmc.c
index 868fc2a..4e6043b 100644
--- a/pmc.c
+++ b/pmc.c
@@ -69,6 +69,7 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
struct tlv_extra *extra;
struct portDS *p;
struct port_ds_np *pnp;
+ struct port_properties_np *ppn;
struct port_stats_np *pcp;
if (msg_type(msg) != MANAGEMENT) {
@@ -323,6 +324,21 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
pnp->neighborPropDelayThresh,
pnp->asCapable ? 1 : 0);
break;
+ case TLV_PORT_PROPERTIES_NP:
+ ppn = (struct port_properties_np *) mgt->data;
+ if (ppn->port_state > PS_SLAVE) {
+ ppn->port_state = 0;
+ }
+ fprintf(fp, "PORT_PROPERTIES_NP "
+ IFMT "portIdentity%s"
+ IFMT "portState %s"
+ IFMT "timestamping%s"
+ IFMT "interface %s",
+ pid2str(>portIdentity),
+ ps_str[ppn->port_state],
+ ts_str(ppn->timestamping),
+ text2str(>interface));
+ break;
case TLV_PORT_STATS_NP:
pcp = (struct port_stats_np *) mgt->data;
fprintf(fp, "PORT_STATS_NP "
diff --git a/pmc_common.c b/pmc_common.c
index 592cc93..46aac30 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -121,6 +121,7 @@ struct management_id idtab[] = {
{ "LOG_MIN_PDELAY_REQ_INTERVAL", TLV_LOG_MIN_PDELAY_REQ_INTERVAL,
do_get_action },
{ "PORT_DATA_SET_NP", TLV_PORT_DATA_SET_NP, do_set_action },
{ "PORT_STATS_NP", TLV_PORT_STATS_NP, do_get_action },
+ { "PORT_PROPERTIES_NP", TLV_PORT_PROPERTIES_NP, do_get_action },
};
static void do_get_action(struct pmc *pmc, int action, int index, char *str)
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH 0/2] pmc: Support querying TLV_PORT_PROPERTIES_NP
In commit 424bbde8fcca ("Custom management TLV PORT_PROPERTIES_NP"), a new
TLV was added that allows introspection into PTP ports, and in particular
translation from PTP port identifiers to system netdevice names. The TLV
was added for phc2sys, and correspondingly pmc support was not added.
However, translating from port IDs to netdevices is as useful to phc2sys as
to end users. Besides just visually correlating what's what also for ad-hoc
scripting and writing automated tests.
This patchset therefore extends pmc to understand this TLV. In patch #1, a
new helper is added that allows human-readable rendering of timestamping
type, which is one of the reported properties. Patch #2 then extends pmc.
Petr Machata (2):
util: Add a function to render timestamp type
pmc: Support querying TLV_PORT_PROPERTIES_NP
pmc.8| 2 ++
pmc.c| 16
pmc_common.c | 1 +
util.c | 18 ++
util.h | 7 +++
5 files changed, 44 insertions(+)
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH 1/2] util: Add a function to render timestamp type
TLV_PORT_PROPERTIES_NP carries, among other attributes, a timestamp type
used for that port. In order to make it possible to format the value for
user consumption, introduce a new function ts_str().
Signed-off-by: Petr Machata
---
util.c | 18 ++
util.h | 7 +++
2 files changed, 25 insertions(+)
diff --git a/util.c b/util.c
index 833f1a5..e64a93d 100644
--- a/util.c
+++ b/util.c
@@ -70,6 +70,24 @@ const char *ev_str[] = {
"RS_PASSIVE",
};
+const char *ts_str(enum timestamp_type ts)
+{
+ switch (ts) {
+ case TS_SOFTWARE:
+ return "SOFTWARE";
+ case TS_HARDWARE:
+ return "HARDWARE";
+ case TS_LEGACY_HW:
+ return "LEGACY_HW";
+ case TS_ONESTEP:
+ return "ONESTEP";
+ case TS_P2P1STEP:
+ return "P2P1STEP";
+ }
+
+ return "???";
+}
+
int addreq(enum transport_type type, struct address *a, struct address *b)
{
void *bufa, *bufb;
diff --git a/util.h b/util.h
index 9d3f227..60d28ac 100644
--- a/util.h
+++ b/util.h
@@ -41,6 +41,13 @@ extern const char *ps_str[];
*/
extern const char *ev_str[];
+/**
+ * Gets a human-readable string for a given timestamp type.
+ * @param tsTimestamp type.
+ * @return Human-readable rendering if TS is valid, otherwise "???".
+ */
+const char *ts_str(enum timestamp_type ts);
+
/**
* Compares two binary addresses for equality.
* @param type One of the enumerated transport types.
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH] pmc.8: Mention PORT_STATS_NP
In commit 2b5bec8d2740 ("pmc: Add a new TLV to obtain per-port
statistics"), pmc gained a new TLV to obtain port stats from ptp4l. Mention
the TLV in the pmc man page.
Signed-off-by: Petr Machata
---
pmc.8 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/pmc.8 b/pmc.8
index ff85cd6..acf2d90 100644
--- a/pmc.8
+++ b/pmc.8
@@ -193,6 +193,8 @@ The MAC address to which PTP management messages should be
sent. Relevant only w
.TP
.B PORT_DATA_SET_NP
.TP
+.B PORT_STATS_NP
+.TP
.B PRIORITY1
.TP
.B PRIORITY2
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 0/2] Introduce per-port stats for received and transmitted messages
Keller, Jacob E writes: > I know we have the pmc man page Gah, and I forgot to update it with the new stats message. I'll do that shortly. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v2 1/2] port: Introduce per-port stats for received and transmitted messages
Add struct PortStats to keep per-port number of messages sent and received,
split by message type. Bump TX counters after messages are sent
successfully, and RX counters after a message is received. To keep things
simple, reserve one counter for each theoretically possible message type,
including the reserved ones.
Signed-off-by: Petr Machata
---
Notes:
v2:
- Add MAX_MESSAGE_TYPES with comment instead of using a bare constant.
ddt.h | 7 +++
port.c | 25 +++--
port_private.h | 1 +
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/ddt.h b/ddt.h
index 4acaa4f..56449a3 100644
--- a/ddt.h
+++ b/ddt.h
@@ -100,4 +100,11 @@ struct FaultRecord {
struct PTPText faultDescription;
};
+/* Four bits are dedicated to messageType field */
+#define MAX_MESSAGE_TYPES 16
+struct PortStats {
+ uint64_t rxMsgType[MAX_MESSAGE_TYPES];
+ uint64_t txMsgType[MAX_MESSAGE_TYPES];
+};
+
#endif
diff --git a/port.c b/port.c
index 5a4a116..471e6f4 100644
--- a/port.c
+++ b/port.c
@@ -580,6 +580,16 @@ static int path_trace_ignore(struct port *p, struct
ptp_message *m)
return 0;
}
+static void port_stats_inc_rx(struct port *p, const struct ptp_message *msg)
+{
+ p->stats.rxMsgType[msg_type(msg)]++;
+}
+
+static void port_stats_inc_tx(struct port *p, const struct ptp_message *msg)
+{
+ p->stats.txMsgType[msg_type(msg)]++;
+}
+
static int peer_prepare_and_send(struct port *p, struct ptp_message *msg,
enum transport_event event)
{
@@ -595,6 +605,7 @@ static int peer_prepare_and_send(struct port *p, struct
ptp_message *msg,
if (cnt <= 0) {
return -1;
}
+ port_stats_inc_tx(p, msg);
if (msg_sots_valid(msg)) {
ts_add(>hwts.ts, p->tx_timestamp_offset);
}
@@ -2627,6 +2638,7 @@ static enum fsm_event bc_event(struct port *p, int
fd_index)
msg_put(msg);
return EV_NONE;
}
+ port_stats_inc_rx(p, msg);
if (port_ignore(p, msg)) {
msg_put(msg);
return EV_NONE;
@@ -2691,14 +2703,22 @@ int port_forward(struct port *p, struct ptp_message
*msg)
{
int cnt;
cnt = transport_send(p->trp, >fda, TRANS_GENERAL, msg);
- return cnt <= 0 ? -1 : 0;
+ if (cnt <= 0) {
+ return -1;
+ }
+ port_stats_inc_tx(p, msg);
+ return 0;
}
int port_forward_to(struct port *p, struct ptp_message *msg)
{
int cnt;
cnt = transport_sendto(p->trp, >fda, TRANS_GENERAL, msg);
- return cnt <= 0 ? -1 : 0;
+ if (cnt <= 0) {
+ return -1;
+ }
+ port_stats_inc_tx(p, msg);
+ return 0;
}
int port_prepare_and_send(struct port *p, struct ptp_message *msg,
@@ -2717,6 +2737,7 @@ int port_prepare_and_send(struct port *p, struct
ptp_message *msg,
if (cnt <= 0) {
return -1;
}
+ port_stats_inc_tx(p, msg);
if (msg_sots_valid(msg)) {
ts_add(>hwts.ts, p->tx_timestamp_offset);
}
diff --git a/port_private.h b/port_private.h
index 9a5022d..5789fbb 100644
--- a/port_private.h
+++ b/port_private.h
@@ -139,6 +139,7 @@ struct port {
struct fault_interval flt_interval_pertype[FT_CNT];
enum fault_type last_fault_type;
unsigned intversionNumber; /*UInteger4*/
+ struct PortStatsstats;
/* foreignMasterDS */
LIST_HEAD(fm, foreign_clock) foreign_masters;
/* TC book keeping */
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH] udp6: Make mc6_addr transport-local
mc6_addr holds the parsed multicast address to which messages should be sent.
But since each port can have a different scope, and the primary MC address
depends on the scope, it really can't be stored in a global variable. Move both
to struct udp6.
Additionally, document the fact that the primary multicast address is changed at
runtime.
Reported-by: Alex Veber
Signed-off-by: Petr Machata
---
udp6.c | 28 +---
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/udp6.c b/udp6.c
index 908f307..74ebc7f 100644
--- a/udp6.c
+++ b/udp6.c
@@ -39,14 +39,19 @@
#define EVENT_PORT319
#define GENERAL_PORT 320
+
+/* The 0x0e in second byte is substituted with udp6_scope at runtime. */
#define PTP_PRIMARY_MCAST_IP6ADDR "FF0E:0:0:0:0:0:0:181"
#define PTP_PDELAY_MCAST_IP6ADDR "FF02:0:0:0:0:0:0:6B"
+enum { MC_PRIMARY, MC_PDELAY };
+
struct udp6 {
struct transport t;
int index;
struct address ip;
struct address mac;
+ struct in6_addr mc6_addr[2];
};
static int is_link_local(struct in6_addr *addr)
@@ -155,10 +160,6 @@ no_socket:
return -1;
}
-enum { MC_PRIMARY, MC_PDELAY };
-
-static struct in6_addr mc6_addr[2];
-
static int udp6_open(struct transport *t, struct interface *iface,
struct fdarray *fda, enum timestamp_type ts_type)
{
@@ -174,19 +175,24 @@ static int udp6_open(struct transport *t, struct
interface *iface,
udp6->ip.len = 0;
sk_interface_addr(name, AF_INET6, >ip);
- if (1 != inet_pton(AF_INET6, PTP_PRIMARY_MCAST_IP6ADDR,
_addr[MC_PRIMARY]))
+ if (1 != inet_pton(AF_INET6, PTP_PRIMARY_MCAST_IP6ADDR,
+ >mc6_addr[MC_PRIMARY]))
return -1;
- mc6_addr[MC_PRIMARY].s6_addr[1] = config_get_int(t->cfg, name,
"udp6_scope");
+ udp6->mc6_addr[MC_PRIMARY].s6_addr[1] = config_get_int(t->cfg, name,
+ "udp6_scope");
- if (1 != inet_pton(AF_INET6, PTP_PDELAY_MCAST_IP6ADDR,
_addr[MC_PDELAY]))
+ if (1 != inet_pton(AF_INET6, PTP_PDELAY_MCAST_IP6ADDR,
+ >mc6_addr[MC_PDELAY]))
return -1;
- efd = open_socket_ipv6(name, mc6_addr, EVENT_PORT, >index,
hop_limit);
+ efd = open_socket_ipv6(name, udp6->mc6_addr, EVENT_PORT, >index,
+ hop_limit);
if (efd < 0)
goto no_event;
- gfd = open_socket_ipv6(name, mc6_addr, GENERAL_PORT, >index,
hop_limit);
+ gfd = open_socket_ipv6(name, udp6->mc6_addr, GENERAL_PORT, >index,
+ hop_limit);
if (gfd < 0)
goto no_general;
@@ -249,8 +255,8 @@ static int udp6_send(struct transport *t, struct fdarray
*fda,
if (!addr) {
memset(_buf, 0, sizeof(addr_buf));
addr_buf.sin6.sin6_family = AF_INET6;
- addr_buf.sin6.sin6_addr = peer ? mc6_addr[MC_PDELAY] :
- mc6_addr[MC_PRIMARY];
+ addr_buf.sin6.sin6_addr = peer ? udp6->mc6_addr[MC_PDELAY] :
+ udp6->mc6_addr[MC_PRIMARY];
if (is_link_local(_buf.sin6.sin6_addr))
addr_buf.sin6.sin6_scope_id = udp6->index;
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v2 0/2] Introduce per-port stats for received and transmitted messages
Black-box switches with PTP support commonly provide per-port statistics of number of messages sent and received, split by the message type. Like other statistics (ip link, ethtool, etc. etc.), network operators use the PTP message stats to monitor the (PTP) network and debug issues. When ptp4l is used to turn a Linux machine (be it a switch or a host) into a PTP clock, there is no easy way to get at these stats. It would certainly be possible to parse ingressing and egressing traffic using e.g. a u32 classifier, or create an ad-hoc eBPF-based tool, or something similar. But all these approaches have to work hard to extract the knowledge that ptp4l already has. ptp4l needs to parse the traffic anyway, and for transmitted packets obviously knows what it is sending. It is thus the natural place to place the stats. To that end, patch #1 introduces the message stats into linuxptp. A logical way to obtain these stats is then through pmc, which is implemented in patch #2, by way of a new TLV type. v2: - Patch #1: - Add MAX_MESSAGE_TYPES with comment instead of using a bare constant. Petr Machata (2): port: Introduce per-port stats for received and transmitted messages pmc: Add a new TLV to obtain per-port statistics ddt.h | 7 +++ pmc.c | 47 +++ pmc_common.c | 1 + port.c | 32 ++-- port_private.h | 1 + tlv.c | 15 +++ tlv.h | 6 ++ 7 files changed, 107 insertions(+), 2 deletions(-) -- 2.20.1 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v2 2/2] pmc: Add a new TLV to obtain per-port statistics
Add an ability of pmc to query per-port stats added in the previous patch.
Signed-off-by: Petr Machata
---
pmc.c| 47 +++
pmc_common.c | 1 +
port.c | 7 +++
tlv.c| 15 +++
tlv.h| 6 ++
5 files changed, 76 insertions(+)
diff --git a/pmc.c b/pmc.c
index 440c905..868fc2a 100644
--- a/pmc.c
+++ b/pmc.c
@@ -69,6 +69,7 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
struct tlv_extra *extra;
struct portDS *p;
struct port_ds_np *pnp;
+ struct port_stats_np *pcp;
if (msg_type(msg) != MANAGEMENT) {
return;
@@ -322,6 +323,52 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
pnp->neighborPropDelayThresh,
pnp->asCapable ? 1 : 0);
break;
+ case TLV_PORT_STATS_NP:
+ pcp = (struct port_stats_np *) mgt->data;
+ fprintf(fp, "PORT_STATS_NP "
+ IFMT "portIdentity %s"
+ IFMT "rx_Sync %" PRIu64
+ IFMT "rx_Delay_Req %" PRIu64
+ IFMT "rx_Pdelay_Req %" PRIu64
+ IFMT "rx_Pdelay_Resp%" PRIu64
+ IFMT "rx_Follow_Up %" PRIu64
+ IFMT "rx_Delay_Resp %" PRIu64
+ IFMT "rx_Pdelay_Resp_Follow_Up %" PRIu64
+ IFMT "rx_Announce %" PRIu64
+ IFMT "rx_Signaling %" PRIu64
+ IFMT "rx_Management %" PRIu64
+ IFMT "tx_Sync %" PRIu64
+ IFMT "tx_Delay_Req %" PRIu64
+ IFMT "tx_Pdelay_Req %" PRIu64
+ IFMT "tx_Pdelay_Resp%" PRIu64
+ IFMT "tx_Follow_Up %" PRIu64
+ IFMT "tx_Delay_Resp %" PRIu64
+ IFMT "tx_Pdelay_Resp_Follow_Up %" PRIu64
+ IFMT "tx_Announce %" PRIu64
+ IFMT "tx_Signaling %" PRIu64
+ IFMT "tx_Management %" PRIu64,
+ pid2str(>portIdentity),
+ pcp->stats.rxMsgType[SYNC],
+ pcp->stats.rxMsgType[DELAY_REQ],
+ pcp->stats.rxMsgType[PDELAY_REQ],
+ pcp->stats.rxMsgType[PDELAY_RESP],
+ pcp->stats.rxMsgType[FOLLOW_UP],
+ pcp->stats.rxMsgType[DELAY_RESP],
+ pcp->stats.rxMsgType[PDELAY_RESP_FOLLOW_UP],
+ pcp->stats.rxMsgType[ANNOUNCE],
+ pcp->stats.rxMsgType[SIGNALING],
+ pcp->stats.rxMsgType[MANAGEMENT],
+ pcp->stats.txMsgType[SYNC],
+ pcp->stats.txMsgType[DELAY_REQ],
+ pcp->stats.txMsgType[PDELAY_REQ],
+ pcp->stats.txMsgType[PDELAY_RESP],
+ pcp->stats.txMsgType[FOLLOW_UP],
+ pcp->stats.txMsgType[DELAY_RESP],
+ pcp->stats.txMsgType[PDELAY_RESP_FOLLOW_UP],
+ pcp->stats.txMsgType[ANNOUNCE],
+ pcp->stats.txMsgType[SIGNALING],
+ pcp->stats.txMsgType[MANAGEMENT]);
+ break;
case TLV_LOG_ANNOUNCE_INTERVAL:
mtd = (struct management_tlv_datum *) mgt->data;
fprintf(fp, "LOG_ANNOUNCE_INTERVAL "
diff --git a/pmc_common.c b/pmc_common.c
index 4d48e3a..592cc93 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -120,6 +120,7 @@ struct management_id idtab[] = {
{ "DELAY_MECHANISM", TLV_DELAY_MECHANISM, do_get_action },
{ "LOG_MIN_PDELAY_REQ_INTERVAL", TLV_LOG_MIN_PDELAY_REQ_INTERVAL,
do_get_action },
{ "PORT_DATA_SET_NP", TLV_PORT_DATA_SET_NP, do_set_action },
+ { "PORT_STATS_NP", TLV_PORT_STATS_NP, do_get_action },
};
static void do_get_action(struct pmc *pmc, int action, int index, char *str)
diff --git a/port.c b/port.c
index 471e6f4..07ad3f0 100644
--- a/port.c
+++ b/port.c
@@ -788,6 +788,7 @@ static int port_management_fill_response(struct port
*target,
struct management_tlv_datum *mtd;
struct clock_description *desc;
struct port_properties_np *ppn;
+ struct port_stats_np *psn;
Re: [Linuxptp-devel] [PATCH 1/2] port: Introduce per-port stats for received and transmitted messages
Keller, Jacob E writes:
>> diff --git a/ddt.h b/ddt.h
>> index 4acaa4f..5486203 100644
>> --- a/ddt.h
>> +++ b/ddt.h
>> @@ -100,4 +100,9 @@ struct FaultRecord {
>> struct PTPText faultDescription;
>> };
>>
>> +struct PortStats {
>> +uint64_t rxMsgType[16];
>> +uint64_t txMsgType[16];
>
> Is there some define we could use that represents this 16, instead of
> just using the number? that would make it more clear why this is 16,
> and possibly update if that ever changes in the future.
messageType in Common Message Header has four bits (Table 18 in IEEE
1588-2008), hence 16 different messages can be encoded. There is no
define for it in ptp4l, at least I haven't found any. I can add one if
you prefer, or add an explanatory comment.
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH 0/2] Introduce per-port stats for received and transmitted messages
Black-box switches with PTP support commonly provide per-port statistics of number of messages sent and received, split by the message type. Like other statistics (ip link, ethtool, etc. etc.), network operators use the PTP message stats to monitor the (PTP) network and debug issues. When ptp4l is used to turn a Linux machine (be it a switch or a host) into a PTP clock, there is no easy way to get at these stats. It would certainly be possible to parse ingressing and egressing traffic using e.g. a u32 classifier, or create an ad-hoc eBPF-based tool, or something similar. But all these approaches have to work hard to extract the knowledge that ptp4l already has. ptp4l needs to parse the traffic anyway, and for transmitted packets obviously knows what it is sending. It is thus the natural place where the stats should be present. To that end, patch #1 introduces the message stats into linuxptp. A natural way to obtain these stats is then through pmc, which is implemented in patch #2, by way of a new TLV type. Petr Machata (2): port: Introduce per-port stats for received and transmitted messages pmc: Add a new TLV to obtain per-port statistics ddt.h | 5 + pmc.c | 47 +++ pmc_common.c | 1 + port.c | 32 ++-- port_private.h | 1 + tlv.c | 15 +++ tlv.h | 6 ++ 7 files changed, 105 insertions(+), 2 deletions(-) -- 2.20.1 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH 2/2] pmc: Add a new TLV to obtain per-port statistics
Add an ability of pmc to query per-port stats added in the previous patch.
Signed-off-by: Petr Machata
---
pmc.c| 47 +++
pmc_common.c | 1 +
port.c | 7 +++
tlv.c| 15 +++
tlv.h| 6 ++
5 files changed, 76 insertions(+)
diff --git a/pmc.c b/pmc.c
index 440c905..868fc2a 100644
--- a/pmc.c
+++ b/pmc.c
@@ -69,6 +69,7 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
struct tlv_extra *extra;
struct portDS *p;
struct port_ds_np *pnp;
+ struct port_stats_np *pcp;
if (msg_type(msg) != MANAGEMENT) {
return;
@@ -322,6 +323,52 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
pnp->neighborPropDelayThresh,
pnp->asCapable ? 1 : 0);
break;
+ case TLV_PORT_STATS_NP:
+ pcp = (struct port_stats_np *) mgt->data;
+ fprintf(fp, "PORT_STATS_NP "
+ IFMT "portIdentity %s"
+ IFMT "rx_Sync %" PRIu64
+ IFMT "rx_Delay_Req %" PRIu64
+ IFMT "rx_Pdelay_Req %" PRIu64
+ IFMT "rx_Pdelay_Resp%" PRIu64
+ IFMT "rx_Follow_Up %" PRIu64
+ IFMT "rx_Delay_Resp %" PRIu64
+ IFMT "rx_Pdelay_Resp_Follow_Up %" PRIu64
+ IFMT "rx_Announce %" PRIu64
+ IFMT "rx_Signaling %" PRIu64
+ IFMT "rx_Management %" PRIu64
+ IFMT "tx_Sync %" PRIu64
+ IFMT "tx_Delay_Req %" PRIu64
+ IFMT "tx_Pdelay_Req %" PRIu64
+ IFMT "tx_Pdelay_Resp%" PRIu64
+ IFMT "tx_Follow_Up %" PRIu64
+ IFMT "tx_Delay_Resp %" PRIu64
+ IFMT "tx_Pdelay_Resp_Follow_Up %" PRIu64
+ IFMT "tx_Announce %" PRIu64
+ IFMT "tx_Signaling %" PRIu64
+ IFMT "tx_Management %" PRIu64,
+ pid2str(>portIdentity),
+ pcp->stats.rxMsgType[SYNC],
+ pcp->stats.rxMsgType[DELAY_REQ],
+ pcp->stats.rxMsgType[PDELAY_REQ],
+ pcp->stats.rxMsgType[PDELAY_RESP],
+ pcp->stats.rxMsgType[FOLLOW_UP],
+ pcp->stats.rxMsgType[DELAY_RESP],
+ pcp->stats.rxMsgType[PDELAY_RESP_FOLLOW_UP],
+ pcp->stats.rxMsgType[ANNOUNCE],
+ pcp->stats.rxMsgType[SIGNALING],
+ pcp->stats.rxMsgType[MANAGEMENT],
+ pcp->stats.txMsgType[SYNC],
+ pcp->stats.txMsgType[DELAY_REQ],
+ pcp->stats.txMsgType[PDELAY_REQ],
+ pcp->stats.txMsgType[PDELAY_RESP],
+ pcp->stats.txMsgType[FOLLOW_UP],
+ pcp->stats.txMsgType[DELAY_RESP],
+ pcp->stats.txMsgType[PDELAY_RESP_FOLLOW_UP],
+ pcp->stats.txMsgType[ANNOUNCE],
+ pcp->stats.txMsgType[SIGNALING],
+ pcp->stats.txMsgType[MANAGEMENT]);
+ break;
case TLV_LOG_ANNOUNCE_INTERVAL:
mtd = (struct management_tlv_datum *) mgt->data;
fprintf(fp, "LOG_ANNOUNCE_INTERVAL "
diff --git a/pmc_common.c b/pmc_common.c
index 4d48e3a..592cc93 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -120,6 +120,7 @@ struct management_id idtab[] = {
{ "DELAY_MECHANISM", TLV_DELAY_MECHANISM, do_get_action },
{ "LOG_MIN_PDELAY_REQ_INTERVAL", TLV_LOG_MIN_PDELAY_REQ_INTERVAL,
do_get_action },
{ "PORT_DATA_SET_NP", TLV_PORT_DATA_SET_NP, do_set_action },
+ { "PORT_STATS_NP", TLV_PORT_STATS_NP, do_get_action },
};
static void do_get_action(struct pmc *pmc, int action, int index, char *str)
diff --git a/port.c b/port.c
index 471e6f4..07ad3f0 100644
--- a/port.c
+++ b/port.c
@@ -788,6 +788,7 @@ static int port_management_fill_response(struct port
*target,
struct management_tlv_datum *mtd;
struct clock_description *desc;
struct port_properties_np *ppn;
+ struct port_stats_np *psn;
[Linuxptp-devel] [PATCH 1/2] port: Introduce per-port stats for received and transmitted messages
Add struct PortStats to keep per-port number of messages sent and received,
split by message type. Bump TX counters after messages are sent
successfully, and RX counters after a message is received. To keep things
simple, reserve one counter for each theoretically possible message type,
including the reserved ones.
Signed-off-by: Petr Machata
---
ddt.h | 5 +
port.c | 25 +++--
port_private.h | 1 +
3 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/ddt.h b/ddt.h
index 4acaa4f..5486203 100644
--- a/ddt.h
+++ b/ddt.h
@@ -100,4 +100,9 @@ struct FaultRecord {
struct PTPText faultDescription;
};
+struct PortStats {
+ uint64_t rxMsgType[16];
+ uint64_t txMsgType[16];
+};
+
#endif
diff --git a/port.c b/port.c
index 5a4a116..471e6f4 100644
--- a/port.c
+++ b/port.c
@@ -580,6 +580,16 @@ static int path_trace_ignore(struct port *p, struct
ptp_message *m)
return 0;
}
+static void port_stats_inc_rx(struct port *p, const struct ptp_message *msg)
+{
+ p->stats.rxMsgType[msg_type(msg)]++;
+}
+
+static void port_stats_inc_tx(struct port *p, const struct ptp_message *msg)
+{
+ p->stats.txMsgType[msg_type(msg)]++;
+}
+
static int peer_prepare_and_send(struct port *p, struct ptp_message *msg,
enum transport_event event)
{
@@ -595,6 +605,7 @@ static int peer_prepare_and_send(struct port *p, struct
ptp_message *msg,
if (cnt <= 0) {
return -1;
}
+ port_stats_inc_tx(p, msg);
if (msg_sots_valid(msg)) {
ts_add(>hwts.ts, p->tx_timestamp_offset);
}
@@ -2627,6 +2638,7 @@ static enum fsm_event bc_event(struct port *p, int
fd_index)
msg_put(msg);
return EV_NONE;
}
+ port_stats_inc_rx(p, msg);
if (port_ignore(p, msg)) {
msg_put(msg);
return EV_NONE;
@@ -2691,14 +2703,22 @@ int port_forward(struct port *p, struct ptp_message
*msg)
{
int cnt;
cnt = transport_send(p->trp, >fda, TRANS_GENERAL, msg);
- return cnt <= 0 ? -1 : 0;
+ if (cnt <= 0) {
+ return -1;
+ }
+ port_stats_inc_tx(p, msg);
+ return 0;
}
int port_forward_to(struct port *p, struct ptp_message *msg)
{
int cnt;
cnt = transport_sendto(p->trp, >fda, TRANS_GENERAL, msg);
- return cnt <= 0 ? -1 : 0;
+ if (cnt <= 0) {
+ return -1;
+ }
+ port_stats_inc_tx(p, msg);
+ return 0;
}
int port_prepare_and_send(struct port *p, struct ptp_message *msg,
@@ -2717,6 +2737,7 @@ int port_prepare_and_send(struct port *p, struct
ptp_message *msg,
if (cnt <= 0) {
return -1;
}
+ port_stats_inc_tx(p, msg);
if (msg_sots_valid(msg)) {
ts_add(>hwts.ts, p->tx_timestamp_offset);
}
diff --git a/port_private.h b/port_private.h
index 9a5022d..5789fbb 100644
--- a/port_private.h
+++ b/port_private.h
@@ -139,6 +139,7 @@ struct port {
struct fault_interval flt_interval_pertype[FT_CNT];
enum fault_type last_fault_type;
unsigned intversionNumber; /*UInteger4*/
+ struct PortStatsstats;
/* foreignMasterDS */
LIST_HEAD(fm, foreign_clock) foreign_masters;
/* TC book keeping */
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [Linuxptp-users] forcing PASSIVE role for a port?
Keller, Jacob E writes:
>> > From: Sanjay Bhandari [mailto:san...@ziffusion.com]
>> > Is there a way to set a port in the PASSIVE role with ptp4l?
>> >
>> > Essentially, we want to listen in on the protocol, but not generate ANY
>> > protocol
>> messages ourselves. Nor do we want to set the PHC from any master.
>> >
>> > Is there a way to achieve this?
>>
>> Does using free_running, along with --slave-only do what you want?
>>
>> I don't think you're going to get a mode which only listens and doesn't do
>> ANY
>> messages back out... For that I'd suggest tcpdump and implement a filter
>> that can
>> read the PTP messages and show relevant fields for you.
WireShark has some related display filters, for example:
$ tshark -i sw1p49 -Y 'ptp.v2.messageid in {0 8}'
Capturing on 'sw1p49'
1 0.0 2001:db8:1::1 → ff0e::181PTPv2 108 Sync Message
2 0.79209 2001:db8:1::1 → ff0e::181PTPv2 108 Follow_Up Message
7 0.500071357 2001:db8:1::1 → ff0e::181PTPv2 108 Sync Message
8 0.500152168 2001:db8:1::1 → ff0e::181PTPv2 108 Follow_Up Message
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v4] sk: Recognize HWTSTAMP_FILTER_SOME
struct hwtstamp_config.rx_filter passed to SIOCSHWTSTAMP can be updated by
the kernel with the value of HWTSTAMP_FILTER_SOME. That indicates that all
requested packets will be timestamped, and some others as well.
Update hwts_init() to recognize this as a valid response in HWTS_FILTER_NORMAL
mode, instead of rejecting it as mismatch.
Cc: "Keller, Jacob E"
Cc: "Geva, Erez"
Signed-off-by: Petr Machata
---
Notes:
v4:
- Do not permit HWTSTAMP_FILTER_SOME for HWTS_FILTER_FULL either.
v3:
- Do not permit HWTSTAMP_FILTER_SOME for SIOCGHWTSTAMP.
v2:
- Fix whitespace.
sk.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sk.c b/sk.c
index 93ba77a..e211175 100644
--- a/sk.c
+++ b/sk.c
@@ -61,6 +61,7 @@ static int hwts_init(int fd, const char *device, int
rx_filter,
{
struct ifreq ifreq;
struct hwtstamp_config cfg;
+ int orig_rx_filter;
int err;
init_ifreq(, , device);
@@ -84,14 +85,14 @@ static int hwts_init(int fd, const char *device, int
rx_filter,
break;
case HWTS_FILTER_NORMAL:
cfg.tx_type = tx_type;
- cfg.rx_filter = rx_filter;
+ cfg.rx_filter = orig_rx_filter = rx_filter;
err = ioctl(fd, SIOCSHWTSTAMP, );
if (err < 0) {
pr_info("driver rejected most general HWTSTAMP filter");
init_ifreq(, , device);
cfg.tx_type = tx_type;
- cfg.rx_filter = rx_filter2;
+ cfg.rx_filter = orig_rx_filter = rx_filter2;
err = ioctl(fd, SIOCSHWTSTAMP, );
if (err < 0) {
@@ -99,6 +100,8 @@ static int hwts_init(int fd, const char *device, int
rx_filter,
return err;
}
}
+ if (cfg.rx_filter == HWTSTAMP_FILTER_SOME)
+ cfg.rx_filter = orig_rx_filter;
break;
}
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3] sk: Recognize HWTSTAMP_FILTER_SOME
Geva, Erez writes:
> It make sense for HWTS_FILTER_NORMAL.
>
> But for HWTSTAMP_FILTER_ALL, it does not make sense to get
> HWTSTAMP_FILTER_SOME.
> Some of what? You ask for all!
Yeah, there isn't really anything besides ALL thas this could timestamp.
I'll just modify the HWTS_FILTER_NORMAL then.
> Neither can you use rx_filter in this case, since it was not used in
> the SIOCSHWTSTAMP request.
>
> Erez
>
> ____
> From: Petr Machata [pe...@mellanox.com]
> Sent: 11 June 2019 15:18
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v3] sk: Recognize HWTSTAMP_FILTER_SOME
>
> struct hwtstamp_config.rx_filter passed to SIOCSHWTSTAMP can be updated by
> the kernel with the value of HWTSTAMP_FILTER_SOME. That indicates that all
> requested packets will be timestamped, and some others as well.
>
> Update hwts_init() to recognize this as a valid response, instead of
> rejecting it as mismatch.
>
> Cc: "Keller, Jacob E"
> Signed-off-by: Petr Machata
> ---
>
> Notes:
> v3:
> - Do not permit HWTSTAMP_FILTER_SOME for SIOCGHWTSTAMP.
>
> v2:
> - Fix whitespace.
>
> sk.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/sk.c b/sk.c
> index 93ba77a..8fda9c2 100644
> --- a/sk.c
> +++ b/sk.c
> @@ -61,6 +61,7 @@ static int hwts_init(int fd, const char *device, int
> rx_filter,
> {
> struct ifreq ifreq;
> struct hwtstamp_config cfg;
> + int orig_rx_filter;
> int err;
>
> init_ifreq(, , device);
> @@ -81,17 +82,19 @@ static int hwts_init(int fd, const char *device, int
> rx_filter,
> pr_err("ioctl SIOCSHWTSTAMP failed: %m");
> return err;
> }
> + if (cfg.rx_filter == HWTSTAMP_FILTER_SOME)
> + cfg.rx_filter = rx_filter;
> break;
> case HWTS_FILTER_NORMAL:
> cfg.tx_type = tx_type;
> - cfg.rx_filter = rx_filter;
> + cfg.rx_filter = orig_rx_filter = rx_filter;
> err = ioctl(fd, SIOCSHWTSTAMP, );
> if (err < 0) {
> pr_info("driver rejected most general HWTSTAMP
> filter");
>
> init_ifreq(, , device);
> cfg.tx_type = tx_type;
> - cfg.rx_filter = rx_filter2;
> + cfg.rx_filter = orig_rx_filter = rx_filter2;
>
> err = ioctl(fd, SIOCSHWTSTAMP, );
> if (err < 0) {
> @@ -99,6 +102,8 @@ static int hwts_init(int fd, const char *device, int
> rx_filter,
> return err;
> }
> }
> + if (cfg.rx_filter == HWTSTAMP_FILTER_SOME)
> + cfg.rx_filter = orig_rx_filter;
> break;
> }
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2] sk: Recognize HWTSTAMP_FILTER_SOME
Geva, Erez writes:
> Hi,
>
> The "cfg.rx_filter != HWTSTAMP_FILTER_ALL)) { "
> line catch both filter mode is "HWTS_FILTER_FULL, and the fact that the
> filter could is in a higher state for both HWTS_FILTER_NORMAL,
> HWTS_FILTER_CHECK.
>
> Could you be more specific what does the HWTSTAMP_FILTER_SOME means?
>From net_tstamp.h:
/* possible values for hwtstamp_config->rx_filter */
enum hwtstamp_rx_filters {
...
/* return value: time stamp all packets requested plus some others */
HWTSTAMP_FILTER_SOME,
};
> For me, the HWTSTAMP_FILTER_SOME might be good when using filter mode
> "HWTS_FILTER_NORMAL".
>
> But when filter mode is "HWTS_FILTER_FULL" I would accept that the flag will
> be HWTSTAMP_FILTER_ALL only.
If you make ALL acceptable, why not SOME? Both mean that the driver will
timestamp everything requested and more.
> What would you accept on filter mode HWTS_FILTER_CHECK?
I agree that for HWTS_FILTER_CHECK, SOME is not acceptable, which is why
I respun v3 of my patch.
> Full state machine:
>
> HWTS_FILTER_CHECK
> get config
> check if RX filter one of Rx filter 1, Rx filter 2, HWTSTAMP_FILTER_ALL
> As HWTSTAMP_FILTER_ALL is a higher state
>
> HWTS_FILTER_NORMAL
> set Rx filter 1
> if fail set Rx filter 2
> check if RX filter one of Rx filter 1, Rx filter 2, HWTSTAMP_FILTER_ALL
> As HWTSTAMP_FILTER_ALL is a higher state
>
> HWTS_FILTER_FULL
> set Rx filter HWTSTAMP_FILTER_ALL
> check if RX filter one of Rx filter 1, Rx filter 2, HWTSTAMP_FILTER_ALL
> Filter should be HWTSTAMP_FILTER_ALL,
> but if NIc driver only set filter to be one of Rx filter 1, Rx filter 2
> then the PTP daemon may proceed normally.
>
> Erez
>
>
> From: Petr Machata [pe...@mellanox.com]
> Sent: 10 June 2019 15:56
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v2] sk: Recognize HWTSTAMP_FILTER_SOME
>
> struct hwtstamp_config.rx_filter passed to SIOCSHWTSTAMP can be updated by
> the kernel with the value of HWTSTAMP_FILTER_SOME. That indicates that all
> requested packets will be timestamped, and some others as well.
>
> Update hwts_init() to recognize this as a valid response, instead of
> rejecting it as mismatch.
>
> Signed-off-by: Petr Machata
> ---
>
> Notes:
> v2: Fix whitespace.
>
> sk.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sk.c b/sk.c
> index 93ba77a..416d784 100644
> --- a/sk.c
> +++ b/sk.c
> @@ -105,7 +105,8 @@ static int hwts_init(int fd, const char *device, int
> rx_filter,
> if (cfg.tx_type != tx_type ||
> (cfg.rx_filter != rx_filter &&
> cfg.rx_filter != rx_filter2 &&
> -cfg.rx_filter != HWTSTAMP_FILTER_ALL)) {
> +cfg.rx_filter != HWTSTAMP_FILTER_ALL &&
> +cfg.rx_filter != HWTSTAMP_FILTER_SOME)) {
> pr_debug("tx_type %d not %d", cfg.tx_type, tx_type);
> pr_debug("rx_filter %d not %d or %d", cfg.rx_filter,
> rx_filter,
> rx_filter2);
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v3] sk: Recognize HWTSTAMP_FILTER_SOME
struct hwtstamp_config.rx_filter passed to SIOCSHWTSTAMP can be updated by
the kernel with the value of HWTSTAMP_FILTER_SOME. That indicates that all
requested packets will be timestamped, and some others as well.
Update hwts_init() to recognize this as a valid response, instead of
rejecting it as mismatch.
Cc: "Keller, Jacob E"
Signed-off-by: Petr Machata
---
Notes:
v3:
- Do not permit HWTSTAMP_FILTER_SOME for SIOCGHWTSTAMP.
v2:
- Fix whitespace.
sk.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sk.c b/sk.c
index 93ba77a..8fda9c2 100644
--- a/sk.c
+++ b/sk.c
@@ -61,6 +61,7 @@ static int hwts_init(int fd, const char *device, int
rx_filter,
{
struct ifreq ifreq;
struct hwtstamp_config cfg;
+ int orig_rx_filter;
int err;
init_ifreq(, , device);
@@ -81,17 +82,19 @@ static int hwts_init(int fd, const char *device, int
rx_filter,
pr_err("ioctl SIOCSHWTSTAMP failed: %m");
return err;
}
+ if (cfg.rx_filter == HWTSTAMP_FILTER_SOME)
+ cfg.rx_filter = rx_filter;
break;
case HWTS_FILTER_NORMAL:
cfg.tx_type = tx_type;
- cfg.rx_filter = rx_filter;
+ cfg.rx_filter = orig_rx_filter = rx_filter;
err = ioctl(fd, SIOCSHWTSTAMP, );
if (err < 0) {
pr_info("driver rejected most general HWTSTAMP filter");
init_ifreq(, , device);
cfg.tx_type = tx_type;
- cfg.rx_filter = rx_filter2;
+ cfg.rx_filter = orig_rx_filter = rx_filter2;
err = ioctl(fd, SIOCSHWTSTAMP, );
if (err < 0) {
@@ -99,6 +102,8 @@ static int hwts_init(int fd, const char *device, int
rx_filter,
return err;
}
}
+ if (cfg.rx_filter == HWTSTAMP_FILTER_SOME)
+ cfg.rx_filter = orig_rx_filter;
break;
}
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] sk: Recognize HWTSTAMP_FILTER_SOME
Keller, Jacob E writes:
>> index 93ba77a..416d784 100644
>> --- a/sk.c
>> +++ b/sk.c
>> @@ -105,7 +105,8 @@ static int hwts_init(int fd, const char *device, int
>> rx_filter,
>> if (cfg.tx_type != tx_type ||
>> (cfg.rx_filter != rx_filter &&
>> cfg.rx_filter != rx_filter2 &&
>> - cfg.rx_filter != HWTSTAMP_FILTER_ALL)) {
>> + cfg.rx_filter != HWTSTAMP_FILTER_ALL &&
>> + cfg.rx_filter != HWTSTAMP_FILTER_SOME)) {
>> pr_debug("tx_type %d not %d", cfg.tx_type, tx_type);
>> pr_debug("rx_filter %d not %d or %d", cfg.rx_filter, rx_filter,
>> rx_filter2);
>
> You can't accept HWTSTAMP_FILTER_SOME from the "get hwtstamp config"
> ioctl, because it's unclear what packets "SOME" actually refers to.
> I.e. one caller could set it to filter V1 packets, the driver could
> report SOME, and then ptp4l could "accept" this filter even though
> it's not valid.
You are right, I missed the FILTER_CHECK scenario in the same function.
I'll try to come up with a way to encode the check that's not three
layers of and-or nesting.
(If the goal of the patch is generally acceptable.)
> You can accept it in response to the "set hwtstamp config", I suppose,
> but it is a tad confusing. I never really liked that return value, and
> just wish the API had been "you may timestamp more than requested
> without reporting it if there is no valid filter matching what you
> timestamp".
It looks like there's currently only one driver doing this in the
vanilla kernel, Intel e1000e. I'm writing PTP support for Mellanox
Spectrum switches, and the HW can be configured to timestamp by PTP
message type, but not by layer or PTP version. I think SOME is exactly
appropriate here.
Having a major PTP stack on Linux reject the configuration is something
of a bummer though, even if it should be fixed in the next version, so I
might reconsider. But it should be fixed, the interface is what it is.
Thanks,
Petr
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v2] sk: Recognize HWTSTAMP_FILTER_SOME
struct hwtstamp_config.rx_filter passed to SIOCSHWTSTAMP can be updated by
the kernel with the value of HWTSTAMP_FILTER_SOME. That indicates that all
requested packets will be timestamped, and some others as well.
Update hwts_init() to recognize this as a valid response, instead of
rejecting it as mismatch.
Signed-off-by: Petr Machata
---
Notes:
v2: Fix whitespace.
sk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sk.c b/sk.c
index 93ba77a..416d784 100644
--- a/sk.c
+++ b/sk.c
@@ -105,7 +105,8 @@ static int hwts_init(int fd, const char *device, int
rx_filter,
if (cfg.tx_type != tx_type ||
(cfg.rx_filter != rx_filter &&
cfg.rx_filter != rx_filter2 &&
-cfg.rx_filter != HWTSTAMP_FILTER_ALL)) {
+cfg.rx_filter != HWTSTAMP_FILTER_ALL &&
+cfg.rx_filter != HWTSTAMP_FILTER_SOME)) {
pr_debug("tx_type %d not %d", cfg.tx_type, tx_type);
pr_debug("rx_filter %d not %d or %d", cfg.rx_filter, rx_filter,
rx_filter2);
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH] sk: Recognize HWTSTAMP_FILTER_SOME
struct hwtstamp_config.rx_filter passed to SIOCSHWTSTAMP can be updated by
the kernel with the value of HWTSTAMP_FILTER_SOME. That indicates that all
requested packets will be timestamped, and some others as well.
Update hwts_init() to recognize this as a valid response, instead of
rejecting it as mismatch.
Signed-off-by: Petr Machata
---
sk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sk.c b/sk.c
index 93ba77a..416d784 100644
--- a/sk.c
+++ b/sk.c
@@ -105,7 +105,8 @@ static int hwts_init(int fd, const char *device, int
rx_filter,
if (cfg.tx_type != tx_type ||
(cfg.rx_filter != rx_filter &&
cfg.rx_filter != rx_filter2 &&
-cfg.rx_filter != HWTSTAMP_FILTER_ALL)) {
+cfg.rx_filter != HWTSTAMP_FILTER_ALL &&
+cfg.rx_filter != HWTSTAMP_FILTER_SOME)) {
pr_debug("tx_type %d not %d", cfg.tx_type, tx_type);
pr_debug("rx_filter %d not %d or %d", cfg.rx_filter, rx_filter,
rx_filter2);
--
2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] sk: Recognize HWTSTAMP_FILTER_SOME
Petr Machata writes:
> @@ -105,7 +105,8 @@ static int hwts_init(int fd, const char *device, int
> rx_filter,
> if (cfg.tx_type != tx_type ||
> (cfg.rx_filter != rx_filter &&
>cfg.rx_filter != rx_filter2 &&
> - cfg.rx_filter != HWTSTAMP_FILTER_ALL)) {
> + cfg.rx_filter != HWTSTAMP_FILTER_ALL &&
> + cfg.rx_filter != HWTSTAMP_FILTER_SOME)) {
Oops, this should be a tab. I'll respin momentarily.
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
