[dpdk-dev] [PATCH v4 1/2] net/mlx5: link status update separation

[dpdk-dev] [PATCH v4 1/2] net/mlx5: link status update separation

[Matan Azrad]

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.

In the previous code the alarm callback calls to the interrupt
handler for link status recheck and may cause to unnecessary
interrupt events check.

This patch separates the link status update and the interrupt event
handler to avoid the unnecessary check and arranges the interrupt
handler for more interrupt supports in the future.

Comment was added in the new function to explain the inconsistent
link status reason.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5_ethdev.c | 83 ++++++++++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 28 deletions(-)

Changes:
V2:
Replace link status update function name.
Add inconsistent link workaround comment.

V3:
Fix indentations.
Accurate inconsistent link comment.

V4:
Separate the patch.
Short the comment.

diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 57f6237..64a8db8 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1112,47 +1112,74 @@ mlx5_ibv_device_to_pci_addr(const struct ibv_device *device,
 }
 
 /**
- * Link status handler.
+ * Update the link status.
  *
  * @param priv
  *   Pointer to private structure.
- * @param dev
- *   Pointer to the rte_eth_dev structure.
  *
  * @return
- *   Nonzero if the callback process can be called immediately.
+ *   Zero if the callback process can be called immediately.
  */
 static int
-priv_dev_link_status_handler(struct priv *priv, struct rte_eth_dev *dev)
+priv_link_status_update(struct priv *priv)
+{
+	struct rte_eth_link *link = &priv->dev->data->dev_link;
+
+	mlx5_link_update(priv->dev, 0);
+	if (((link->link_speed == 0) && link->link_status) ||
+		((link->link_speed != 0) && !link->link_status)) {
+		/*
+		 * Inconsistent status. Event likely occurred before the
+		 * kernel netdevice exposes the new status.
+		 */
+		if (!priv->pending_alarm) {
+			priv->pending_alarm = 1;
+			rte_eal_alarm_set(MLX5_ALARM_TIMEOUT_US,
+					  mlx5_dev_link_status_handler,
+					  priv->dev);
+		}
+		return 1;
+	} else if (unlikely(priv->pending_alarm)) {
+		/* Link interrupt occurred while alarm is already scheduled. */
+		priv->pending_alarm = 0;
+		rte_eal_alarm_cancel(mlx5_dev_link_status_handler, priv->dev);
+	}
+	return 0;
+}
+
+/**
+ * Device status handler.
+ *
+ * @param priv
+ *   Pointer to private structure.
+ * @param events
+ *   Pointer to event flags holder.
+ *
+ * @return
+ *   Events bitmap of callback process which can be called immediately.
+ */
+static uint32_t
+priv_dev_status_handler(struct priv *priv)
 {
 	struct ibv_async_event event;
-	struct rte_eth_link *link = &dev->data->dev_link;
-	int ret = 0;
+	uint32_t ret = 0;
 
 	/* Read all message and acknowledge them. */
 	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)
+		if ((event.event_type == IBV_EVENT_PORT_ACTIVE ||
+			event.event_type == IBV_EVENT_PORT_ERR) &&
+			(priv->dev->data->dev_conf.intr_conf.lsc == 1))
+			ret |= (1 << RTE_ETH_EVENT_INTR_LSC);
+		else
 			DEBUG("event type %d on port %d not handled",
 			      event.event_type, event.element.port_num);
 		ibv_ack_async_event(&event);
 	}
-	mlx5_link_update(dev, 0);
-	if (((link->link_speed == 0) && link->link_status) ||
-	    ((link->link_speed != 0) && !link->link_status)) {
-		if (!priv->pending_alarm) {
-			/* Inconsistent status, check again later. */
-			priv->pending_alarm = 1;
-			rte_eal_alarm_set(MLX5_ALARM_TIMEOUT_US,
-					  mlx5_dev_link_status_handler,
-					  dev);
-		}
-	} else {
-		ret = 1;
-	}
+	if (ret & (1 << RTE_ETH_EVENT_INTR_LSC))
+		if (priv_link_status_update(priv))
+			ret &= ~(1 << RTE_ETH_EVENT_INTR_LSC);
 	return ret;
 }
 
@@ -1172,9 +1199,9 @@ mlx5_dev_link_status_handler(void *arg)
 	priv_lock(priv);
 	assert(priv->pending_alarm == 1);
 	priv->pending_alarm = 0;
-	ret = priv_dev_link_status_handler(priv, dev);
+	ret = priv_link_status_update(priv);
 	priv_unlock(priv);
-	if (ret)
+	if (!ret)
 		_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL,
 					      NULL);
 }
@@ -1192,12 +1219,12 @@ mlx5_dev_interrupt_handler(void *cb_arg)
 {
 	struct rte_eth_dev *dev = cb_arg;
 	struct priv *priv = dev->data->dev_private;
-	int ret;
+	uint32_t events;
 
 	priv_lock(priv);
-	ret = priv_dev_link_status_handler(priv, dev);
+	events = priv_dev_status_handler(priv);
 	priv_unlock(priv);
-	if (ret)
+	if (events & (1 << RTE_ETH_EVENT_INTR_LSC))
 		_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL,
 					      NULL);
 }
-- 
2.7.4

[dpdk-dev] [PATCH v4 2/2] net/mlx5: support device removal event

[Matan Azrad]

Extend the LSC event handling to support the device removal as well.

The mlx5 event handling has been made capable of receiving and
signaling several event types at once.

This support includes next:
1. Removal event detection according to the user configuration.
2. Calling to all registered mlx5 removal callbacks.
3. Capabilities extension to include removal interrupt handling.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5.c        |  2 +-
 drivers/net/mlx5/mlx5_ethdev.c | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 32081a4..99a2fb3 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -870,7 +870,7 @@ static struct rte_pci_driver mlx5_driver = {
 	},
 	.id_table = mlx5_pci_id_map,
 	.probe = mlx5_pci_probe,
-	.drv_flags = RTE_PCI_DRV_INTR_LSC,
+	.drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV,
 };
 
 /**
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 64a8db8..b57de4b 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1172,6 +1172,9 @@ priv_dev_status_handler(struct priv *priv)
 			event.event_type == IBV_EVENT_PORT_ERR) &&
 			(priv->dev->data->dev_conf.intr_conf.lsc == 1))
 			ret |= (1 << RTE_ETH_EVENT_INTR_LSC);
+		else if (event.event_type == IBV_EVENT_DEVICE_FATAL &&
+			priv->dev->data->dev_conf.intr_conf.rmv == 1)
+			ret |= (1 << RTE_ETH_EVENT_INTR_RMV);
 		else
 			DEBUG("event type %d on port %d not handled",
 			      event.event_type, event.element.port_num);
@@ -1227,6 +1230,9 @@ mlx5_dev_interrupt_handler(void *cb_arg)
 	if (events & (1 << RTE_ETH_EVENT_INTR_LSC))
 		_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL,
 					      NULL);
+	if (events & (1 << RTE_ETH_EVENT_INTR_RMV))
+		_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_RMV, NULL,
+					      NULL);
 }
 
 /**
@@ -1240,7 +1246,8 @@ mlx5_dev_interrupt_handler(void *cb_arg)
 void
 priv_dev_interrupt_handler_uninstall(struct priv *priv, struct rte_eth_dev *dev)
 {
-	if (!dev->data->dev_conf.intr_conf.lsc)
+	if (!dev->data->dev_conf.intr_conf.lsc &&
+		!dev->data->dev_conf.intr_conf.rmv)
 		return;
 	rte_intr_callback_unregister(&priv->intr_handle,
 				     mlx5_dev_interrupt_handler,
@@ -1265,7 +1272,8 @@ priv_dev_interrupt_handler_install(struct priv *priv, struct rte_eth_dev *dev)
 {
 	int rc, flags;
 
-	if (!dev->data->dev_conf.intr_conf.lsc)
+	if (!dev->data->dev_conf.intr_conf.lsc &&
+		!dev->data->dev_conf.intr_conf.rmv)
 		return;
 	assert(priv->ctx->async_fd > 0);
 	flags = fcntl(priv->ctx->async_fd, F_GETFL);
@@ -1273,6 +1281,7 @@ priv_dev_interrupt_handler_install(struct priv *priv, struct rte_eth_dev *dev)
 	if (rc < 0) {
 		INFO("failed to change file descriptor async event queue");
 		dev->data->dev_conf.intr_conf.lsc = 0;
+		dev->data->dev_conf.intr_conf.rmv = 0;
 	} else {
 		priv->intr_handle.fd = priv->ctx->async_fd;
 		priv->intr_handle.type = RTE_INTR_HANDLE_EXT;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v4 1/2] net/mlx5: link status update separation

[Adrien Mazarguil]

On Wed, Sep 06, 2017 at 06:03:57PM +0300, Matan Azrad wrote:
> 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.
> 
> In the previous code the alarm callback calls to the interrupt
> handler for link status recheck and may cause to unnecessary
> interrupt events check.
> 
> This patch separates the link status update and the interrupt event
> handler to avoid the unnecessary check and arranges the interrupt
> handler for more interrupt supports in the future.
> 
> Comment was added in the new function to explain the inconsistent
> link status reason.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v4 2/2] net/mlx5: support device removal event

[Adrien Mazarguil]

On Wed, Sep 06, 2017 at 06:03:58PM +0300, Matan Azrad wrote:
> Extend the LSC event handling to support the device removal as well.
> 
> The mlx5 event handling has been made capable of receiving and
> signaling several event types at once.
> 
> This support includes next:
> 1. Removal event detection according to the user configuration.
> 2. Calling to all registered mlx5 removal callbacks.
> 3. Capabilities extension to include removal interrupt handling.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v4 2/2] net/mlx5: support device removal event

[Ferruh Yigit]

On 9/6/2017 4:03 PM, Matan Azrad wrote:
> Extend the LSC event handling to support the device removal as well.
> 
> The mlx5 event handling has been made capable of receiving and
> signaling several event types at once.
> 
> This support includes next:
> 1. Removal event detection according to the user configuration.
> 2. Calling to all registered mlx5 removal callbacks.
> 3. Capabilities extension to include removal interrupt handling.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Hi Matan,

Can you please update features file [1] to document the support [2] ?

[1]
doc/guides/nics/features/mlx5.ini

[2]
"Removal event        ="

[dpdk-dev] [PATCH v5] net/mlx5: support device removal event

[Matan Azrad]

Extend the LSC event handling to support the device removal as well.

The mlx5 event handling has been made capable of receiving and
signaling several event types at once.

This support includes next:
1. Removal event detection according to the user configuration.
2. Calling to all registered mlx5 removal callbacks.
3. Capabilities extension to include removal interrupt handling.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 doc/guides/nics/features/mlx5.ini |  1 +
 drivers/net/mlx5/mlx5.c           |  2 +-
 drivers/net/mlx5/mlx5_ethdev.c    | 13 +++++++++++--
 3 files changed, 13 insertions(+), 3 deletions(-)

V5 for adding support in mlx5 feature doc.

diff --git a/doc/guides/nics/features/mlx5.ini b/doc/guides/nics/features/mlx5.ini
index 2913591..20a1461 100644
--- a/doc/guides/nics/features/mlx5.ini
+++ b/doc/guides/nics/features/mlx5.ini
@@ -7,6 +7,7 @@
 Speed capabilities   = Y
 Link status          = Y
 Link status event    = Y
+Removal event        = Y
 Rx interrupt         = Y
 Queue start/stop     = Y
 MTU update           = Y
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 32081a4..99a2fb3 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -870,7 +870,7 @@ static struct rte_pci_driver mlx5_driver = {
 	},
 	.id_table = mlx5_pci_id_map,
 	.probe = mlx5_pci_probe,
-	.drv_flags = RTE_PCI_DRV_INTR_LSC,
+	.drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV,
 };
 
 /**
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 64a8db8..b57de4b 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1172,6 +1172,9 @@ priv_dev_status_handler(struct priv *priv)
 			event.event_type == IBV_EVENT_PORT_ERR) &&
 			(priv->dev->data->dev_conf.intr_conf.lsc == 1))
 			ret |= (1 << RTE_ETH_EVENT_INTR_LSC);
+		else if (event.event_type == IBV_EVENT_DEVICE_FATAL &&
+			priv->dev->data->dev_conf.intr_conf.rmv == 1)
+			ret |= (1 << RTE_ETH_EVENT_INTR_RMV);
 		else
 			DEBUG("event type %d on port %d not handled",
 			      event.event_type, event.element.port_num);
@@ -1227,6 +1230,9 @@ mlx5_dev_interrupt_handler(void *cb_arg)
 	if (events & (1 << RTE_ETH_EVENT_INTR_LSC))
 		_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL,
 					      NULL);
+	if (events & (1 << RTE_ETH_EVENT_INTR_RMV))
+		_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_RMV, NULL,
+					      NULL);
 }
 
 /**
@@ -1240,7 +1246,8 @@ mlx5_dev_interrupt_handler(void *cb_arg)
 void
 priv_dev_interrupt_handler_uninstall(struct priv *priv, struct rte_eth_dev *dev)
 {
-	if (!dev->data->dev_conf.intr_conf.lsc)
+	if (!dev->data->dev_conf.intr_conf.lsc &&
+		!dev->data->dev_conf.intr_conf.rmv)
 		return;
 	rte_intr_callback_unregister(&priv->intr_handle,
 				     mlx5_dev_interrupt_handler,
@@ -1265,7 +1272,8 @@ priv_dev_interrupt_handler_install(struct priv *priv, struct rte_eth_dev *dev)
 {
 	int rc, flags;
 
-	if (!dev->data->dev_conf.intr_conf.lsc)
+	if (!dev->data->dev_conf.intr_conf.lsc &&
+		!dev->data->dev_conf.intr_conf.rmv)
 		return;
 	assert(priv->ctx->async_fd > 0);
 	flags = fcntl(priv->ctx->async_fd, F_GETFL);
@@ -1273,6 +1281,7 @@ priv_dev_interrupt_handler_install(struct priv *priv, struct rte_eth_dev *dev)
 	if (rc < 0) {
 		INFO("failed to change file descriptor async event queue");
 		dev->data->dev_conf.intr_conf.lsc = 0;
+		dev->data->dev_conf.intr_conf.rmv = 0;
 	} else {
 		priv->intr_handle.fd = priv->ctx->async_fd;
 		priv->intr_handle.type = RTE_INTR_HANDLE_EXT;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v4 1/2] net/mlx5: link status update separation

[Ferruh Yigit]

On 9/6/2017 4:14 PM, Adrien Mazarguil wrote:
> On Wed, Sep 06, 2017 at 06:03:57PM +0300, Matan Azrad wrote:
>> 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.
>>
>> In the previous code the alarm callback calls to the interrupt
>> handler for link status recheck and may cause to unnecessary
>> interrupt events check.
>>
>> This patch separates the link status update and the interrupt event
>> handler to avoid the unnecessary check and arranges the interrupt
>> handler for more interrupt supports in the future.
>>
>> Comment was added in the new function to explain the inconsistent
>> link status reason.
>>
>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> 
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v5] net/mlx5: support device removal event

[Ferruh Yigit]

On 9/8/2017 11:47 AM, Matan Azrad wrote:
> Extend the LSC event handling to support the device removal as well.
> 
> The mlx5 event handling has been made capable of receiving and
> signaling several event types at once.
> 
> This support includes next:
> 1. Removal event detection according to the user configuration.
> 2. Calling to all registered mlx5 removal callbacks.
> 3. Capabilities extension to include removal interrupt handling.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Applied to dpdk-next-net/master, thanks.

(kept ack from previous version of the patch)