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

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

[Unai Uribarri]

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 <unai.uribarri@optenet.com>

---

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 1322d62..a4f2da3 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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;

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

[Evgeniy Polyakov]

On Thu, Aug 09, 2007 at 04:21:54PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> 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?

-- 
	Evgeniy Polyakov

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

[Unai Uribarri]

On jue, 2007-08-09 at 18:33 +0400, Evgeniy Polyakov wrote:
> On Thu, Aug 09, 2007 at 04:21:54PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> > 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.

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

[Evgeniy Polyakov]

On Thu, Aug 09, 2007 at 08:13:54PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> On jue, 2007-08-09 at 18:33 +0400, Evgeniy Polyakov wrote:
> > On Thu, Aug 09, 2007 at 04:21:54PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> > > 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.
 
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?

-- 
	Evgeniy Polyakov

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

[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.
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.


On jue, 2007-08-09 at 22:18 +0400, Evgeniy Polyakov wrote:
> On Thu, Aug 09, 2007 at 08:13:54PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> > On jue, 2007-08-09 at 18:33 +0400, Evgeniy Polyakov wrote:
> > > On Thu, Aug 09, 2007 at 04:21:54PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> > > > 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.
>  
> 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?
> 

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

[Unai Uribarri]

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

---
commit d1d6e6bf196e31b6306fd0fef95f4190983c8a86
tree 22637506c0aafeabfbe05faf5352d0358c4d9460
parent 6a302358d87fedaf7bda12b8e909265ebf1ce674
author Unai Uribarri <unai.uribarri@optenet.com> Tue, 31 Jul 2007 20:38:42 +0200
committer Unai Uribarri <unai.uribarri@optenet.com> Tue, 31 Jul 2007 20:38:42 +0200

 net/packet/af_packet.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 1322d62..a4f2da3 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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 <unai.uribarri@optenet.com> Tue, 31 Jul 2007 20:43:00 +0200
committer Unai Uribarri <unai.uribarri@optenet.com> Tue, 31 Jul 2007 20:43:00 +0200

 net/core/sock.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index cfed7d4..3af2322 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -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 <unai.uribarri@optenet.com> Tue, 31 Jul 2007 20:43:59 +0200
committer Unai Uribarri <unai.uribarri@optenet.com> Tue, 31 Jul 2007 20:43:59 +0200

 net/packet/af_packet.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index a4f2da3..5179daf 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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-



On jue, 2007-08-09 at 22:18 +0400, Evgeniy Polyakov wrote:
> On Thu, Aug 09, 2007 at 08:13:54PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> > On jue, 2007-08-09 at 18:33 +0400, Evgeniy Polyakov wrote:
> > > On Thu, Aug 09, 2007 at 04:21:54PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> > > > 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.
>  
> 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?
> 

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

[Evgeniy Polyakov]

Hi Unai.

On Thu, Aug 09, 2007 at 08:44:21PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> 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.

> 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.

-- 
	Evgeniy Polyakov

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

[Unai Uribarri]

On vie, 2007-08-10 at 12:34 +0400, Evgeniy Polyakov wrote:
> Hi Unai.
> 
> On Thu, Aug 09, 2007 at 08:44:21PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> > 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.

> > 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.

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

[Evgeniy Polyakov]

On Fri, Aug 10, 2007 at 01:55:07PM +0200, Unai Uribarri (unai.uribarri@optenet.com) wrote:
> > 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? 

-- 
	Evgeniy Polyakov

[RFC] af_packet: allow disabling timestamps

[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 <shemminger@linux-foundation.org>

--- a/net/core/sock.c	2007-09-12 15:08:43.000000000 +0200
+++ b/net/core/sock.c	2007-09-13 12:10:19.000000000 +0200
@@ -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);
--- a/net/packet/af_packet.c	2007-09-12 17:07:00.000000000 +0200
+++ b/net/packet/af_packet.c	2007-09-13 12:09:10.000000000 +0200
@@ -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;

Re: [RFC] af_packet: allow disabling timestamps

[Eric Dumazet]

On Thu, 13 Sep 2007 12:42:53 +0200
Stephen Hemminger <shemminger@linux-foundation.org> wrote:

> 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 <dada1@cosmosbay.com>

diff --git a/net/core/sock.c b/net/core/sock.c
index 5a16e38..1c10b9d 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -563,6 +563,7 @@ set_rcvbuf:
 		} else {
 			sock_reset_flag(sk, SOCK_RCVTSTAMP);
 			sock_reset_flag(sk, SOCK_RCVTSTAMPNS);
+			sock_disable_timestamp(sk);
 		}
 		break;
 
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 745e2cb..409de44 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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;

Re: [RFC] af_packet: allow disabling timestamps

[Stephen Hemminger]

On Thu, 13 Sep 2007 14:24:06 +0200
Eric Dumazet <dada1@cosmosbay.com> wrote:

> On Thu, 13 Sep 2007 12:42:53 +0200
> Stephen Hemminger <shemminger@linux-foundation.org> wrote:
> 
> > 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 <dada1@cosmosbay.com>
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 5a16e38..1c10b9d 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -563,6 +563,7 @@ set_rcvbuf:
>  		} else {
>  			sock_reset_flag(sk, SOCK_RCVTSTAMP);
>  			sock_reset_flag(sk, SOCK_RCVTSTAMPNS);
> +			sock_disable_timestamp(sk);
>  		}
>  		break;
>  
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 745e2cb..409de44 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -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;

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?

Re: [RFC] af_packet: allow disabling timestamps

[Evgeniy Polyakov]

On Fri, Sep 14, 2007 at 12:26:42PM +0200, Stephen Hemminger (shemminger@linux-foundation.org) wrote:
> 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?

-- 
	Evgeniy Polyakov

Re: [RFC] af_packet: allow disabling timestamps

[Unai Uribarri]

On vie, 2007-09-14 at 12:26 +0200, Stephen Hemminger wrote:
> On Thu, 13 Sep 2007 14:24:06 +0200
> Eric Dumazet <dada1@cosmosbay.com> wrote:
> 
> > On Thu, 13 Sep 2007 12:42:53 +0200
> > Stephen Hemminger <shemminger@linux-foundation.org> wrote:
> > 
> > > 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 <dada1@cosmosbay.com>
> > 
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index 5a16e38..1c10b9d 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -563,6 +563,7 @@ set_rcvbuf:
> >  		} else {
> >  			sock_reset_flag(sk, SOCK_RCVTSTAMP);
> >  			sock_reset_flag(sk, SOCK_RCVTSTAMPNS);
> > +			sock_disable_timestamp(sk);
> >  		}
> >  		break;
> >  
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 745e2cb..409de44 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -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;
> 
> 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?
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> 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.

Re: [RFC] af_packet: allow disabling timestamps

[Unai Uribarri]

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 <unai.uribarri@optenet.com>

--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -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);
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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;



On jue, 2007-09-13 at 12:42 +0200, Stephen Hemminger wrote:
> 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 <shemminger@linux-foundation.org>
> 
> --- a/net/core/sock.c	2007-09-12 15:08:43.000000000 +0200
> +++ b/net/core/sock.c	2007-09-13 12:10:19.000000000 +0200
> @@ -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);
> --- a/net/packet/af_packet.c	2007-09-12 17:07:00.000000000 +0200
> +++ b/net/packet/af_packet.c	2007-09-13 12:09:10.000000000 +0200
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>