[PATCH v2] net/memif: redesign control channel teardown

[Sriram Yagnaraman]

The control channel can be torn down from two threads: the EAL interrupt
thread (memif_intr_handler on peer disconnect/error) and an application
thread (dev_stop/dev_close -> memif_disconnect). Both race to unregister
the callback and free the channel, which memif_pmd_autotest SIGSEGVs on
during EAL teardown (seen on amd64 and arm64).

Rework teardown around a single owner:

  - memif_disconnect() clears pmd->cc under cc_lock; whoever clears it
    owns the free.
  - Device teardown (link down, flags, queue eventfds, regions) moves to
    memif_disconnect_queues(), run by whichever thread owns the
    disconnect, so flag/link state is left consistent for reconnect.
  - memif_cc_release() frees the channel: unregister_pending() on the
    interrupt thread (deferred free, drops the fd from epoll after
    dispatch), unregister_sync() on an application thread.
  - memif_intr_handler() self-unregisters on disconnect so the EOF
    (level-triggered) socket fd cannot re-fire (no livelock).

Reported-by: Stephen Hemminger <stephen at networkplumber.org>
Suggested-by: Stephen Hemminger <stephen at networkplumber.org>
Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman at ericsson.com>
---
v2:
  - Redesign rather than extend v1. v1's approach (detach + handler
    ownership re-check that returns without unregistering) could
    livelock: the EOF control socket is level-triggered, so the handler
    re-fires continuously and starves the application thread waiting in
    unregister_sync(). v2 makes the handler always self-unregister via
    unregister_pending(), removing the fd from epoll so it cannot
    re-fire.
  - Split device-level teardown (link/flags/queues/regions) into
    memif_disconnect_queues() so the handler-owned disconnect leaves the
    same consistent state as memif_disconnect(); fixes a reconnect
    regression seen while developing the single-owner model.
  - Addresses the "teardown too complex" feedback by giving the channel
    a single owner (whoever clears pmd->cc) instead of splitting the
    free across two threads.
---
 drivers/net/memif/memif_socket.c | 179 ++++++++++++++++++++++---------
 1 file changed, 126 insertions(+), 53 deletions(-)

diff --git a/drivers/net/memif/memif_socket.c b/drivers/net/memif/memif_socket.c
index 971964133c..77d3044681 100644
--- a/drivers/net/memif/memif_socket.c
+++ b/drivers/net/memif/memif_socket.c
@@ -590,67 +590,23 @@ memif_intr_unregister_handler(const struct rte_intr_handle *intr_handle, void *a
 	rte_free(cc);
 }
 
-void
-memif_disconnect(struct rte_eth_dev *dev)
+/*
+ * Device-level teardown: mark the link down, close the per-queue interrupt
+ * eventfds and release the shared-memory regions. Idempotent and independent
+ * of the control channel, so it is safe to run from whichever thread owns the
+ * disconnect.
+ */
+static void
+memif_disconnect_queues(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
-	struct memif_msg_queue_elt *elt, *next;
 	struct memif_queue *mq;
-	struct rte_intr_handle *ih;
 	int i;
-	int ret;
 
 	dev->data->dev_link.link_status = RTE_ETH_LINK_DOWN;
 	pmd->flags &= ~ETH_MEMIF_FLAG_CONNECTING;
 	pmd->flags &= ~ETH_MEMIF_FLAG_CONNECTED;
 
-	rte_spinlock_lock(&pmd->cc_lock);
-	if (pmd->cc != NULL) {
-		/* Clear control message queue (except disconnect message if any). */
-		for (elt = TAILQ_FIRST(&pmd->cc->msg_queue); elt != NULL; elt = next) {
-			next = TAILQ_NEXT(elt, next);
-			if (elt->msg.type != MEMIF_MSG_TYPE_DISCONNECT) {
-				TAILQ_REMOVE(&pmd->cc->msg_queue, elt, next);
-				rte_free(elt);
-			}
-		}
-		/* send disconnect message (if there is any in queue) */
-		memif_msg_send_from_queue(pmd->cc);
-
-		/* at this point, there should be no more messages in queue */
-		if (TAILQ_FIRST(&pmd->cc->msg_queue) != NULL) {
-			MIF_LOG(WARNING,
-				"Unexpected message(s) in message queue.");
-		}
-
-		ih = pmd->cc->intr_handle;
-		if (rte_intr_fd_get(ih) >= 0) {
-			ret = rte_intr_callback_unregister(ih,
-							memif_intr_handler,
-							pmd->cc);
-			/*
-			 * If callback is active (disconnecting based on
-			 * received control message).
-			 */
-			if (ret == -EAGAIN) {
-				ret = rte_intr_callback_unregister_pending(ih,
-							memif_intr_handler,
-							pmd->cc,
-							memif_intr_unregister_handler);
-			} else if (ret > 0) {
-				rte_intr_fd_close(ih);
-				rte_intr_instance_free(ih);
-				rte_free(pmd->cc);
-			}
-			pmd->cc = NULL;
-			if (ret <= 0)
-				MIF_LOG(WARNING,
-					"Failed to unregister control channel callback.");
-		}
-	}
-	rte_spinlock_unlock(&pmd->cc_lock);
-
-	/* unconfig interrupts */
 	for (i = 0; i < pmd->cfg.num_c2s_rings; i++) {
 		if (pmd->role == MEMIF_ROLE_CLIENT) {
 			if (dev->data->tx_queues != NULL)
@@ -697,6 +653,88 @@ memif_disconnect(struct rte_eth_dev *dev)
 		(pmd->role == MEMIF_ROLE_SERVER) ? "server" : "client");
 }
 
+/*
+ * Release a control channel and free it. Exactly one thread must do this
+ * per channel: the caller that cleared pmd->cc under cc_lock owns the free.
+ *
+ * On the interrupt thread the running callback cannot be waited for (it may
+ * be us), so the free is deferred to the pending-unregister callback, which
+ * EAL runs - and which removes the fd from the epoll set - once the current
+ * dispatch finishes. The handler therefore never returns leaving its own fd
+ * registered and hot, so it cannot re-fire on a half-torn-down channel.
+ *
+ * On an application thread the handler (if any) is waited out and the channel
+ * freed directly.
+ */
+static void
+memif_cc_release(struct memif_control_channel *cc)
+{
+	struct rte_intr_handle *ih = cc->intr_handle;
+	int ret;
+
+	if (rte_intr_fd_get(ih) < 0) {
+		/* Never registered; free directly. */
+		memif_intr_unregister_handler(ih, cc);
+		return;
+	}
+
+	if (rte_thread_is_intr()) {
+		ret = rte_intr_callback_unregister_pending(ih,
+				memif_intr_handler, cc,
+				memif_intr_unregister_handler);
+		if (ret < 0)
+			MIF_LOG(WARNING,
+				"Failed to defer control channel callback unregister.");
+	} else {
+		ret = rte_intr_callback_unregister_sync(ih,
+				memif_intr_handler, cc);
+		if (ret > 0 || ret == -ENOENT)
+			memif_intr_unregister_handler(ih, cc);
+		else
+			MIF_LOG(WARNING,
+				"Failed to unregister control channel callback.");
+	}
+}
+
+void
+memif_disconnect(struct rte_eth_dev *dev)
+{
+	struct pmd_internals *pmd = dev->data->dev_private;
+	struct memif_msg_queue_elt *elt, *next;
+	struct memif_control_channel *cc;
+
+	/*
+	 * Detach the control channel: whoever clears pmd->cc under the lock
+	 * becomes the single owner of the free. If it is already NULL another
+	 * thread (the interrupt handler, or a concurrent disconnect) has taken
+	 * ownership and will release it; we only do the device-level teardown.
+	 */
+	rte_spinlock_lock(&pmd->cc_lock);
+	cc = pmd->cc;
+	pmd->cc = NULL;
+	if (cc != NULL) {
+		/* Clear control message queue (except disconnect message if any). */
+		for (elt = TAILQ_FIRST(&cc->msg_queue); elt != NULL; elt = next) {
+			next = TAILQ_NEXT(elt, next);
+			if (elt->msg.type != MEMIF_MSG_TYPE_DISCONNECT) {
+				TAILQ_REMOVE(&cc->msg_queue, elt, next);
+				rte_free(elt);
+			}
+		}
+		/* send disconnect message (if there is any in queue) */
+		memif_msg_send_from_queue(cc);
+		/* at this point, there should be no more messages in queue */
+		if (TAILQ_FIRST(&cc->msg_queue) != NULL)
+			MIF_LOG(WARNING, "Unexpected message(s) in message queue.");
+	}
+	rte_spinlock_unlock(&pmd->cc_lock);
+
+	if (cc != NULL)
+		memif_cc_release(cc);
+
+	memif_disconnect_queues(dev);
+}
+
 static int
 memif_msg_receive(struct memif_control_channel *cc)
 {
@@ -868,6 +906,8 @@ static void
 memif_intr_handler(void *arg)
 {
 	struct memif_control_channel *cc = arg;
+	struct pmd_internals *pmd;
+	bool owned;
 	int ret;
 
 	ret = memif_msg_receive(cc);
@@ -898,7 +938,40 @@ memif_intr_handler(void *arg)
 		MIF_LOG(WARNING, "eth dev not allocated");
 		return;
 	}
-	memif_disconnect(cc->dev);
+
+	/*
+	 * Self-unregister first. rte_intr_callback_unregister_pending() marks
+	 * this callback for deletion once the current dispatch returns, which
+	 * removes the fd from the epoll set. This is essential: the control
+	 * socket is at EOF and level-triggered, so without removing it the
+	 * handler would be re-dispatched forever (livelock) and starve any
+	 * application thread waiting in rte_intr_callback_unregister_sync().
+	 *
+	 * Ownership of the free is decided under cc_lock: whoever clears
+	 * pmd->cc frees the channel.
+	 *   - If this handler wins, it passes memif_intr_unregister_handler as
+	 *     the pending-unregister callback so EAL frees the channel after
+	 *     the dispatch, and it runs the device teardown.
+	 *   - If an application thread already cleared pmd->cc, it owns the
+	 *     free (it waits for us via unregister_sync, then frees); this
+	 *     handler passes a NULL unregister callback and does nothing else.
+	 */
+	pmd = cc->dev->data->dev_private;
+	rte_spinlock_lock(&pmd->cc_lock);
+	owned = (pmd->cc == cc);
+	if (owned)
+		pmd->cc = NULL;
+	rte_spinlock_unlock(&pmd->cc_lock);
+
+	ret = rte_intr_callback_unregister_pending(cc->intr_handle,
+			memif_intr_handler, cc,
+			owned ? memif_intr_unregister_handler : NULL);
+	if (ret < 0)
+		MIF_LOG(WARNING,
+			"Failed to defer control channel callback unregister.");
+
+	if (owned)
+		memif_disconnect_queues(cc->dev);
 }
 
 static void
-- 
2.43.7