Skip to contentSkip to conversation

net/mlx4: additional interrupt handling fixes

PATCH In review Adrien Mazarguil 3 participants 7 messages Sep 5, 2017 – Sep 8, 2017 Network drivers

lore
Share & tools
lore

Adrien Mazarguil

author
#0 lore raw

cover net/mlx4: additional interrupt handling fixes

While the previous interrupt rework improved the situation, it failed to address one last issue pointed out by Matan: RMV/LSC events may be missed and not reported to the application.

Adrien Mazarguil (3):
net/mlx4: fix unhandled event debug message
net/mlx4: fix rescheduled link status check
net/mlx4: merge interrupt collector function

 drivers/net/mlx4/mlx4_intr.c | 150 ++++++++++++++++++--------------------
 1 file changed, 71 insertions(+), 79 deletions(-)
Signature · 1 line
2.1.4
author
#1 lore raw

PATCH 1/3 net/mlx4: fix unhandled event debug message

When LSC or RMV events are received by the PMD but are not requested by the application, a misleading debugging message implying the PMD does not support them is shown.

Fixes 6dd7b7056d7f ("net/mlx4: support device removal event")
Cc Gaetan Rivet <[email protected]>
Cc [email protected]
Signed-off-by Adrien Mazarguil <[email protected]>
---
 drivers/net/mlx4/mlx4_intr.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/net/mlx4/mlx4_intr.c +11 -6
index e3449ee..d7f1098 100644
@@ -160,16 +160,21 @@ mlx4_collect_interrupt_events(struct priv *priv, uint32_t *events)
 	for (;;) {
 		if (ibv_get_async_event(priv->ctx, &event))
 			break;
-		if ((event.event_type == IBV_EVENT_PORT_ACTIVE ||
-		     event.event_type == IBV_EVENT_PORT_ERR) &&
-		    intr_conf->lsc) {
+		switch (event.event_type) {
+		case IBV_EVENT_PORT_ACTIVE:
+		case IBV_EVENT_PORT_ERR:
+			if (!intr_conf->lsc)
+				break;
 			port_change = 1;
 			ret++;
-		} else if (event.event_type == IBV_EVENT_DEVICE_FATAL &&
-			   intr_conf->rmv) {
+			break;
+		case IBV_EVENT_DEVICE_FATAL:
+			if (!intr_conf->rmv)
+				break;
 			*events |= (1 << RTE_ETH_EVENT_INTR_RMV);
 			ret++;
-		} else {
+			break;
+		default:
 			DEBUG("event type %d on port %d not handled",
 			      event.event_type, event.element.port_num);
 		}
Signature · 1 line
2.1.4
author
#2 lore raw

PATCH 2/3 net/mlx4: fix rescheduled link status check

In reply to Adrien Mazarguil

Link status is sometimes inconsistent during a LSC event. When it occurs, the PMD refrains from immediately notifying the application; instead, an alarm is scheduled to check link status later and notify the application once it has settled.

The problem is that subsequent link status checks are only performed if additional LSC events occur in the meantime, which is not always the case.

Worse, since support for removal events was added, rescheduled link status checks may consume them as well without notifying the application. With the right timing, a link loss occurring just before a device removal event may hide it from the application.

Fixes 6dd7b7056d7f ("net/mlx4: support device removal event")
Fixes 2d449f7c52de ("net/mlx4: fix assertion failure on link update")
Cc [email protected]
Cc Gaetan Rivet <[email protected]>
Reported-by Matan Azrad <[email protected]>
Signed-off-by Adrien Mazarguil <[email protected]>
---
 drivers/net/mlx4/mlx4_intr.c | 71 +++++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 25 deletions(-)
drivers/net/mlx4/mlx4_intr.c +46 -25
index d7f1098..e1e6c05 100644
@@ -59,7 +59,7 @@
 #include "mlx4_rxtx.h"
 #include "mlx4_utils.h"
 
-static void mlx4_link_status_alarm(struct priv *priv);
+static int mlx4_link_status_check(struct priv *priv);
 
 /**
  * Clean up Rx interrupts handler.

@@ -149,8 +149,6 @@ static int
 mlx4_collect_interrupt_events(struct priv *priv, uint32_t *events)
 {
 	struct ibv_async_event event;
-	int port_change = 0;
-	struct rte_eth_link *link = &priv->dev->data->dev_link;
 	const struct rte_intr_conf *const intr_conf =
 		&priv->dev->data->dev_conf.intr_conf;
 	int ret = 0;

@@ -163,9 +161,9 @@ mlx4_collect_interrupt_events(struct priv *priv, uint32_t *events)
 		switch (event.event_type) {
 		case IBV_EVENT_PORT_ACTIVE:
 		case IBV_EVENT_PORT_ERR:
-			if (!intr_conf->lsc)
+			if (!intr_conf->lsc || mlx4_link_status_check(priv))
 				break;
-			port_change = 1;
+			*events |= (1 << RTE_ETH_EVENT_INTR_LSC);
 			ret++;
 			break;
 		case IBV_EVENT_DEVICE_FATAL:

@@ -180,47 +178,70 @@ mlx4_collect_interrupt_events(struct priv *priv, uint32_t *events)
 		}
 		ibv_ack_async_event(&event);
 	}
-	if (!port_change)
-		return ret;
-	mlx4_link_update(priv->dev, 0);
-	if (((link->link_speed == 0) && link->link_status) ||
-	    ((link->link_speed != 0) && !link->link_status)) {
-		if (!priv->intr_alarm) {
-			/* Inconsistent status, check again later. */
-			priv->intr_alarm = 1;
-			rte_eal_alarm_set(MLX4_INTR_ALARM_TIMEOUT,
-					  (void (*)(void *))
-					  mlx4_link_status_alarm,
-					  priv);
-		}
-	} else {
-		*events |= (1 << RTE_ETH_EVENT_INTR_LSC);
-	}
 	return ret;
 }
 
 /**
  * Process scheduled link status check.
  *
+ * If LSC interrupts are requested, process related callback.
+ *
  * @param priv
  *   Pointer to private structure.
  */
 static void
 mlx4_link_status_alarm(struct priv *priv)
 {
-	uint32_t events;
-	int ret;
+	const struct rte_intr_conf *const intr_conf =
+		&priv->dev->data->dev_conf.intr_conf;
 
 	assert(priv->intr_alarm == 1);
 	priv->intr_alarm = 0;
-	ret = mlx4_collect_interrupt_events(priv, &events);
-	if (ret > 0 && events & (1 << RTE_ETH_EVENT_INTR_LSC))
+	if (intr_conf->lsc && !mlx4_link_status_check(priv))
 		_rte_eth_dev_callback_process(priv->dev,
 					      RTE_ETH_EVENT_INTR_LSC,
 					      NULL, NULL);
 }
 
 /**
+ * Check link status.
+ *
+ * In case of inconsistency, another check is scheduled.
+ *
+ * @param priv
+ *   Pointer to private structure.
+ *
+ * @return
+ *   0 on success (link status is consistent), negative errno value
+ *   otherwise and rte_errno is set.
+ */
+static int
+mlx4_link_status_check(struct priv *priv)
+{
+	struct rte_eth_link *link = &priv->dev->data->dev_link;
+	int ret = mlx4_link_update(priv->dev, 0);
+
+	if (ret)
+		return ret;
+	if ((!link->link_speed && link->link_status) ||
+	    (link->link_speed && !link->link_status)) {
+		if (!priv->intr_alarm) {
+			/* Inconsistent status, check again later. */
+			ret = rte_eal_alarm_set(MLX4_INTR_ALARM_TIMEOUT,
+						(void (*)(void *))
+						mlx4_link_status_alarm,
+						priv);
+			if (ret)
+				return ret;
+			priv->intr_alarm = 1;
+		}
+		rte_errno = EINPROGRESS;
+		return -rte_errno;
+	}
+	return 0;
+}
+
+/**
  * Handle interrupts from the NIC.
  *
  * @param priv
Signature · 1 line
2.1.4
author
#3 lore raw

PATCH 3/3 net/mlx4: merge interrupt collector function

In reply to Adrien Mazarguil

Since interrupt handler is the only function relying on it, merging them simplifies the code as there is no need for an API to return collected events.

Signed-off-by Adrien Mazarguil <[email protected]>
---
 drivers/net/mlx4/mlx4_intr.c | 94 +++++++++++++--------------------------
 1 file changed, 30 insertions(+), 64 deletions(-)
drivers/net/mlx4/mlx4_intr.c +30 -64
index e1e6c05..3806322 100644
@@ -135,53 +135,6 @@ mlx4_rx_intr_vec_enable(struct priv *priv)
 }
 
 /**
- * Collect interrupt events.
- *
- * @param priv
- *   Pointer to private structure.
- * @param events
- *   Pointer to event flags holder.
- *
- * @return
- *   Number of events.
- */
-static int
-mlx4_collect_interrupt_events(struct priv *priv, uint32_t *events)
-{
-	struct ibv_async_event event;
-	const struct rte_intr_conf *const intr_conf =
-		&priv->dev->data->dev_conf.intr_conf;
-	int ret = 0;
-
-	*events = 0;
-	/* Read all message and acknowledge them. */
-	for (;;) {
-		if (ibv_get_async_event(priv->ctx, &event))
-			break;
-		switch (event.event_type) {
-		case IBV_EVENT_PORT_ACTIVE:
-		case IBV_EVENT_PORT_ERR:
-			if (!intr_conf->lsc || mlx4_link_status_check(priv))
-				break;
-			*events |= (1 << RTE_ETH_EVENT_INTR_LSC);
-			ret++;
-			break;
-		case IBV_EVENT_DEVICE_FATAL:
-			if (!intr_conf->rmv)
-				break;
-			*events |= (1 << RTE_ETH_EVENT_INTR_RMV);
-			ret++;
-			break;
-		default:
-			DEBUG("event type %d on port %d not handled",
-			      event.event_type, event.element.port_num);
-		}
-		ibv_ack_async_event(&event);
-	}
-	return ret;
-}
-
-/**
  * Process scheduled link status check.
  *
  * If LSC interrupts are requested, process related callback.

@@ -250,26 +203,39 @@ mlx4_link_status_check(struct priv *priv)
 static void
 mlx4_interrupt_handler(struct priv *priv)
 {
-	int ret;
-	uint32_t ev;
-	int i;
+	enum { LSC, RMV, };
+	static const enum rte_eth_event_type type[] = {
+		[LSC] = RTE_ETH_EVENT_INTR_LSC,
+		[RMV] = RTE_ETH_EVENT_INTR_RMV,
+	};
+	uint32_t caught[RTE_DIM(type)] = { 0 };
+	struct ibv_async_event event;
+	const struct rte_intr_conf *const intr_conf =
+		&priv->dev->data->dev_conf.intr_conf;
+	unsigned int i;
 
-	ret = mlx4_collect_interrupt_events(priv, &ev);
-	if (ret > 0) {
-		for (i = RTE_ETH_EVENT_UNKNOWN;
-		     i < RTE_ETH_EVENT_MAX;
-		     i++) {
-			if (ev & (1 << i)) {
-				ev &= ~(1 << i);
-				_rte_eth_dev_callback_process(priv->dev, i,
-							      NULL, NULL);
-				ret--;
-			}
+	/* Read all message and acknowledge them. */
+	while (!ibv_get_async_event(priv->ctx, &event)) {
+		switch (event.event_type) {
+		case IBV_EVENT_PORT_ACTIVE:
+		case IBV_EVENT_PORT_ERR:
+			if (intr_conf->lsc && !mlx4_link_status_check(priv))
+				++caught[LSC];
+			break;
+		case IBV_EVENT_DEVICE_FATAL:
+			if (intr_conf->rmv)
+				++caught[RMV];
+			break;
+		default:
+			DEBUG("event type %d on physical port %d not handled",
+			      event.event_type, event.element.port_num);
 		}
-		if (ret)
-			WARN("%d event%s not processed", ret,
-			     (ret > 1 ? "s were" : " was"));
+		ibv_ack_async_event(&event);
 	}
+	for (i = 0; i != RTE_DIM(caught); ++i)
+		if (caught[i])
+			_rte_eth_dev_callback_process(priv->dev, type[i],
+						      NULL, NULL);
 }
 
 /**
Signature · 1 line
2.1.4
#4 lore raw

Re: [PATCH v1 3/3] net/mlx4: merge interrupt collector function

Quoted 70 lines · Adrien Mazarguil

Since interrupt handler is the only function relying on it, merging them simplifies the code as there is no need for an API to return collected events.

Signed-off-by Adrien Mazarguil <[email protected]>
---
 drivers/net/mlx4/mlx4_intr.c | 94 +++++++++++++--------------------------
 1 file changed, 30 insertions(+), 64 deletions(-)
drivers/net/mlx4/mlx4_intr.c +0 -47
index e1e6c05..3806322 100644
@@ -135,53 +135,6 @@ mlx4_rx_intr_vec_enable(struct priv *priv)
 }
 
 /**
- * Collect interrupt events.
- *
- * @param priv
- *   Pointer to private structure.
- * @param events
- *   Pointer to event flags holder.
- *
- * @return
- *   Number of events.
- */
-static int
-mlx4_collect_interrupt_events(struct priv *priv, uint32_t *events)
-{
-	struct ibv_async_event event;
-	const struct rte_intr_conf *const intr_conf =
-		&priv->dev->data->dev_conf.intr_conf;
-	int ret = 0;
-
-	*events = 0;
-	/* Read all message and acknowledge them. */
-	for (;;) {
-		if (ibv_get_async_event(priv->ctx, &event))
-			break;
-		switch (event.event_type) {
-		case IBV_EVENT_PORT_ACTIVE:
-		case IBV_EVENT_PORT_ERR:
-			if (!intr_conf->lsc || mlx4_link_status_check(priv))
-				break;
-			*events |= (1 << RTE_ETH_EVENT_INTR_LSC);
-			ret++;
-			break;
-		case IBV_EVENT_DEVICE_FATAL:
-			if (!intr_conf->rmv)
-				break;
-			*events |= (1 << RTE_ETH_EVENT_INTR_RMV);
-			ret++;
-			break;
-		default:
-			DEBUG("event type %d on port %d not handled",
-			      event.event_type, event.element.port_num);
-		}
-		ibv_ack_async_event(&event);
-	}
-	return ret;
-}
-
-/**
  * Process scheduled link status check.
  *
  * If LSC interrupts are requested, process related callback.

@@ -250,26 +203,39 @@ mlx4_link_status_check(struct priv *priv)
 static void
 mlx4_interrupt_handler(struct priv *priv)

drivers/net/mlx4/mlx4_intr.c mlx4_link_status_check(struct priv *priv) · line 211

Quoted 10 lines
drivers/net/mlx4/mlx4_intr.c +6 -3
 {
-	int ret;
-	uint32_t ev;
-	int i;
+	enum { LSC, RMV, };
+	static const enum rte_eth_event_type type[] = {
+		[LSC] = RTE_ETH_EVENT_INTR_LSC,
+		[RMV] = RTE_ETH_EVENT_INTR_RMV,
+	};
+	uint32_t caught[RTE_DIM(type)] = { 0 };

This is nicely written

Quoted 47 lines
drivers/net/mlx4/mlx4_intr.c +24 -14
+	struct ibv_async_event event;
+	const struct rte_intr_conf *const intr_conf =
+		&priv->dev->data->dev_conf.intr_conf;
+	unsigned int i;
 
-	ret = mlx4_collect_interrupt_events(priv, &ev);
-	if (ret > 0) {
-		for (i = RTE_ETH_EVENT_UNKNOWN;
-		     i < RTE_ETH_EVENT_MAX;
-		     i++) {
-			if (ev & (1 << i)) {
-				ev &= ~(1 << i);
-				_rte_eth_dev_callback_process(priv->dev, i,
-							      NULL, NULL);
-				ret--;
-			}
+	/* Read all message and acknowledge them. */
+	while (!ibv_get_async_event(priv->ctx, &event)) {
+		switch (event.event_type) {
+		case IBV_EVENT_PORT_ACTIVE:
+		case IBV_EVENT_PORT_ERR:
+			if (intr_conf->lsc && !mlx4_link_status_check(priv))
+				++caught[LSC];
+			break;
+		case IBV_EVENT_DEVICE_FATAL:
+			if (intr_conf->rmv)
+				++caught[RMV];
+			break;
+		default:
+			DEBUG("event type %d on physical port %d not handled",
+			      event.event_type, event.element.port_num);
 		}
-		if (ret)
-			WARN("%d event%s not processed", ret,
-			     (ret > 1 ? "s were" : " was"));
+		ibv_ack_async_event(&event);
 	}
+	for (i = 0; i != RTE_DIM(caught); ++i)
+		if (caught[i])
+			_rte_eth_dev_callback_process(priv->dev, type[i],
+						      NULL, NULL);
 }
 
 /**
Signature · 1 line
2.1.4
Signature · 2 lines
Gaëtan Rivet
6WIND
Acked-by on the v1 series
#5 lore raw
In reply to Adrien Mazarguil
Quoted 4 lines · Adrien Mazarguil

While the previous interrupt rework improved the situation, it failed to address one last issue pointed out by Matan: RMV/LSC events may be missed and not reported to the application.

Quoted 11 lines

Adrien Mazarguil (3):
net/mlx4: fix unhandled event debug message
net/mlx4: fix rescheduled link status check
net/mlx4: merge interrupt collector function

 drivers/net/mlx4/mlx4_intr.c | 150 ++++++++++++++++++--------------------
 1 file changed, 71 insertions(+), 79 deletions(-)
Signature · 1 line
2.1.4

For the series:

Acked-by Gaetan Rivet <[email protected]>
Signature · 2 lines
Gaëtan Rivet
6WIND
#6 lore raw
In reply to Adrien Mazarguil
Quoted 8 lines · Adrien Mazarguil

While the previous interrupt rework improved the situation, it failed to address one last issue pointed out by Matan: RMV/LSC events may be missed and not reported to the application.

Adrien Mazarguil (3):
net/mlx4: fix unhandled event debug message
net/mlx4: fix rescheduled link status check
net/mlx4: merge interrupt collector function

Series applied to dpdk-next-net/master, thanks.