Skip to contentSkip to conversation

af_packet: don't enable timestamps in mmap'ed sockets

PATCH In review Unai Uribarri 4 participants 15 messages Aug 9, 2007 – Sep 27, 2007

Subsystems (4)
Networking Network drivers Sockets Packet sockets
lore
Share & tools
lore

Unai Uribarri

author
#0 lore raw

PATCH 1/1 af_packet: don't enable timestamps in mmap'ed sockets

Hello folks,

I've discovered two strange behaviours (bugs?) about timestamp generation:

1. If a program opens an AF_PACKET socket and setup a reception ring with setsockopt(sock, SOL_PACKET, PACKET_RX_RING), timestamps are automatically (re)enabled at the reception of every packet.

2. Setting SOL_SOCKET/SO_TIMESTAMP to 0 doesn't disables timestamp generation. Every skb continues begin timestamped until you close the socket that activated it.

Timestamp generation is a heavy task that is consuming more than 50% of the CPU (using ACPI PM clock) and is currently the bottleneck in my packet capturing application.

The attached patch removes the automatic timestamp activation, that only mmap'ed AF_PACKET sockets perform. I known it can break user applications, but I believe that it's the correct solution.

I will be very pleased to receive any feedback.

Signed-off-by Unai Uribarri <[email protected]>

---

net/packet/af_packet.c +0 -4
index 1322d62..a4f2da3 100644
@@ -640,10 +640,6 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, struct packe
        h->tp_snaplen = snaplen;
        h->tp_mac = macoff;
        h->tp_net = netoff;
-       if (skb->tstamp.tv64 == 0) {
-               __net_timestamp(skb);
-               sock_enable_timestamp(sk);
-       }
        tv = ktime_to_timeval(skb->tstamp);
        h->tp_sec = tv.tv_sec;
        h->tp_usec = tv.tv_usec;
#1 lore raw
Quoted 3 lines · Unai Uribarri

The attached patch removes the automatic timestamp activation, that only mmap'ed AF_PACKET sockets perform. I known it can break user applications, but I believe that it's the correct solution.

How tcpdump with mmap libpcap will work with it?

Signature · 1 line
	Evgeniy Polyakov
author
#2 lore raw
Quoted 6 lines · Evgeniy Polyakov
Quoted 3 lines · Unai Uribarri

The attached patch removes the automatic timestamp activation, that only mmap'ed AF_PACKET sockets perform. I known it can break user applications, but I believe that it's the correct solution.

How tcpdump with mmap libpcap will work with it?

In Linux, you can enable timestamps on any socket executing:

int val = 1;
setsockopt(sock, SOL_SOCKET, SO_TIMESTAMP, &val, sizeof(val));

PD: Current release of tcpdump doesn't mmap the reception ring and timestamps packets at user space with gettimeofday. It isn't the best performing alternative, but it's portable.

#3 lore raw
Quoted 8 lines · Unai Uribarri
Quoted 6 lines · Evgeniy Polyakov
Quoted 3 lines · Unai Uribarri

The attached patch removes the automatic timestamp activation, that only mmap'ed AF_PACKET sockets perform. I known it can break user applications, but I believe that it's the correct solution.

How tcpdump with mmap libpcap will work with it?

Quoted 8 lines

In Linux, you can enable timestamps on any socket executing:

int val = 1;
setsockopt(sock, SOL_SOCKET, SO_TIMESTAMP, &val, sizeof(val));

PD: Current release of tcpdump doesn't mmap the reception ring and timestamps packets at user space with gettimeofday. It isn't the best performing alternative, but it's portable.

IIRC, there was/is a libpcap which worked with mapped sockets, mybe not official though. Any application which depened on having timestamps with packets will not work now. Why not to implement an absolutely_turn_off_timestamps option instead of breaking compatibility?

Signature · 1 line
	Evgeniy Polyakov
author
#4 lore raw

There is another option:

1. Move timestampt activation to packet_set_ring(), so it's activated only once at setup instead of every time a packet arrives.
2. Fix sock_setsockopt() so setting SO_TIMESTAMP to 0 effectively disables timestamp.

My first patch set did that. I sent that patches in mime multipart format and they were removed from the list archives. I can resent them in plain text if needed.

Quoted 23 lines · Evgeniy Polyakov
Quoted 8 lines · Unai Uribarri
Quoted 6 lines · Evgeniy Polyakov
Quoted 3 lines · Unai Uribarri

The attached patch removes the automatic timestamp activation, that only mmap'ed AF_PACKET sockets perform. I known it can break user applications, but I believe that it's the correct solution.

How tcpdump with mmap libpcap will work with it?

Quoted 8 lines

In Linux, you can enable timestamps on any socket executing:

int val = 1;
setsockopt(sock, SOL_SOCKET, SO_TIMESTAMP, &val, sizeof(val));

PD: Current release of tcpdump doesn't mmap the reception ring and timestamps packets at user space with gettimeofday. It isn't the best performing alternative, but it's portable.

IIRC, there was/is a libpcap which worked with mapped sockets, mybe not official though. Any application which depened on having timestamps with packets will not work now. Why not to implement an absolutely_turn_off_timestamps option instead of breaking compatibility?

#5 lore raw

Hi Unai.

Quoted 4 lines · Unai Uribarri

There is another option:

1. Move timestampt activation to packet_set_ring(), so it's activated only once at setup instead of every time a packet arrives.

Does this break existing systems which expects timestamp be turned on always if there are packet sockets.

Quoted 2 lines

2. Fix sock_setsockopt() so setting SO_TIMESTAMP to 0 effectively disables timestamp.

This breaks compatibility. Add new socket option, which will really disable it and do all your logic, but not breaking existing applications.

Signature · 1 line
	Evgeniy Polyakov
author
#6 lore raw
Quoted 11 lines · Evgeniy Polyakov

Hi Unai.

Quoted 4 lines · Unai Uribarri

There is another option:

1. Move timestampt activation to packet_set_ring(), so it's activated only once at setup instead of every time a packet arrives.

Does this break existing systems which expects timestamp be turned on always if there are packet sockets.

Well, current behaviour is that all packets get always timestamped if the socket has a reception ring. We are just activating it a bit sooner at the setsockopt(SOL_PACKET, PACKET_RX_RING) call instead of waiting until the reception of the first packet. And current applications can't disable it if we use a new socket option. So I can see how an application can break.

Quoted 7 lines
Quoted 2 lines

2. Fix sock_setsockopt() so setting SO_TIMESTAMP to 0 effectively disables timestamp.

This breaks compatibility. Add new socket option, which will really disable it and do all your logic, but not breaking existing applications.

Is SO_TIMESTAMP2 a valid name? I can't imagine how to call it.

#7 lore raw
Quoted 6 lines · Unai Uribarri
Quoted 4 lines

This breaks compatibility. Add new socket option, which will really disable it and do all your logic, but not breaking existing applications.

Is SO_TIMESTAMP2 a valid name? I can't imagine how to call it.

:) what about name, which really shows what option does?

Signature · 1 line
	Evgeniy Polyakov
author
#8 lore raw
In reply to Evgeniy Polyakov

Do not enable timestamps automatically on mmap'ed AF_PACKET sockets.

---
commit d1d6e6bf196e31b6306fd0fef95f4190983c8a86
tree 22637506c0aafeabfbe05faf5352d0358c4d9460
parent 6a302358d87fedaf7bda12b8e909265ebf1ce674
author Unai Uribarri <[email protected]> Tue, 31 Jul 2007 20:38:42 +0200 committer Unai Uribarri <[email protected]> Tue, 31 Jul 2007 20:38:42 +0200

 net/packet/af_packet.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)
net/packet/af_packet.c +0 -4
index 1322d62..a4f2da3 100644
@@ -640,10 +640,6 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, struct packe
        h->tp_snaplen = snaplen;
        h->tp_mac = macoff;
        h->tp_net = netoff;
-       if (skb->tstamp.tv64 == 0) {
-               __net_timestamp(skb);
-               sock_enable_timestamp(sk);
-       }
        tv = ktime_to_timeval(skb->tstamp);
        h->tp_sec = tv.tv_sec;
        h->tp_usec = tv.tv_usec;

!-------------------------------------------------------------flip-

Effectively disable timestamping when requested by SO_TIMESTAMP

---
commit 1fdf6bb534dfbc6e9bdf8958620b05d5334b15eb
tree ec4b577c1704f178f0f7c5d8d69af41454fc8f14
parent d1d6e6bf196e31b6306fd0fef95f4190983c8a86
author Unai Uribarri <[email protected]> Tue, 31 Jul 2007 20:43:00 +0200 committer Unai Uribarri <[email protected]> Tue, 31 Jul 2007 20:43:00 +0200

 net/core/sock.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
net/core/sock.c +1 -0
index cfed7d4..3af2322 100644
@@ -561,6 +561,7 @@ set_rcvbuf:
                } else {
                        sock_reset_flag(sk, SOCK_RCVTSTAMP);
                        sock_reset_flag(sk, SOCK_RCVTSTAMPNS);
+                       sock_disable_timestamp(sk);
                }
                break;

!-------------------------------------------------------------flip-

Automatically enable timestamping on mmap'ed AF_PACKET sockets.

---
commit 4564f367ff054bd8837cc6cb1cfb9a927c57054a
tree 3475d56cc7ea74052677c944ec0a6bf6e9f4817c
parent 1fdf6bb534dfbc6e9bdf8958620b05d5334b15eb
author Unai Uribarri <[email protected]> Tue, 31 Jul 2007 20:43:59 +0200 committer Unai Uribarri <[email protected]> Tue, 31 Jul 2007 20:43:59 +0200

 net/packet/af_packet.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
net/packet/af_packet.c +1 -0
index a4f2da3..5179daf 100644
@@ -1750,6 +1750,7 @@ static int packet_set_ring(struct sock *sk, struct tpacket_req *req, int closing
 
                po->pg_vec_pages = req->tp_block_size/PAGE_SIZE;
                po->prot_hook.func = po->pg_vec ? tpacket_rcv : packet_rcv;
+               sock_enable_timestamp(sk);
                skb_queue_purge(&sk->sk_receive_queue);
 #undef XC
                if (atomic_read(&po->mapped))

!-------------------------------------------------------------flip-

Quoted 23 lines · Evgeniy Polyakov
Quoted 8 lines · Unai Uribarri
Quoted 6 lines · Evgeniy Polyakov
Quoted 3 lines · Unai Uribarri

The attached patch removes the automatic timestamp activation, that only mmap'ed AF_PACKET sockets perform. I known it can break user applications, but I believe that it's the correct solution.

How tcpdump with mmap libpcap will work with it?

Quoted 8 lines

In Linux, you can enable timestamps on any socket executing:

int val = 1;
setsockopt(sock, SOL_SOCKET, SO_TIMESTAMP, &val, sizeof(val));

PD: Current release of tcpdump doesn't mmap the reception ring and timestamps packets at user space with gettimeofday. It isn't the best performing alternative, but it's portable.

IIRC, there was/is a libpcap which worked with mapped sockets, mybe not official though. Any application which depened on having timestamps with packets will not work now. Why not to implement an absolutely_turn_off_timestamps option instead of breaking compatibility?

#9 lore raw

RFC 1/1 af_packet: allow disabling timestamps

Currently, af_packet does not allow disabling timestamps. This patch changes that but doesn't force global timestamps on.

This shows up in bugzilla as:

	http://bugzilla.kernel.org/show_bug.cgi?id=4809

Patch against net-2.6.24 tree.

Signed-off-by Stephen Hemminger <[email protected]>
net/core/sock.c +4 -2
@@ -259,7 +259,8 @@ static void sock_disable_timestamp(struc
 {
 	if (sock_flag(sk, SOCK_TIMESTAMP)) {
 		sock_reset_flag(sk, SOCK_TIMESTAMP);
-		net_disable_timestamp();
+		if (sk->sk_family != PF_PACKET)
+			net_disable_timestamp();
 	}
 }
 

@@ -1645,7 +1646,8 @@ void sock_enable_timestamp(struct sock *
 {
 	if (!sock_flag(sk, SOCK_TIMESTAMP)) {
 		sock_set_flag(sk, SOCK_TIMESTAMP);
-		net_enable_timestamp();
+		if (sk->sk_family != PF_PACKET)
+			net_enable_timestamp();
 	}
 }
 EXPORT_SYMBOL(sock_enable_timestamp);
net/packet/af_packet.c +14 -7
@@ -572,7 +572,6 @@ static int tpacket_rcv(struct sk_buff *s
 	unsigned long status = TP_STATUS_LOSING|TP_STATUS_USER;
 	unsigned short macoff, netoff;
 	struct sk_buff *copy_skb = NULL;
-	struct timeval tv;
 
 	if (dev->nd_net != &init_net)
 		goto drop;

@@ -650,12 +649,19 @@ static int tpacket_rcv(struct sk_buff *s
 	h->tp_snaplen = snaplen;
 	h->tp_mac = macoff;
 	h->tp_net = netoff;
-	if (skb->tstamp.tv64)
-		tv = ktime_to_timeval(skb->tstamp);
-	else
-		do_gettimeofday(&tv);
-	h->tp_sec = tv.tv_sec;
-	h->tp_usec = tv.tv_usec;
+
+	if (sock_flag(sk, SOCK_TIMESTAMP)) {
+		struct timeval tv;
+		if (skb->tstamp.tv64)
+			tv = ktime_to_timeval(skb->tstamp);
+		else
+			do_gettimeofday(&tv);
+		h->tp_sec = tv.tv_sec;
+		h->tp_usec = tv.tv_usec;
+	} else {
+		h->tp_sec = 0;
+		h->tp_usec = 0;
+	}
 
 	sll = (struct sockaddr_ll*)((u8*)h + TPACKET_ALIGN(sizeof(*h)));
 	sll->sll_halen = 0;

@@ -1014,6 +1020,7 @@ static int packet_create(struct net *net
 		sock->ops = &packet_ops_spkt;
 
 	sock_init_data(sock, sk);
+	sock_set_flag(sk, SOCK_TIMESTAMP);
 
 	po = pkt_sk(sk);
 	sk->sk_family = PF_PACKET;
#10 lore raw

Re: [RFC] af_packet: allow disabling timestamps

Quoted 8 lines · On Thu, 13 Sep 2007 12:42:53 +0200 Stephen Hemminger…

Currently, af_packet does not allow disabling timestamps. This patch changes that but doesn't force global timestamps on.

This shows up in bugzilla as:

	http://bugzilla.kernel.org/show_bug.cgi?id=4809

Patch against net-2.6.24 tree.

I am not sure I understood this patch.

This means that tcpdump/ethereal wont get precise timestamps
(gathered when packet is received), but imprecise ones (gathered when the sniffer reads the packet)

I added some time ago ktime infrastructure to eventually get nanosecond precision in libpcap, so I would prefer a step in the right direction :)

Should'nt we use something like :

[PATCH] af_packet : allow disabling timestamps, or requesting nanosecond precision.

Signed-off-by Eric Dumazet <[email protected]>
net/core/sock.c +1 -0
index 5a16e38..1c10b9d 100644
@@ -563,6 +563,7 @@ set_rcvbuf:
 		} else {
 			sock_reset_flag(sk, SOCK_RCVTSTAMP);
 			sock_reset_flag(sk, SOCK_RCVTSTAMPNS);
+			sock_disable_timestamp(sk);
 		}
 		break;
net/packet/af_packet.c +22 -6
index 745e2cb..409de44 100644
@@ -650,12 +650,27 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, struct packe
 	h->tp_snaplen = snaplen;
 	h->tp_mac = macoff;
 	h->tp_net = netoff;
-	if (skb->tstamp.tv64)
-		tv = ktime_to_timeval(skb->tstamp);
-	else
-		do_gettimeofday(&tv);
-	h->tp_sec = tv.tv_sec;
-	h->tp_usec = tv.tv_usec;
+	h->tp_sec = 0;
+	h->tp_usec = 0;
+	if ((sock_flag(sk, SOCK_TIMESTAMP))) {
+		if (sock_flag(sk, SOCK_RCVTSTAMPNS)) {
+			struct timespec ts;
+			if (skb->tstamp.tv64)
+				ts = ktime_to_timespec(skb->tstamp);
+			else
+				getnstimeofday(&ts);
+			h->tp_sec = ts.tv_sec;
+			h->tp_usec = ts.tv_nsec; /* cheat a litle bit */
+		}
+		else {
+			if (skb->tstamp.tv64)
+				tv = ktime_to_timeval(skb->tstamp);
+			else
+				do_gettimeofday(&tv);
+			h->tp_sec = tv.tv_sec;
+			h->tp_usec = tv.tv_usec;
+		}
+	}
 
 	sll = (struct sockaddr_ll*)((u8*)h + TPACKET_ALIGN(sizeof(*h)));
 	sll->sll_halen = 0;

@@ -1014,6 +1029,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol)
 		sock->ops = &packet_ops_spkt;
 
 	sock_init_data(sock, sk);
+	sock_enable_timestamp(sk);
 
 	po = pkt_sk(sk);
 	sk->sk_family = PF_PACKET;
#11 lore raw

Re: [RFC] af_packet: allow disabling timestamps

Quoted 77 lines · On Thu, 13 Sep 2007 14:24:06 +0200 Eric Dumazet <[email protected]> wrote:
Quoted 8 lines · On Thu, 13 Sep 2007 12:42:53 +0200 Stephen Hemminger…

Currently, af_packet does not allow disabling timestamps. This patch changes that but doesn't force global timestamps on.

This shows up in bugzilla as:

	http://bugzilla.kernel.org/show_bug.cgi?id=4809

Patch against net-2.6.24 tree.

I am not sure I understood this patch.

This means that tcpdump/ethereal wont get precise timestamps
(gathered when packet is received), but imprecise ones (gathered when the sniffer reads the packet)

I added some time ago ktime infrastructure to eventually get nanosecond precision in libpcap, so I would prefer a step in the right direction :)

Should'nt we use something like :

[PATCH] af_packet : allow disabling timestamps, or requesting nanosecond precision.

Signed-off-by Eric Dumazet <[email protected]>
net/core/sock.c +1 -0
index 5a16e38..1c10b9d 100644
@@ -563,6 +563,7 @@ set_rcvbuf:
 		} else {
 			sock_reset_flag(sk, SOCK_RCVTSTAMP);
 			sock_reset_flag(sk, SOCK_RCVTSTAMPNS);
+			sock_disable_timestamp(sk);
 		}
 		break;
net/packet/af_packet.c +21 -6
index 745e2cb..409de44 100644
@@ -650,12 +650,27 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, struct packe
 	h->tp_snaplen = snaplen;
 	h->tp_mac = macoff;
 	h->tp_net = netoff;
-	if (skb->tstamp.tv64)
-		tv = ktime_to_timeval(skb->tstamp);
-	else
-		do_gettimeofday(&tv);
-	h->tp_sec = tv.tv_sec;
-	h->tp_usec = tv.tv_usec;
+	h->tp_sec = 0;
+	h->tp_usec = 0;
+	if ((sock_flag(sk, SOCK_TIMESTAMP))) {
+		if (sock_flag(sk, SOCK_RCVTSTAMPNS)) {
+			struct timespec ts;
+			if (skb->tstamp.tv64)
+				ts = ktime_to_timespec(skb->tstamp);
+			else
+				getnstimeofday(&ts);
+			h->tp_sec = ts.tv_sec;
+			h->tp_usec = ts.tv_nsec; /* cheat a litle bit */
+		}
+		else {
+			if (skb->tstamp.tv64)
+				tv = ktime_to_timeval(skb->tstamp);
+			else
+				do_gettimeofday(&tv);
+			h->tp_sec = tv.tv_sec;
+			h->tp_usec = tv.tv_usec;
+		}
+	}
 
 	sll = (struct sockaddr_ll*)((u8*)h + TPACKET_ALIGN(sizeof(*h)));
 	sll->sll_halen = 0;

net/packet/af_packet.c static int packet_create(struct net *net, struct socket *sock, int protocol) · line 1035

Quoted 7 lines
net/packet/af_packet.c +1 -0
@@ -1014,6 +1029,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol)
 		sock->ops = &packet_ops_spkt;
 
 	sock_init_data(sock, sk);
+	sock_enable_timestamp(sk);
 
 	po = pkt_sk(sk);
 	sk->sk_family = PF_PACKET;

No, then we end up timestamping all the packets, even if they get dropped by packet filter. The change in 2.6.24 allows dhclient (and rstp) to only call hires clock source for packets they want, not all packets.

Perhaps the timestamping needs to change into a tristate flag?

#12 lore raw

Re: [RFC] af_packet: allow disabling timestamps

Quoted 5 lines · Stephen Hemminger

No, then we end up timestamping all the packets, even if they get dropped by packet filter. The change in 2.6.24 allows dhclient (and rstp) to only call hires clock source for packets they want, not all packets.

Perhaps the timestamping needs to change into a tristate flag?

Sorry for late reply - the situation we need to workaround (if I uderstood correctly) is to disable timestamp when there are packet sockets. Why tristate flag should ever be used?

Signature · 1 line
	Evgeniy Polyakov
author
#13 lore raw

Re: [RFC] af_packet: allow disabling timestamps

In reply to Stephen Hemminger
Quoted 92 lines · Stephen Hemminger
Quoted 77 lines · On Thu, 13 Sep 2007 14:24:06 +0200 Eric Dumazet <[email protected]> wrote:
Quoted 8 lines · On Thu, 13 Sep 2007 12:42:53 +0200 Stephen Hemminger…

Currently, af_packet does not allow disabling timestamps. This patch changes that but doesn't force global timestamps on.

This shows up in bugzilla as:

	http://bugzilla.kernel.org/show_bug.cgi?id=4809

Patch against net-2.6.24 tree.

I am not sure I understood this patch.

This means that tcpdump/ethereal wont get precise timestamps
(gathered when packet is received), but imprecise ones (gathered when the sniffer reads the packet)

I added some time ago ktime infrastructure to eventually get nanosecond precision in libpcap, so I would prefer a step in the right direction :)

Should'nt we use something like :

[PATCH] af_packet : allow disabling timestamps, or requesting nanosecond precision.

Signed-off-by Eric Dumazet <[email protected]>
net/core/sock.c +1 -0
index 5a16e38..1c10b9d 100644
@@ -563,6 +563,7 @@ set_rcvbuf:
 		} else {
 			sock_reset_flag(sk, SOCK_RCVTSTAMP);
 			sock_reset_flag(sk, SOCK_RCVTSTAMPNS);
+			sock_disable_timestamp(sk);
 		}
 		break;
net/packet/af_packet.c +21 -6
index 745e2cb..409de44 100644
@@ -650,12 +650,27 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, struct packe
 	h->tp_snaplen = snaplen;
 	h->tp_mac = macoff;
 	h->tp_net = netoff;
-	if (skb->tstamp.tv64)
-		tv = ktime_to_timeval(skb->tstamp);
-	else
-		do_gettimeofday(&tv);
-	h->tp_sec = tv.tv_sec;
-	h->tp_usec = tv.tv_usec;
+	h->tp_sec = 0;
+	h->tp_usec = 0;
+	if ((sock_flag(sk, SOCK_TIMESTAMP))) {
+		if (sock_flag(sk, SOCK_RCVTSTAMPNS)) {
+			struct timespec ts;
+			if (skb->tstamp.tv64)
+				ts = ktime_to_timespec(skb->tstamp);
+			else
+				getnstimeofday(&ts);
+			h->tp_sec = ts.tv_sec;
+			h->tp_usec = ts.tv_nsec; /* cheat a litle bit */
+		}
+		else {
+			if (skb->tstamp.tv64)
+				tv = ktime_to_timeval(skb->tstamp);
+			else
+				do_gettimeofday(&tv);
+			h->tp_sec = tv.tv_sec;
+			h->tp_usec = tv.tv_usec;
+		}
+	}
 
 	sll = (struct sockaddr_ll*)((u8*)h + TPACKET_ALIGN(sizeof(*h)));
 	sll->sll_halen = 0;

net/packet/af_packet.c static int packet_create(struct net *net, struct socket *sock, int protocol) · line 1035

Quoted 7 lines
net/packet/af_packet.c +1 -0
@@ -1014,6 +1029,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol)
 		sock->ops = &packet_ops_spkt;
 
 	sock_init_data(sock, sk);
+	sock_enable_timestamp(sk);
 
 	po = pkt_sk(sk);
 	sk->sk_family = PF_PACKET;

No, then we end up timestamping all the packets, even if they get dropped by packet filter. The change in 2.6.24 allows dhclient (and rstp) to only call hires clock source for packets they want, not all packets.

Quoted 6 lines

Perhaps the timestamping needs to change into a tristate flag? -
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric's patch has a feature your previous patch hasn't: a way to disable timestamping from userspace (the changes at net/core/sock.c). But it changes the userspace API.

I really think that any developer that sets SO_TIMESTAMP to 0 and still expect to receive valid timestamp is terminally insane and doesn't deserve any mercy. But we should take pity of these poor souls that uses (suffers) closed software and found another way that doesn't changes the API.

Bye.

author
#14 lore raw

Re: [RFC] af_packet: allow disabling timestamps

In reply to Stephen Hemminger

This small modification to Stephen's patch timestamps the skb when needed, so the timestamp can be reused by other af_packet sockets.

Signed-off-by Unai Uribarri <[email protected]>
net/core/sock.c +4 -2
@@ -259,7 +259,8 @@ static void sock_disable_timestamp(struct sock *sk)
 {
 	if (sock_flag(sk, SOCK_TIMESTAMP)) {
 		sock_reset_flag(sk, SOCK_TIMESTAMP);
-		net_disable_timestamp();
+		if (sk->sk_family != PF_PACKET)
+			net_disable_timestamp();
 	}
 }
 

@@ -1655,7 +1656,8 @@ void sock_enable_timestamp(struct sock *sk)
 {
 	if (!sock_flag(sk, SOCK_TIMESTAMP)) {
 		sock_set_flag(sk, SOCK_TIMESTAMP);
-		net_enable_timestamp();
+		if (sk->sk_family != PF_PACKET)
+			net_enable_timestamp();
 	}
 }
 EXPORT_SYMBOL(sock_enable_timestamp);
net/packet/af_packet.c +0 -0
@@ -570,7 +570,6 @@ static int tpacket_rcv(struct sk_buff *skb, struct

net_device *dev, struct packe
unsigned long status = TP_STATUS_LOSING|TP_STATUS_USER; unsigned short macoff, netoff;
struct sk_buff *copy_skb = NULL;
- struct timeval tv;

if (dev->nd_net != &init_net)
goto drop;
@@ -648,12 +647,18 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, struct packe
h->tp_snaplen = snaplen;
h->tp_mac = macoff;
h->tp_net = netoff;
- if (skb->tstamp.tv64)
+
+ if (sock_flag(sk, SOCK_TIMESTAMP)) {
+ struct timeval tv;
+ if (skb->tstamp.tv64 == 0)
+ __net_timestamp(skb);
tv = ktime_to_timeval(skb->tstamp);
- else
- do_gettimeofday(&tv);
- h->tp_sec = tv.tv_sec;
- h->tp_usec = tv.tv_usec;
+ h->tp_sec = tv.tv_sec;
+ h->tp_usec = tv.tv_usec;
+ } else {
+ h->tp_sec = 0;
+ h->tp_usec = 0;
+ }

sll = (struct sockaddr_ll*)((u8*)h + TPACKET_ALIGN(sizeof(*h))); sll->sll_halen = dev_parse_header(skb, sll->sll_addr);
@@ -1004,6 +1009,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol)
sock->ops = &packet_ops_spkt;

sock_init_data(sock, sk);
+ sock_set_flag(sk, SOCK_TIMESTAMP);

po = pkt_sk(sk);
sk->sk_family = PF_PACKET;

Quoted 81 lines · Stephen Hemminger

Currently, af_packet does not allow disabling timestamps. This patch changes that but doesn't force global timestamps on.

This shows up in bugzilla as:

	http://bugzilla.kernel.org/show_bug.cgi?id=4809

Patch against net-2.6.24 tree.

Signed-off-by Stephen Hemminger <[email protected]>
net/core/sock.c +4 -2
@@ -259,7 +259,8 @@ static void sock_disable_timestamp(struc
 {
 	if (sock_flag(sk, SOCK_TIMESTAMP)) {
 		sock_reset_flag(sk, SOCK_TIMESTAMP);
-		net_disable_timestamp();
+		if (sk->sk_family != PF_PACKET)
+			net_disable_timestamp();
 	}
 }
 

@@ -1645,7 +1646,8 @@ void sock_enable_timestamp(struct sock *
 {
 	if (!sock_flag(sk, SOCK_TIMESTAMP)) {
 		sock_set_flag(sk, SOCK_TIMESTAMP);
-		net_enable_timestamp();
+		if (sk->sk_family != PF_PACKET)
+			net_enable_timestamp();
 	}
 }
 EXPORT_SYMBOL(sock_enable_timestamp);
net/packet/af_packet.c +14 -8
@@ -572,7 +572,6 @@ static int tpacket_rcv(struct sk_buff *s
 	unsigned long status = TP_STATUS_LOSING|TP_STATUS_USER;
 	unsigned short macoff, netoff;
 	struct sk_buff *copy_skb = NULL;
-	struct timeval tv;
 
 	if (dev->nd_net != &init_net)
 		goto drop;

@@ -650,12 +649,19 @@ static int tpacket_rcv(struct sk_buff *s
 	h->tp_snaplen = snaplen;
 	h->tp_mac = macoff;
 	h->tp_net = netoff;
-	if (skb->tstamp.tv64)
-		tv = ktime_to_timeval(skb->tstamp);
-	else
-		do_gettimeofday(&tv);
-	h->tp_sec = tv.tv_sec;
-	h->tp_usec = tv.tv_usec;
+
+	if (sock_flag(sk, SOCK_TIMESTAMP)) {
+		struct timeval tv;
+		if (skb->tstamp.tv64)
+			tv = ktime_to_timeval(skb->tstamp);
+		else
+			do_gettimeofday(&tv);
+		h->tp_sec = tv.tv_sec;
+		h->tp_usec = tv.tv_usec;
+	} else {
+		h->tp_sec = 0;
+		h->tp_usec = 0;
+	}
 
 	sll = (struct sockaddr_ll*)((u8*)h + TPACKET_ALIGN(sizeof(*h)));
 	sll->sll_halen = 0;

@@ -1014,6 +1020,7 @@ static int packet_create(struct net *net
 		sock->ops = &packet_ops_spkt;
 
 	sock_init_data(sock, sk);
+	sock_set_flag(sk, SOCK_TIMESTAMP);
 
 	po = pkt_sk(sk);
 	sk->sk_family = PF_PACKET;
-

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html