Re: [PATCH RT 2/6] tcp: Remove superfluous BH-disable around listening_hash

[Sebastian Andrzej Siewior]

On 2020-11-09 12:01:06 [-0500], Steven Rostedt wrote:
> Note, I took this because it mentioned:
> 
> "Commit
>9652dc2eb9e40 ("tcp: relax listening_hash operations")
> 
> removed the need to disable bottom half while acquiring
> listening_hash.lock. There are still two callers left which disable
> bottom half before the lock is acquired."
> 
> And that commit was added in 4.10.

Yes. I was aiming to sell this as an optimisation to upstream but that
patch isn't exactly a beauty queen and that optimisation isn't enormous
so they may not want to buy it.
Also: upstream's lockdep isn't complaining here so the alternative route
is to get RT's lockdep to be quiet here as well and then drop that
patch.

Sebastian

Re: [PATCH RT 2/6] tcp: Remove superfluous BH-disable around listening_hash

[Steven Rostedt]

On Mon, 9 Nov 2020 10:22:31 +0100
Sebastian Andrzej Siewior  wrote:

> On 2020-11-06 21:06:38 [-0500], Steven Rostedt wrote:
> > 5.4.74-rt42-rc1 stable review patch.
> > If anyone has any objections, please let me know.  
> 
> Please drop that one. Lockep complains on RT with newer softirq code.
> Older RT and mainline does not complain here unless it observes
> inet_listen_hashbucket::lock in serving-softirq context (which is not
> the case).
> 

Note, I took this because it mentioned:

"Commit
   9652dc2eb9e40 ("tcp: relax listening_hash operations")

removed the need to disable bottom half while acquiring
listening_hash.lock. There are still two callers left which disable
bottom half before the lock is acquired."

And that commit was added in 4.10.

I will remove it and release a rc2.

Thanks!

-- Steve

Re: [PATCH RT 2/6] tcp: Remove superfluous BH-disable around listening_hash

[Sebastian Andrzej Siewior]

On 2020-11-06 21:06:38 [-0500], Steven Rostedt wrote:
> 5.4.74-rt42-rc1 stable review patch.
> If anyone has any objections, please let me know.

Please drop that one. Lockep complains on RT with newer softirq code.
Older RT and mainline does not complain here unless it observes
inet_listen_hashbucket::lock in serving-softirq context (which is not
the case).

Sebastian

[PATCH RT 2/6] tcp: Remove superfluous BH-disable around listening_hash

[Steven Rostedt]

5.4.74-rt42-rc1 stable review patch.
If anyone has any objections, please let me know.

--

From: Sebastian Andrzej Siewior 

Commit
   9652dc2eb9e40 ("tcp: relax listening_hash operations")

removed the need to disable bottom half while acquiring
listening_hash.lock. There are still two callers left which disable
bottom half before the lock is acquired.

Drop local_bh_disable() around __inet_hash() which acquires
listening_hash->lock, invoke inet_ehash_nolisten() with disabled BH.
inet_unhash() conditionally acquires listening_hash->lock.

Signed-off-by: Sebastian Andrzej Siewior 
Signed-off-by: Steven Rostedt (VMware) 
---
 net/ipv4/inet_hashtables.c  | 19 ---
 net/ipv6/inet6_hashtables.c |  5 +
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 006a34b18537..4c8565d6624c 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -543,7 +543,9 @@ int __inet_hash(struct sock *sk, struct sock *osk)
int err = 0;
 
if (sk->sk_state != TCP_LISTEN) {
+   local_bh_disable();
inet_ehash_nolisten(sk, osk);
+   local_bh_enable();
return 0;
}
WARN_ON(!sk_unhashed(sk));
@@ -575,11 +577,8 @@ int inet_hash(struct sock *sk)
 {
int err = 0;
 
-   if (sk->sk_state != TCP_CLOSE) {
-   local_bh_disable();
+   if (sk->sk_state != TCP_CLOSE)
err = __inet_hash(sk, NULL);
-   local_bh_enable();
-   }
 
return err;
 }
@@ -590,17 +589,20 @@ void inet_unhash(struct sock *sk)
struct inet_hashinfo *hashinfo = sk->sk_prot->h.hashinfo;
struct inet_listen_hashbucket *ilb = NULL;
spinlock_t *lock;
+   bool state_listen;
 
if (sk_unhashed(sk))
return;
 
if (sk->sk_state == TCP_LISTEN) {
+   state_listen = true;
ilb = >listening_hash[inet_sk_listen_hashfn(sk)];
-   lock = >lock;
+   spin_lock(>lock);
} else {
+   state_listen = false;
lock = inet_ehash_lockp(hashinfo, sk->sk_hash);
+   spin_lock_bh(lock);
}
-   spin_lock_bh(lock);
if (sk_unhashed(sk))
goto unlock;
 
@@ -613,7 +615,10 @@ void inet_unhash(struct sock *sk)
__sk_nulls_del_node_init_rcu(sk);
sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
 unlock:
-   spin_unlock_bh(lock);
+   if (state_listen)
+   spin_unlock(>lock);
+   else
+   spin_unlock_bh(lock);
 }
 EXPORT_SYMBOL_GPL(inet_unhash);
 
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index fbe9d4295eac..5d1c1c6967cb 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -287,11 +287,8 @@ int inet6_hash(struct sock *sk)
 {
int err = 0;
 
-   if (sk->sk_state != TCP_CLOSE) {
-   local_bh_disable();
+   if (sk->sk_state != TCP_CLOSE)
err = __inet_hash(sk, NULL);
-   local_bh_enable();
-   }
 
return err;
 }
-- 
2.28.0