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

[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