[PATCH net 0/2] rfs: annotate lockless accesses

[PATCH net 0/2] rfs: annotate lockless accesses

[Eric Dumazet]

rfs runs without locks held, so we should annotate
read and writes to shared variables.

It should prevent compilers forcing writes
in the following situation:

  if (var != val)
     var = val;

A compiler could indeed simply avoid the conditional:

    var = val;

This matters if var is shared between many cpus.

Eric Dumazet (2):
  rfs: annotate lockless accesses to sk->sk_rxhash
  rfs: annotate lockless accesses to RFS sock flow table

 include/linux/netdevice.h |  7 +++++--
 include/net/sock.h        | 18 +++++++++++++-----
 net/core/dev.c            |  6 ++++--
 3 files changed, 22 insertions(+), 9 deletions(-)

-- 
2.41.0.rc0.172.g3f132b7071-goog

[PATCH net 1/2] rfs: annotate lockless accesses to sk->sk_rxhash

[Eric Dumazet]

Add READ_ONCE()/WRITE_ONCE() on accesses to sk->sk_rxhash.

This also prevents a (smart ?) compiler to remove the condition in:

if (sk->sk_rxhash != newval)
	sk->sk_rxhash = newval;

We need the condition to avoid dirtying a shared cache line.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/sock.h | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index b418425d7230c8cee81df34fcc66d771ea5085e9..bf71855d47feccda716b3cabf259d6055b764a3c 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1152,8 +1152,12 @@ static inline void sock_rps_record_flow(const struct sock *sk)
 		 * OR	an additional socket flag
 		 * [1] : sk_state and sk_prot are in the same cache line.
 		 */
-		if (sk->sk_state == TCP_ESTABLISHED)
-			sock_rps_record_flow_hash(sk->sk_rxhash);
+		if (sk->sk_state == TCP_ESTABLISHED) {
+			/* This READ_ONCE() is paired with the WRITE_ONCE()
+			 * from sock_rps_save_rxhash() and sock_rps_reset_rxhash().
+			 */
+			sock_rps_record_flow_hash(READ_ONCE(sk->sk_rxhash));
+			}
 	}
 #endif
 }
@@ -1162,15 +1166,19 @@ static inline void sock_rps_save_rxhash(struct sock *sk,
 					const struct sk_buff *skb)
 {
 #ifdef CONFIG_RPS
-	if (unlikely(sk->sk_rxhash != skb->hash))
-		sk->sk_rxhash = skb->hash;
+	/* The following WRITE_ONCE() is paired with the READ_ONCE()
+	 * here, and another one in sock_rps_record_flow().
+	 */
+	if (unlikely(READ_ONCE(sk->sk_rxhash) != skb->hash))
+		WRITE_ONCE(sk->sk_rxhash, skb->hash);
 #endif
 }
 
 static inline void sock_rps_reset_rxhash(struct sock *sk)
 {
 #ifdef CONFIG_RPS
-	sk->sk_rxhash = 0;
+	/* Paired with READ_ONCE() in sock_rps_record_flow() */
+	WRITE_ONCE(sk->sk_rxhash, 0);
 #endif
 }
 
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH net 1/2] rfs: annotate lockless accesses to sk->sk_rxhash

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Fri,  2 Jun 2023 16:31:40 +0000
> Add READ_ONCE()/WRITE_ONCE() on accesses to sk->sk_rxhash.
> 
> This also prevents a (smart ?) compiler to remove the condition in:
> 
> if (sk->sk_rxhash != newval)
> 	sk->sk_rxhash = newval;
> 
> We need the condition to avoid dirtying a shared cache line.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/net/sock.h | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index b418425d7230c8cee81df34fcc66d771ea5085e9..bf71855d47feccda716b3cabf259d6055b764a3c 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1152,8 +1152,12 @@ static inline void sock_rps_record_flow(const struct sock *sk)
>  		 * OR	an additional socket flag
>  		 * [1] : sk_state and sk_prot are in the same cache line.
>  		 */
> -		if (sk->sk_state == TCP_ESTABLISHED)
> -			sock_rps_record_flow_hash(sk->sk_rxhash);
> +		if (sk->sk_state == TCP_ESTABLISHED) {
> +			/* This READ_ONCE() is paired with the WRITE_ONCE()
> +			 * from sock_rps_save_rxhash() and sock_rps_reset_rxhash().
> +			 */
> +			sock_rps_record_flow_hash(READ_ONCE(sk->sk_rxhash));
> +			}

nit: unnecessary \t here

other than that

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Thanks!

>  	}
>  #endif
>  }
> @@ -1162,15 +1166,19 @@ static inline void sock_rps_save_rxhash(struct sock *sk,
>  					const struct sk_buff *skb)
>  {
>  #ifdef CONFIG_RPS
> -	if (unlikely(sk->sk_rxhash != skb->hash))
> -		sk->sk_rxhash = skb->hash;
> +	/* The following WRITE_ONCE() is paired with the READ_ONCE()
> +	 * here, and another one in sock_rps_record_flow().
> +	 */
> +	if (unlikely(READ_ONCE(sk->sk_rxhash) != skb->hash))
> +		WRITE_ONCE(sk->sk_rxhash, skb->hash);
>  #endif
>  }
>  
>  static inline void sock_rps_reset_rxhash(struct sock *sk)
>  {
>  #ifdef CONFIG_RPS
> -	sk->sk_rxhash = 0;
> +	/* Paired with READ_ONCE() in sock_rps_record_flow() */
> +	WRITE_ONCE(sk->sk_rxhash, 0);
>  #endif
>  }
>  
> -- 
> 2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH net 1/2] rfs: annotate lockless accesses to sk->sk_rxhash

[Jakub Kicinski]

On Fri,  2 Jun 2023 16:31:40 +0000 Eric Dumazet wrote:
> +		if (sk->sk_state == TCP_ESTABLISHED) {
> +			/* This READ_ONCE() is paired with the WRITE_ONCE()
> +			 * from sock_rps_save_rxhash() and sock_rps_reset_rxhash().
> +			 */
> +			sock_rps_record_flow_hash(READ_ONCE(sk->sk_rxhash));
> +			}

Hi Eric, the series got "changes requested", a bit unclear why, 
I'm guessing it's because it lacks Fixes tags.

I also noticed that the closing bracket above looks misaligned.

Would you mind reposting? If you prefer not to add Fixes tag
a mention that it's intentional in the cover letter is enough.

Re: [PATCH net 1/2] rfs: annotate lockless accesses to sk->sk_rxhash

[Eric Dumazet]

On Tue, Jun 6, 2023 at 12:52 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri,  2 Jun 2023 16:31:40 +0000 Eric Dumazet wrote:
> > +             if (sk->sk_state == TCP_ESTABLISHED) {
> > +                     /* This READ_ONCE() is paired with the WRITE_ONCE()
> > +                      * from sock_rps_save_rxhash() and sock_rps_reset_rxhash().
> > +                      */
> > +                     sock_rps_record_flow_hash(READ_ONCE(sk->sk_rxhash));
> > +                     }
>
> Hi Eric, the series got "changes requested", a bit unclear why,
> I'm guessing it's because it lacks Fixes tags.
>
> I also noticed that the closing bracket above looks misaligned.

Right I think Simon gave this feedback.

>
> Would you mind reposting? If you prefer not to add Fixes tag
> a mention that it's intentional in the cover letter is enough.

Yes, I do not think a Fixes: tag is necessary.

I will post a v2 with an aligned closing bracket.

Thanks.

Re: [PATCH net 1/2] rfs: annotate lockless accesses to sk->sk_rxhash

[Eric Dumazet]

On Tue, Jun 6, 2023 at 9:30 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Jun 6, 2023 at 12:52 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Fri,  2 Jun 2023 16:31:40 +0000 Eric Dumazet wrote:
> > > +             if (sk->sk_state == TCP_ESTABLISHED) {
> > > +                     /* This READ_ONCE() is paired with the WRITE_ONCE()
> > > +                      * from sock_rps_save_rxhash() and sock_rps_reset_rxhash().
> > > +                      */
> > > +                     sock_rps_record_flow_hash(READ_ONCE(sk->sk_rxhash));
> > > +                     }
> >
> > Hi Eric, the series got "changes requested", a bit unclear why,
> > I'm guessing it's because it lacks Fixes tags.
> >
> > I also noticed that the closing bracket above looks misaligned.
>
> Right I think Simon gave this feedback.

Oops, that was Kuniyuki

Kuniyuki, do you mind adding your Reviewed-by: tag that I forgot to add ?

>
> >
> > Would you mind reposting? If you prefer not to add Fixes tag
> > a mention that it's intentional in the cover letter is enough.
>
> Yes, I do not think a Fixes: tag is necessary.

I added them, because why not ;)

Re: [PATCH net 1/2] rfs: annotate lockless accesses to sk->sk_rxhash

[Simon Horman]

On Tue, Jun 06, 2023 at 09:30:12AM +0200, Eric Dumazet wrote:
> On Tue, Jun 6, 2023 at 12:52 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Fri,  2 Jun 2023 16:31:40 +0000 Eric Dumazet wrote:
> > > +             if (sk->sk_state == TCP_ESTABLISHED) {
> > > +                     /* This READ_ONCE() is paired with the WRITE_ONCE()
> > > +                      * from sock_rps_save_rxhash() and sock_rps_reset_rxhash().
> > > +                      */
> > > +                     sock_rps_record_flow_hash(READ_ONCE(sk->sk_rxhash));
> > > +                     }
> >
> > Hi Eric, the series got "changes requested", a bit unclear why,
> > I'm guessing it's because it lacks Fixes tags.
> >
> > I also noticed that the closing bracket above looks misaligned.
> 
> Right I think Simon gave this feedback.

FWIWI, I think it was Kuniyuki.

> 
> >
> > Would you mind reposting? If you prefer not to add Fixes tag
> > a mention that it's intentional in the cover letter is enough.
> 
> Yes, I do not think a Fixes: tag is necessary.
> 
> I will post a v2 with an aligned closing bracket.
> 
> Thanks.
> 

[PATCH net 2/2] rfs: annotate lockless accesses to RFS sock flow table

[Eric Dumazet]

Add READ_ONCE()/WRITE_ONCE() on accesses to the sock flow table.

This also prevents a (smart ?) compiler to remove the condition in:

if (table->ents[index] != newval)
        table->ents[index] = newval;

We need the condition to avoid dirtying a shared cache line.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/netdevice.h | 7 +++++--
 net/core/dev.c            | 6 ++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 08fbd4622ccf731daaee34ad99773d6dc2e82fa6..e6f22b7403d014a2cf4d81d931109a594ce1398e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -768,8 +768,11 @@ static inline void rps_record_sock_flow(struct rps_sock_flow_table *table,
 		/* We only give a hint, preemption can change CPU under us */
 		val |= raw_smp_processor_id();
 
-		if (table->ents[index] != val)
-			table->ents[index] = val;
+		/* The following WRITE_ONCE() is paired with the READ_ONCE()
+		 * here, and another one in get_rps_cpu().
+		 */
+		if (READ_ONCE(table->ents[index]) != val)
+			WRITE_ONCE(table->ents[index], val);
 	}
 }
 
diff --git a/net/core/dev.c b/net/core/dev.c
index b3c13e0419356b943e90b1f46dd7e035c6ec1a9c..1495f8aff288e944c8cab21297f244a6fcde752f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4471,8 +4471,10 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
 		u32 next_cpu;
 		u32 ident;
 
-		/* First check into global flow table if there is a match */
-		ident = sock_flow_table->ents[hash & sock_flow_table->mask];
+		/* First check into global flow table if there is a match.
+		 * This READ_ONCE() pairs with WRITE_ONCE() from rps_record_sock_flow().
+		 */
+		ident = READ_ONCE(sock_flow_table->ents[hash & sock_flow_table->mask]);
 		if ((ident ^ hash) & ~rps_cpu_mask)
 			goto try_rps;
 
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH net 2/2] rfs: annotate lockless accesses to RFS sock flow table

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Fri,  2 Jun 2023 16:31:41 +0000
> Add READ_ONCE()/WRITE_ONCE() on accesses to the sock flow table.
> 
> This also prevents a (smart ?) compiler to remove the condition in:
> 
> if (table->ents[index] != newval)
>         table->ents[index] = newval;
> 
> We need the condition to avoid dirtying a shared cache line.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Thanks!


> ---
>  include/linux/netdevice.h | 7 +++++--
>  net/core/dev.c            | 6 ++++--
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 08fbd4622ccf731daaee34ad99773d6dc2e82fa6..e6f22b7403d014a2cf4d81d931109a594ce1398e 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -768,8 +768,11 @@ static inline void rps_record_sock_flow(struct rps_sock_flow_table *table,
>  		/* We only give a hint, preemption can change CPU under us */
>  		val |= raw_smp_processor_id();
>  
> -		if (table->ents[index] != val)
> -			table->ents[index] = val;
> +		/* The following WRITE_ONCE() is paired with the READ_ONCE()
> +		 * here, and another one in get_rps_cpu().
> +		 */
> +		if (READ_ONCE(table->ents[index]) != val)
> +			WRITE_ONCE(table->ents[index], val);
>  	}
>  }
>  
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b3c13e0419356b943e90b1f46dd7e035c6ec1a9c..1495f8aff288e944c8cab21297f244a6fcde752f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4471,8 +4471,10 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
>  		u32 next_cpu;
>  		u32 ident;
>  
> -		/* First check into global flow table if there is a match */
> -		ident = sock_flow_table->ents[hash & sock_flow_table->mask];
> +		/* First check into global flow table if there is a match.
> +		 * This READ_ONCE() pairs with WRITE_ONCE() from rps_record_sock_flow().
> +		 */
> +		ident = READ_ONCE(sock_flow_table->ents[hash & sock_flow_table->mask]);
>  		if ((ident ^ hash) & ~rps_cpu_mask)
>  			goto try_rps;
>  
> -- 
> 2.41.0.rc0.172.g3f132b7071-goog