[Openvpn-devel] [PATCH ovpn net] ovpn: batch socket release to call synchronize_net() once

unlock_ovpn() released each queued peer's socket via ovpn_socket_release(),
which runs a synchronize_rcu() per call before freeing the socket.
Tearing down N peers serializes N grace periods.

Split ovpn_socket_release() around the grace period. The new
ovpn_socket_release_prepare() detaches peer->sock and drops the ref,
returning the socket once its refcount hits zero.
ovpn_socket_release_finish() frees it afterwards. unlock_ovpn() now
prepares all sockets, waits once, then finishes them and drops the peer
refs. The wait is skipped when nothing needs freeing. ovpn_socket_release()
chains both halves to keep its behaviour.

Switch synchronize_rcu() to synchronize_net() while at it.

Signed-off-by: Marco Baffo <[email protected]>
---
 drivers/net/ovpn/peer.c   | 22 +++++++--
 drivers/net/ovpn/socket.c | 96 +++++++++++++++++++++++++++------------
 drivers/net/ovpn/socket.h |  4 ++
 3 files changed, 87 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index a21d02ac715e..c2110d8435e6 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -26,15 +26,27 @@ static void unlock_ovpn(struct ovpn_priv *ovpn,
 			 struct llist_head *release_list)
 	__releases(&ovpn->lock)
 {
-	struct ovpn_peer *peer, *next;
+	struct ovpn_peer *peer, *next_peer;
+	struct ovpn_socket *sock, *next_sock;
+	LIST_HEAD(sock_release_list);
 
 	spin_unlock_bh(&ovpn->lock);
 
-	llist_for_each_entry_safe(peer, next, release_list->first,
-				  release_entry) {
-		ovpn_socket_release(peer);
-		ovpn_peer_put(peer);
+	llist_for_each_entry(peer, release_list->first, release_entry) {
+		sock = ovpn_socket_release_prepare(peer);
+		if (!sock)
+			continue;
+		list_add_tail(&sock->release_entry, &sock_release_list);
 	}
+
+	if (!list_empty(&sock_release_list))
+		synchronize_net();
+
+	list_for_each_entry_safe(sock, next_sock, &sock_release_list, release_entry)
+		ovpn_socket_release_finish(sock);
+
+	llist_for_each_entry_safe(peer, next_peer, release_list->first, release_entry)
+		ovpn_peer_put(peer);
 }
 
 /**
diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
index 517caa64a4fe..dd27b79a062e 100644
--- a/drivers/net/ovpn/socket.c
+++ b/drivers/net/ovpn/socket.c
@@ -45,6 +45,69 @@ static bool ovpn_socket_put(struct ovpn_peer *peer, struct ovpn_socket *sock)
 	return kref_put(&sock->refcount, ovpn_socket_release_kref);
 }
 
+/**
+ * ovpn_socket_release_prepare - detach peer from its socket
+ * @peer: peer whose socket should be detached
+ *
+ * Return: the detached socket if its refcount reached zero, NULL otherwise.
+ * If a socket is returned, the caller must call synchronize_net() before
+ * invoking ovpn_socket_release_finish().
+ */
+struct ovpn_socket *ovpn_socket_release_prepare(struct ovpn_peer *peer)
+{
+	struct ovpn_socket *sock;
+	bool released;
+
+	might_sleep();
+
+	sock = rcu_replace_pointer(peer->sock, NULL, true);
+	/* release may be invoked after socket was detached */
+	if (!sock)
+		return NULL;
+
+	/* Drop the reference while holding the sock lock to avoid
+	 * concurrent ovpn_socket_new call to mess up with a partially
+	 * detached socket.
+	 *
+	 * Holding the lock ensures that a socket with refcnt 0 is fully
+	 * detached before it can be picked by a concurrent reader.
+	 */
+	lock_sock(sock->sk);
+	released = ovpn_socket_put(peer, sock);
+	release_sock(sock->sk);
+
+	if (released)
+		return sock;
+
+	return NULL;
+}
+
+/**
+ * ovpn_socket_release_finish - complete release of a detached socket
+ * @sock: socket whose refcount reached zero
+ *
+ * The caller must have called synchronize_net() after
+ * ovpn_socket_release_prepare() returned this socket.
+ */
+void ovpn_socket_release_finish(struct ovpn_socket *sock)
+{
+	might_sleep();
+
+	if (sock->sk->sk_protocol == IPPROTO_UDP) {
+		netdev_put(sock->ovpn->dev, &sock->dev_tracker);
+	} else if (sock->sk->sk_protocol == IPPROTO_TCP) {
+		/* wait for TCP jobs to terminate */
+		ovpn_tcp_socket_wait_finish(sock);
+		ovpn_peer_put(sock->peer);
+	}
+	/* drop reference acquired in ovpn_socket_new() */
+	sock_put(sock->sk);
+	/* we can call plain kfree() because we already waited one RCU
+	 * period due to synchronize_net()
+	 */
+	kfree(sock);
+}
+
 /**
  * ovpn_socket_release - release resources owned by socket user
  * @peer: peer whose socket should be released
@@ -66,45 +129,18 @@ static bool ovpn_socket_put(struct ovpn_peer *peer, struct ovpn_socket *sock)
 void ovpn_socket_release(struct ovpn_peer *peer)
 {
 	struct ovpn_socket *sock;
-	bool released;
 
 	might_sleep();
 
-	sock = rcu_replace_pointer(peer->sock, NULL, true);
-	/* release may be invoked after socket was detached */
+	sock = ovpn_socket_release_prepare(peer);
 	if (!sock)
 		return;
 
-	/* Drop the reference while holding the sock lock to avoid
-	 * concurrent ovpn_socket_new call to mess up with a partially
-	 * detached socket.
-	 *
-	 * Holding the lock ensures that a socket with refcnt 0 is fully
-	 * detached before it can be picked by a concurrent reader.
-	 */
-	lock_sock(sock->sk);
-	released = ovpn_socket_put(peer, sock);
-	release_sock(sock->sk);
-
 	/* align all readers with sk_user_data being NULL */
-	synchronize_rcu();
+	synchronize_net();
 
 	/* following cleanup should happen with lock released */
-	if (released) {
-		if (sock->sk->sk_protocol == IPPROTO_UDP) {
-			netdev_put(sock->ovpn->dev, &sock->dev_tracker);
-		} else if (sock->sk->sk_protocol == IPPROTO_TCP) {
-			/* wait for TCP jobs to terminate */
-			ovpn_tcp_socket_wait_finish(sock);
-			ovpn_peer_put(sock->peer);
-		}
-		/* drop reference acquired in ovpn_socket_new() */
-		sock_put(sock->sk);
-		/* we can call plain kfree() because we already waited one RCU
-		 * period due to synchronize_rcu()
-		 */
-		kfree(sock);
-	}
+	ovpn_socket_release_finish(sock);
 }
 
 static bool ovpn_socket_hold(struct ovpn_socket *sock)
diff --git a/drivers/net/ovpn/socket.h b/drivers/net/ovpn/socket.h
index 4afcec71040d..b4c7aa9b6179 100644
--- a/drivers/net/ovpn/socket.h
+++ b/drivers/net/ovpn/socket.h
@@ -24,6 +24,7 @@ struct ovpn_peer;
  * @peer: unique peer transmitting over this socket (TCP only)
  * @sk: the low level sock object
  * @refcount: amount of contexts currently referencing this object
+ * @release_entry: entry for the deferred release list
  * @work: member used to schedule release routine (it may block)
  * @tcp_tx_work: work for deferring outgoing packet processing (TCP only)
  */
@@ -38,6 +39,7 @@ struct ovpn_socket {
 
 	struct sock *sk;
 	struct kref refcount;
+	struct list_head release_entry;
 	struct work_struct work;
 	struct work_struct tcp_tx_work;
 };
@@ -45,5 +47,7 @@ struct ovpn_socket {
 struct ovpn_socket *ovpn_socket_new(struct socket *sock,
 				    struct ovpn_peer *peer);
 void ovpn_socket_release(struct ovpn_peer *peer);
+struct ovpn_socket *ovpn_socket_release_prepare(struct ovpn_peer *peer);
+void ovpn_socket_release_finish(struct ovpn_socket *sock);
 
 #endif /* _NET_OVPN_SOCK_H_ */
-- 
2.43.0



_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel