[I meant to cc netdev on this originally. Added now, preserving entire
message below for context]
On Fri, Jun 05, 2020 at 05:12:05AM -0700, Matthew Wilcox wrote:
> While doing the XArray conversion, I came across your commit
> f16a4b26f31f95dddb12cf3c2390906a735203ae
>
> synchronize_rcu() is kind of expensive on larger systems. Is there a
> reason to call it instead of using RCU to free the socket?
>
> I don't know whether I've set the flag in the right place, but maybe
> something like this would be a good idea?
>
> @@ -663,13 +663,8 @@ static void qrtr_port_remove(struct qrtr_sock *ipc)
> if (port == QRTR_PORT_CTRL)
> port = 0;
>
> - __sock_put(&ipc->sk);
> -
> xa_erase(&qrtr_ports, port);
> -
> - /* Ensure that if qrtr_port_lookup() did enter the RCU read section we
> - * wait for it to up increment the refcount */
> - synchronize_rcu();
> + __sock_put(&ipc->sk);
> }
>
> /* Assign port number to socket.
> @@ -749,6 +744,7 @@ static int __qrtr_bind(struct socket *sock,
> qrtr_port_remove(ipc);
> ipc->us.sq_port = port;
>
> + sock_set_flag(sk, SOCK_RCU_FREE);
> sock_reset_flag(sk, SOCK_ZAPPED);
>
> /* Notify all open ports about the new controller */
I realised this isn't sufficient. If we want to eliminate the
synchronize_rcu() call, we need to have a 'try' variant of sock_hold().
It would look something like this:
static inline __must_check bool sock_try_get(struct sock *sk)
{
return refcount_inc_not_zero(&sk->sk_refcnt);
}
and then ...
rcu_read_lock();
ipc = xa_load(&qrtr_ports, port);
if (ipc && !sock_try_get(&ipc->sk))
ipc = NULL;
rcu_read_unlock();
If we wanted to be able to atomically replace a pointer in the qrtr_ports
array with another without ever seeing NULL in between, we'd want to
make this:
rcu_read_lock():
do {
ipc = xa_load(&qrtr_ports, port);
} while (ipc && !sock_try_get(&ipc->sk));
rcu_read_unlock();
but as far as I can tell, we don't need to do that for qrtr.
