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

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

[Hannes Frederic Sowa]

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 <brouer@redhat.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 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

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

[Richard Cochran]

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

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

[Hannes Frederic Sowa]

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

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

[Richard Cochran]

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:
> > 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?
> 
> 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

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

[Hannes Frederic Sowa]

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

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

[Hannes Frederic Sowa]

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:
> > 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:
> > > > 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?
> > > 
> > > 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

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

[Jesper Dangaard Brouer]

On Tue, 27 Oct 2015 12:15:16 +0100 Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:

> 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:
> > > 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:
> > > > > 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?
> > > > 
> > > > 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

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

[Eric Dumazet]

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

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

[Hannes Frederic Sowa]

On Tue, Oct 27, 2015, at 14:19, Eric Dumazet wrote:
> 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 ?

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

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

[Eric Dumazet]

On Tue, 2015-10-27 at 14:44 +0100, Hannes Frederic Sowa wrote:
> 
> On Tue, Oct 27, 2015, at 14:19, Eric Dumazet wrote:
> > 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 ?
> 
> 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.

Thanks for clarifying

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

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

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
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 <brouer@redhat.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Applied.