[PATCH net-next] sock: avoid dirtying sk_stamp, if possible

[PATCH net-next] sock: avoid dirtying sk_stamp, if possible

[Paolo Abeni]

sock_recv_ts_and_drops() unconditionally set sk->sk_stamp for
every packet, even if the SOCK_TIMESTAMP flag is not set in the
related socket.
If selinux is enabled, this cause a cache miss for every packet
since sk->sk_stamp and sk->sk_security share the same cacheline.
With this change sk_stamp is set only if the SOCK_TIMESTAMP
flag is set, and is cleared for the first packet, so that the user
perceived behavior is unchanged.

This gives up to 5% speed-up under udp-flood with small packets.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/sock.h | 5 ++++-
 net/core/sock.c    | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index cb241a0..8e53158 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2239,6 +2239,7 @@ sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
 void __sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
 			      struct sk_buff *skb);
 
+#define SK_DEFAULT_STAMP (-1L * NSEC_PER_SEC)
 static inline void sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
 					  struct sk_buff *skb)
 {
@@ -2249,8 +2250,10 @@ static inline void sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
 
 	if (sk->sk_flags & FLAGS_TS_OR_DROPS || sk->sk_tsflags & TSFLAGS_ANY)
 		__sock_recv_ts_and_drops(msg, sk, skb);
-	else
+	else if (unlikely(sk->sk_flags & SOCK_TIMESTAMP))
 		sk->sk_stamp = skb->tstamp;
+	else if (unlikely(sk->sk_stamp == SK_DEFAULT_STAMP))
+		sk->sk_stamp = 0;
 }
 
 void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags);
diff --git a/net/core/sock.c b/net/core/sock.c
index 1a58a9d..392f9b6 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2613,7 +2613,7 @@ void sock_init_data(struct socket *sock, struct sock *sk)
 	sk->sk_rcvtimeo		=	MAX_SCHEDULE_TIMEOUT;
 	sk->sk_sndtimeo		=	MAX_SCHEDULE_TIMEOUT;
 
-	sk->sk_stamp = ktime_set(-1L, 0);
+	sk->sk_stamp = SK_DEFAULT_STAMP;
 
 #ifdef CONFIG_NET_RX_BUSY_POLL
 	sk->sk_napi_id		=	0;
-- 
2.9.3

Re: [PATCH net-next] sock: avoid dirtying sk_stamp, if possible

[Eric Dumazet]

On Thu, 2017-03-30 at 14:03 +0200, Paolo Abeni wrote:
> sock_recv_ts_and_drops() unconditionally set sk->sk_stamp for
> every packet, even if the SOCK_TIMESTAMP flag is not set in the
> related socket.
> If selinux is enabled, this cause a cache miss for every packet
> since sk->sk_stamp and sk->sk_security share the same cacheline.
> With this change sk_stamp is set only if the SOCK_TIMESTAMP
> flag is set, and is cleared for the first packet, so that the user
> perceived behavior is unchanged.
> 
> This gives up to 5% speed-up under udp-flood with small packets.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  include/net/sock.h | 5 ++++-
>  net/core/sock.c    | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index cb241a0..8e53158 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2239,6 +2239,7 @@ sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
>  void __sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
>  			      struct sk_buff *skb);
>  
> +#define SK_DEFAULT_STAMP (-1L * NSEC_PER_SEC)
>  static inline void sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
>  					  struct sk_buff *skb)
>  {
> @@ -2249,8 +2250,10 @@ static inline void sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
>  
>  	if (sk->sk_flags & FLAGS_TS_OR_DROPS || sk->sk_tsflags & TSFLAGS_ANY)
>  		__sock_recv_ts_and_drops(msg, sk, skb);
> -	else
> +	else if (unlikely(sk->sk_flags & SOCK_TIMESTAMP))
>  		sk->sk_stamp = skb->tstamp;
> +	else if (unlikely(sk->sk_stamp == SK_DEFAULT_STAMP))
> +		sk->sk_stamp = 0;
>  }
>  

This looks very nice, but why using 0 here instead of skb->tstamp ?

This might give some regression on applications reading their first
socket timestamp in some contexts.

What about 

if (sk->sk_flags & FLAGS_TS_OR_DROPS || sk->sk_tsflags & TSFLAGS_ANY)
	__sock_recv_ts_and_drops(msg, sk, skb);
else if (unlikely(sk->sk_flags & SOCK_TIMESTAMP ||
                  sk->sk_stamp == SK_DEFAULT_STAMP))
	sk->sk_stamp = skb->tstamp;

Re: [PATCH net-next] sock: avoid dirtying sk_stamp, if possible

[Paolo Abeni]

On Thu, 2017-03-30 at 06:52 -0700, Eric Dumazet wrote:
> On Thu, 2017-03-30 at 14:03 +0200, Paolo Abeni wrote:
> > sock_recv_ts_and_drops() unconditionally set sk->sk_stamp for
> > every packet, even if the SOCK_TIMESTAMP flag is not set in the
> > related socket.
> > If selinux is enabled, this cause a cache miss for every packet
> > since sk->sk_stamp and sk->sk_security share the same cacheline.
> > With this change sk_stamp is set only if the SOCK_TIMESTAMP
> > flag is set, and is cleared for the first packet, so that the user
> > perceived behavior is unchanged.
> > 
> > This gives up to 5% speed-up under udp-flood with small packets.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  include/net/sock.h | 5 ++++-
> >  net/core/sock.c    | 2 +-
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index cb241a0..8e53158 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -2239,6 +2239,7 @@ sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
> >  void __sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
> >  			      struct sk_buff *skb);
> >  
> > +#define SK_DEFAULT_STAMP (-1L * NSEC_PER_SEC)
> >  static inline void sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
> >  					  struct sk_buff *skb)
> >  {
> > @@ -2249,8 +2250,10 @@ static inline void sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
> >  
> >  	if (sk->sk_flags & FLAGS_TS_OR_DROPS || sk->sk_tsflags & TSFLAGS_ANY)
> >  		__sock_recv_ts_and_drops(msg, sk, skb);
> > -	else
> > +	else if (unlikely(sk->sk_flags & SOCK_TIMESTAMP))
> >  		sk->sk_stamp = skb->tstamp;
> > +	else if (unlikely(sk->sk_stamp == SK_DEFAULT_STAMP))
> > +		sk->sk_stamp = 0;
> >  }
> >  
> 
> This looks very nice, but why using 0 here instead of skb->tstamp ?

Thank you for reviewing this.

The network stack can already mark sk->sk_stamp with 0, if the
'netstamp_needed' static key is false when the packet is received.

'0' is used as a special value by sock_get_timestamp(), providing to
the caller the current ktime. 

This way the kernel is able to detect if no packets have been received
and to provide a somewhat valid timestamp for the last packet received 
before that the SOCK_TIMESTAMP flag was enabled; the assumption is that
the ioctl() follows closely the read call.

This should be the same behavior the user space already observes if net
timestamping is disabled when the SOCK_TIMESTAMP flag is set.

> This might give some regression on applications reading their first
> socket timestamp in some contexts.
> 
> What about 
> 
> if (sk->sk_flags & FLAGS_TS_OR_DROPS || sk->sk_tsflags & TSFLAGS_ANY)
> 	__sock_recv_ts_and_drops(msg, sk, skb);
> else if (unlikely(sk->sk_flags & SOCK_TIMESTAMP ||
>                   sk->sk_stamp == SK_DEFAULT_STAMP))
> 	sk->sk_stamp = skb->tstamp;

That way, if the net timestamp is enable, we will record the timestamp
of the first packet received by the socket (it can be far away in the
past).
I think is just a different kind of approximation.

Cheers,

Paolo

Re: [PATCH net-next] sock: avoid dirtying sk_stamp, if possible

[Eric Dumazet]

On Thu, 2017-03-30 at 16:23 +0200, Paolo Abeni wrote:

> That way, if the net timestamp is enable, we will record the timestamp
> of the first packet received by the socket (it can be far away in the
> past).
> I think is just a different kind of approximation.

I see.

This (64bit) sk_stamp stuff is quite buggy on 32bit kernels anyway.

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

Re: [PATCH net-next] sock: avoid dirtying sk_stamp, if possible

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Thu, 30 Mar 2017 14:03:06 +0200

> sock_recv_ts_and_drops() unconditionally set sk->sk_stamp for
> every packet, even if the SOCK_TIMESTAMP flag is not set in the
> related socket.
> If selinux is enabled, this cause a cache miss for every packet
> since sk->sk_stamp and sk->sk_security share the same cacheline.
> With this change sk_stamp is set only if the SOCK_TIMESTAMP
> flag is set, and is cleared for the first packet, so that the user
> perceived behavior is unchanged.
> 
> This gives up to 5% speed-up under udp-flood with small packets.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Applied, thanks.

[PATCH net-next] sock: correctly test SOCK_TIMESTAMP in sock_recv_ts_and_drops()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

It seems the code does not match the intent.

This broke packetdrill, and probably other programs.

Fixes: 6c7c98bad488 ("sock: avoid dirtying sk_stamp, if possible")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
---
 include/net/sock.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 8e53158a7d957ea2a480cc449606dca2480b1259..66349e49d468646ce724485bb8e74952825f0d6c 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2250,7 +2250,7 @@ static inline void sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
 
 	if (sk->sk_flags & FLAGS_TS_OR_DROPS || sk->sk_tsflags & TSFLAGS_ANY)
 		__sock_recv_ts_and_drops(msg, sk, skb);
-	else if (unlikely(sk->sk_flags & SOCK_TIMESTAMP))
+	else if (unlikely(sock_flag(sk, SOCK_TIMESTAMP)))
 		sk->sk_stamp = skb->tstamp;
 	else if (unlikely(sk->sk_stamp == SK_DEFAULT_STAMP))
 		sk->sk_stamp = 0;

Re: [PATCH net-next] sock: correctly test SOCK_TIMESTAMP in sock_recv_ts_and_drops()

[Paolo Abeni]

On Fri, 2017-03-31 at 14:59 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> It seems the code does not match the intent.
> 
> This broke packetdrill, and probably other programs.
> 
> Fixes: 6c7c98bad488 ("sock: avoid dirtying sk_stamp, if possible")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> ---
>  include/net/sock.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 8e53158a7d957ea2a480cc449606dca2480b1259..66349e49d468646ce724485bb8e74952825f0d6c 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2250,7 +2250,7 @@ static inline void sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
>  
>  	if (sk->sk_flags & FLAGS_TS_OR_DROPS || sk->sk_tsflags & TSFLAGS_ANY)
>  		__sock_recv_ts_and_drops(msg, sk, skb);
> -	else if (unlikely(sk->sk_flags & SOCK_TIMESTAMP))
> +	else if (unlikely(sock_flag(sk, SOCK_TIMESTAMP)))
>  		sk->sk_stamp = skb->tstamp;
>  	else if (unlikely(sk->sk_stamp == SK_DEFAULT_STAMP))
>  		sk->sk_stamp = 0;
> 

Oh, my bad! 

Thanks Eric for fixing this.

Acked-by: Paolo Abeni <pabeni@redhat.com>

Re: [PATCH net-next] sock: correctly test SOCK_TIMESTAMP in sock_recv_ts_and_drops()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 31 Mar 2017 14:59:25 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> It seems the code does not match the intent.
> 
> This broke packetdrill, and probably other programs.
> 
> Fixes: 6c7c98bad488 ("sock: avoid dirtying sk_stamp, if possible")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Paolo Abeni <pabeni@redhat.com>

Applied, thanks.