| Message ID | 20230602163141.2115187-2-edumazet@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rfs: annotate lockless accesses | expand |
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
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.
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.
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 ;)
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. >
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 }
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(-)