[PATCH net-next] sock: don't enable netstamp for af_unix sockets

netstamp_needed is toggled for all socket families if they request
timestamping. But some protocols don't need the lower-layer timestamping
code at all. This patch starts disabling it for af-unix.

E.g. systemd enables timestamping during boot-up on the journald af-unix
sockets, thus causing the system to globally enable timestamping in the
lower networking stack. Still, it is very probable that timestamping
gets activated, by e.g. dhclient or various NTP implementations.

Reported-by: Jesper Dangaard Brouer <redacted>
Signed-off-by: Hannes Frederic Sowa <redacted>
---
 net/core/sock.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index dcc7d62..0ef30aa 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c

@@ -422,13 +422,25 @@ static void sock_warn_obsolete_bsdism(const char *name)
 	}
 }
 
+static bool sock_needs_netstamp(const struct sock *sk)
+{
+	switch (sk->sk_family) {
+	case AF_UNSPEC:
+	case AF_UNIX:
+		return false;
+	default:
+		return true;
+	}
+}
+
 #define SK_FLAGS_TIMESTAMP ((1UL << SOCK_TIMESTAMP) | (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE))
 
 static void sock_disable_timestamp(struct sock *sk, unsigned long flags)
 {
 	if (sk->sk_flags & flags) {
 		sk->sk_flags &= ~flags;
-		if (!(sk->sk_flags & SK_FLAGS_TIMESTAMP))
+		if (sock_needs_netstamp(sk) &&
+		    !(sk->sk_flags & SK_FLAGS_TIMESTAMP))
 			net_disable_timestamp();
 	}
 }

@@ -1582,7 +1594,8 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
 		if (newsk->sk_prot->sockets_allocated)
 			sk_sockets_allocated_inc(newsk);
 
-		if (newsk->sk_flags & SK_FLAGS_TIMESTAMP)
+		if (sock_needs_netstamp(sk) &&
+		    newsk->sk_flags & SK_FLAGS_TIMESTAMP)
 			net_enable_timestamp();
 	}
 out:

@@ -2510,7 +2523,8 @@ void sock_enable_timestamp(struct sock *sk, int flag)
 		 * time stamping, but time stamping might have been on
 		 * already because of the other one
 		 */
-		if (!(previous_flags & SK_FLAGS_TIMESTAMP))
+		if (sock_needs_netstamp(sk) &&
+		    !(previous_flags & SK_FLAGS_TIMESTAMP))
 			net_enable_timestamp();
 	}
 }

-- 
2.5.0

On Mon, Oct 26, 2015 at 01:51:37PM +0100, Hannes Frederic Sowa wrote:

netstamp_needed is toggled for all socket families if they request
timestamping. But some protocols don't need the lower-layer timestamping
code at all. This patch starts disabling it for af-unix.

What problem is this patch trying to solve?

Thanks,
Richard

Hello,

On Mon, Oct 26, 2015, at 14:19, Richard Cochran wrote:

On Mon, Oct 26, 2015 at 01:51:37PM +0100, Hannes Frederic Sowa wrote:

quoted

netstamp_needed is toggled for all socket families if they request
timestamping. But some protocols don't need the lower-layer timestamping
code at all. This patch starts disabling it for af-unix.

What problem is this patch trying to solve?

netstamp_needed is a static-key which enables timestamping code in the
networking stack receive functions for every packet, while it is not
needed for AF_UNIX/LOCAL. So it is merely a small performance
enhancement.

Bye,
Hannes

On Mon, Oct 26, 2015 at 02:32:59PM +0100, Hannes Frederic Sowa wrote:

On Mon, Oct 26, 2015, at 14:19, Richard Cochran wrote:

quoted

On Mon, Oct 26, 2015 at 01:51:37PM +0100, Hannes Frederic Sowa wrote:

quoted

netstamp_needed is toggled for all socket families if they request
timestamping. But some protocols don't need the lower-layer timestamping
code at all. This patch starts disabling it for af-unix.

What problem is this patch trying to solve?

netstamp_needed is a static-key which enables timestamping code in the
networking stack receive functions for every packet, while it is not
needed for AF_UNIX/LOCAL. So it is merely a small performance
enhancement.

Are there any numbers that show the effect of this enhancement?

Thanks,
Richard

Hi Richard,

On Tue, Oct 27, 2015, at 11:11, Richard Cochran wrote:

On Mon, Oct 26, 2015 at 02:32:59PM +0100, Hannes Frederic Sowa wrote:

quoted

On Mon, Oct 26, 2015, at 14:19, Richard Cochran wrote:

quoted

On Mon, Oct 26, 2015 at 01:51:37PM +0100, Hannes Frederic Sowa wrote:

quoted

netstamp_needed is toggled for all socket families if they request
timestamping. But some protocols don't need the lower-layer timestamping
code at all. This patch starts disabling it for af-unix.

What problem is this patch trying to solve?

netstamp_needed is a static-key which enables timestamping code in the
networking stack receive functions for every packet, while it is not
needed for AF_UNIX/LOCAL. So it is merely a small performance
enhancement.

Are there any numbers that show the effect of this enhancement?

I haven't personally done any performance numbers.

Jesper (in Cc) noticed that it showed up in perf performance reports
even though he used a very minimal setup. Turned out that
systemd-journald enables timestamping on AF_UNIX sockets which thus
enabled netstamps globally. I think Jesper can chime in here.

Bye,
Hannes

On Tue, Oct 27, 2015, at 12:09, Hannes Frederic Sowa wrote:

Hi Richard,

On Tue, Oct 27, 2015, at 11:11, Richard Cochran wrote:

quoted

On Mon, Oct 26, 2015 at 02:32:59PM +0100, Hannes Frederic Sowa wrote:

quoted

On Mon, Oct 26, 2015, at 14:19, Richard Cochran wrote:

quoted

On Mon, Oct 26, 2015 at 01:51:37PM +0100, Hannes Frederic Sowa wrote:

quoted

netstamp_needed is toggled for all socket families if they request
timestamping. But some protocols don't need the lower-layer timestamping
code at all. This patch starts disabling it for af-unix.

What problem is this patch trying to solve?

netstamp_needed is a static-key which enables timestamping code in the
networking stack receive functions for every packet, while it is not
needed for AF_UNIX/LOCAL. So it is merely a small performance
enhancement.

Are there any numbers that show the effect of this enhancement?

I haven't personally done any performance numbers.

Jesper (in Cc) noticed that it showed up in perf performance reports
even though he used a very minimal setup. Turned out that
systemd-journald enables timestamping on AF_UNIX sockets which thus
enabled netstamps globally. I think Jesper can chime in here.

Also counter question: why is the netstamp code protected by a
static_key otherwise if not for trying to suppress the code path as
often as possible if not used? ;)

Bye,
Hannes

On Tue, 27 Oct 2015 12:15:16 +0100 Hannes Frederic Sowa [off-list ref] wrote:

On Tue, Oct 27, 2015, at 12:09, Hannes Frederic Sowa wrote:

quoted

Hi Richard,

On Tue, Oct 27, 2015, at 11:11, Richard Cochran wrote:

quoted

On Mon, Oct 26, 2015 at 02:32:59PM +0100, Hannes Frederic Sowa wrote:

quoted

On Mon, Oct 26, 2015, at 14:19, Richard Cochran wrote:

quoted

On Mon, Oct 26, 2015 at 01:51:37PM +0100, Hannes Frederic Sowa wrote:

quoted

netstamp_needed is toggled for all socket families if they request
timestamping. But some protocols don't need the lower-layer timestamping
code at all. This patch starts disabling it for af-unix.

What problem is this patch trying to solve?

netstamp_needed is a static-key which enables timestamping code in the
networking stack receive functions for every packet, while it is not
needed for AF_UNIX/LOCAL. So it is merely a small performance
enhancement.

Are there any numbers that show the effect of this enhancement?

I haven't personally done any performance numbers.

Jesper (in Cc) noticed that it showed up in perf performance reports
even though he used a very minimal setup. Turned out that
systemd-journald enables timestamping on AF_UNIX sockets which thus
enabled netstamps globally. I think Jesper can chime in here.

Well, it should be quite obvious that requesting a timestamp on every
packet is a fairly expensive, especially when not used for anything.

I can estimate the cost by looking at perf report, on a single-flow
IP-fwd test (1989575 pps) CPU i7-4790K @ 4.2GHz.

I quick IP-fwd test show perf top:
 1.54%  ksoftirqd/1  [kernel.vmlinux]  [k] read_tsc
 1.07%  ksoftirqd/1  [kernel.vmlinux]  [k] ktime_get_with_offset

(1/1989575*10^9)*((1.54+1.07)/100) = 13.12 nanosec

On some of my slower systems, I've seen cost of just reading TSC be
around 32 ns.

Also counter question: why is the netstamp code protected by a
static_key otherwise if not for trying to suppress the code path as
often as possible if not used? ;)

Exactly ;-)

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

On Tue, 2015-10-27 at 12:15 +0100, Hannes Frederic Sowa wrote:

Also counter question: why is the netstamp code protected by a
static_key otherwise if not for trying to suppress the code path as
often as possible if not used? ;)

Any idea of why timestamping is asked on AF_UNIX in the first place ?

For messages sent/received on af_unix sockets, in which place timestamp
is taken ?

Is it at the time skb is cooked and stored in receive queue, or the time
it was dequeued ?

In any case, is your patch changing af_unix behavior ? It is not clear
from your changelog...

On Tue, Oct 27, 2015, at 14:19, Eric Dumazet wrote:

On Tue, 2015-10-27 at 12:15 +0100, Hannes Frederic Sowa wrote:

quoted

Also counter question: why is the netstamp code protected by a
static_key otherwise if not for trying to suppress the code path as
often as possible if not used? ;)

Any idea of why timestamping is asked on AF_UNIX in the first place ?

I guess syslog code want's to have more accurate timetstamps on when the
packet is send.

For messages sent/received on af_unix sockets, in which place timestamp
is taken ?

in unix_sendmsg on the sending unix socket (we check peer unix socket
for timestamp flag).

Is it at the time skb is cooked and stored in receive queue, or the time
it was dequeued ?

No, at time it is send by sendmsg on the sending socket.

In any case, is your patch changing af_unix behavior ? It is not clear
from your changelog...

No, af_unix logic does not pass this logic at all, so we don't need to
care about netstamp code. netstamp_needed is private to dev.c.

Bye,
Hannes

On Tue, 2015-10-27 at 14:44 +0100, Hannes Frederic Sowa wrote:

On Tue, Oct 27, 2015, at 14:19, Eric Dumazet wrote:

quoted

On Tue, 2015-10-27 at 12:15 +0100, Hannes Frederic Sowa wrote:

quoted

Also counter question: why is the netstamp code protected by a
static_key otherwise if not for trying to suppress the code path as
often as possible if not used? ;)

Any idea of why timestamping is asked on AF_UNIX in the first place ?

I guess syslog code want's to have more accurate timetstamps on when the
packet is send.

quoted

For messages sent/received on af_unix sockets, in which place timestamp
is taken ?

in unix_sendmsg on the sending unix socket (we check peer unix socket
for timestamp flag).

quoted

Is it at the time skb is cooked and stored in receive queue, or the time
it was dequeued ?

No, at time it is send by sendmsg on the sending socket.

quoted

In any case, is your patch changing af_unix behavior ? It is not clear
from your changelog...

No, af_unix logic does not pass this logic at all, so we don't need to
care about netstamp code. netstamp_needed is private to dev.c.

Thanks for clarifying

Acked-by: Eric Dumazet <edumazet@google.com>

From: Hannes Frederic Sowa <redacted>
Date: Mon, 26 Oct 2015 13:51:37 +0100

netstamp_needed is toggled for all socket families if they request
timestamping. But some protocols don't need the lower-layer timestamping
code at all. This patch starts disabling it for af-unix.

E.g. systemd enables timestamping during boot-up on the journald af-unix
sockets, thus causing the system to globally enable timestamping in the
lower networking stack. Still, it is very probable that timestamping
gets activated, by e.g. dhclient or various NTP implementations.

Reported-by: Jesper Dangaard Brouer <redacted>
Signed-off-by: Hannes Frederic Sowa <redacted>

Applied.