patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH 1/4] net/virtio: fix interrupt unregistering for listening socket
       [not found] <20210317202530.4145673-1-i.maximets@ovn.org>
@ 2021-03-17 20:25 ` Ilya Maximets
  2021-03-25  8:32   ` Maxime Coquelin
  0 siblings, 1 reply; 3+ messages in thread
From: Ilya Maximets @ 2021-03-17 20:25 UTC (permalink / raw)
  To: Maxime Coquelin
  Cc: Chenbo Xia, dev, Adrian Moreno, Stefan Hajnoczi, Julia Suvorova,
	Ilya Maximets, stable, Zhiyong Yang

virtio_user_dev_server_reconnect() is typically called from the
interrupt context while checking the link state:

  vhost_user_update_link_state()
  --> virtio_user_dev_server_reconnect()

Under this conditions callback unregistering always fails.  This means
that listenfd is never unregistered and continue to trigger interrupts.
For example, if second client will try to connect to the same socket,
the server will receive interrupts infinitely because it will not
accept them while listen fd is readable and generates epoll events.

Fix that by moving reconfiguration of interrupts out of the
interrupt context to alarm handler.

'virtio_user_dev_delayed_handler' renamed to
'virtio_user_dev_delayed_disconnect_handler' to better reflect its
purpose.

Additionally improved error logging around interrupt management.

Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
Cc: stable@dpdk.org

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---

CC: Zhiyong Yang <zhiyong.yang@intel.com>

 drivers/net/virtio/virtio_user/vhost_user.c   |  4 +-
 .../net/virtio/virtio_user/virtio_user_dev.c  | 70 ++++++++++++++-----
 .../net/virtio/virtio_user/virtio_user_dev.h  |  2 +-
 3 files changed, 57 insertions(+), 19 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index f8569f6e6f..25f74c625b 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -957,7 +957,9 @@ vhost_user_update_link_state(struct virtio_user_dev *dev)
 			 * of interrupt handling, callback cannot be
 			 * unregistered here, set an alarm to do it.
 			 */
-			rte_eal_alarm_set(1, virtio_user_dev_delayed_handler, (void *)dev);
+			rte_eal_alarm_set(1,
+				virtio_user_dev_delayed_disconnect_handler,
+				(void *)dev);
 		} else {
 			dev->net_status |= VIRTIO_NET_S_LINK_UP;
 		}
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 1b54d55bd8..68cece42d3 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -13,6 +13,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 
+#include <rte_alarm.h>
 #include <rte_string_fns.h>
 #include <rte_eal_memconfig.h>
 
@@ -885,7 +886,7 @@ virtio_user_dev_reset_queues_packed(struct rte_eth_dev *eth_dev)
 }
 
 void
-virtio_user_dev_delayed_handler(void *param)
+virtio_user_dev_delayed_disconnect_handler(void *param)
 {
 	struct virtio_user_dev *dev = param;
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
@@ -894,14 +895,27 @@ virtio_user_dev_delayed_handler(void *param)
 		PMD_DRV_LOG(ERR, "interrupt disable failed");
 		return;
 	}
-	rte_intr_callback_unregister(eth_dev->intr_handle,
-				     virtio_interrupt_handler, eth_dev);
+	PMD_DRV_LOG(DEBUG, "Unregistering intr fd: %d",
+		    eth_dev->intr_handle->fd);
+	if (rte_intr_callback_unregister(eth_dev->intr_handle,
+					 virtio_interrupt_handler,
+					 eth_dev) != 1)
+		PMD_DRV_LOG(ERR, "interrupt unregister failed");
+
 	if (dev->is_server) {
 		if (dev->ops->server_disconnect)
 			dev->ops->server_disconnect(dev);
+
 		eth_dev->intr_handle->fd = dev->ops->get_intr_fd(dev);
-		rte_intr_callback_register(eth_dev->intr_handle,
-					   virtio_interrupt_handler, eth_dev);
+
+		PMD_DRV_LOG(DEBUG, "Registering intr fd: %d",
+			    eth_dev->intr_handle->fd);
+
+		if (rte_intr_callback_register(eth_dev->intr_handle,
+					       virtio_interrupt_handler,
+					       eth_dev))
+			PMD_DRV_LOG(ERR, "interrupt register failed");
+
 		if (rte_intr_enable(eth_dev->intr_handle) < 0) {
 			PMD_DRV_LOG(ERR, "interrupt enable failed");
 			return;
@@ -909,6 +923,32 @@ virtio_user_dev_delayed_handler(void *param)
 	}
 }
 
+static void
+virtio_user_dev_delayed_intr_reconfig_handler(void *param)
+{
+	struct virtio_user_dev *dev = param;
+	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
+
+	PMD_DRV_LOG(DEBUG, "Unregistering intr fd: %d",
+		    eth_dev->intr_handle->fd);
+
+	if (rte_intr_callback_unregister(eth_dev->intr_handle,
+					 virtio_interrupt_handler,
+					 eth_dev) != 1)
+		PMD_DRV_LOG(ERR, "interrupt unregister failed");
+
+	eth_dev->intr_handle->fd = dev->ops->get_intr_fd(dev);
+
+	PMD_DRV_LOG(DEBUG, "Registering intr fd: %d", eth_dev->intr_handle->fd);
+
+	if (rte_intr_callback_register(eth_dev->intr_handle,
+				       virtio_interrupt_handler, eth_dev))
+		PMD_DRV_LOG(ERR, "interrupt register failed");
+
+	if (rte_intr_enable(eth_dev->intr_handle) < 0)
+		PMD_DRV_LOG(ERR, "interrupt enable failed");
+}
+
 int
 virtio_user_dev_server_reconnect(struct virtio_user_dev *dev)
 {
@@ -974,18 +1014,14 @@ virtio_user_dev_server_reconnect(struct virtio_user_dev *dev)
 			PMD_DRV_LOG(ERR, "interrupt disable failed");
 			return -1;
 		}
-		rte_intr_callback_unregister(eth_dev->intr_handle,
-					     virtio_interrupt_handler,
-					     eth_dev);
-
-		eth_dev->intr_handle->fd = dev->ops->get_intr_fd(dev);
-		rte_intr_callback_register(eth_dev->intr_handle,
-					   virtio_interrupt_handler, eth_dev);
-
-		if (rte_intr_enable(eth_dev->intr_handle) < 0) {
-			PMD_DRV_LOG(ERR, "interrupt enable failed");
-			return -1;
-		}
+		/*
+		 * This function can be called from the interrupt handler, so
+		 * we can't unregister interrupt handler here.  Setting
+		 * alarm to do that later.
+		 */
+		rte_eal_alarm_set(1,
+			virtio_user_dev_delayed_intr_reconfig_handler,
+			(void *)dev);
 	}
 	PMD_INIT_LOG(NOTICE, "server mode virtio-user reconnection succeeds!");
 	return 0;
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 8a62f7ea79..15d177ccd2 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -78,7 +78,7 @@ uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs);
 int virtio_user_dev_set_status(struct virtio_user_dev *dev, uint8_t status);
 int virtio_user_dev_update_status(struct virtio_user_dev *dev);
 int virtio_user_dev_update_link_state(struct virtio_user_dev *dev);
-void virtio_user_dev_delayed_handler(void *param);
+void virtio_user_dev_delayed_disconnect_handler(void *param);
 int virtio_user_dev_server_reconnect(struct virtio_user_dev *dev);
 extern const char * const virtio_user_backend_strings[];
 #endif
-- 
2.26.2


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [dpdk-stable] [PATCH 1/4] net/virtio: fix interrupt unregistering for listening socket
  2021-03-17 20:25 ` [dpdk-stable] [PATCH 1/4] net/virtio: fix interrupt unregistering for listening socket Ilya Maximets
@ 2021-03-25  8:32   ` Maxime Coquelin
  2021-04-07  7:21     ` Xia, Chenbo
  0 siblings, 1 reply; 3+ messages in thread
From: Maxime Coquelin @ 2021-03-25  8:32 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: Chenbo Xia, dev, Adrian Moreno, Stefan Hajnoczi, Julia Suvorova,
	stable, Zhiyong Yang



On 3/17/21 9:25 PM, Ilya Maximets wrote:
> virtio_user_dev_server_reconnect() is typically called from the
> interrupt context while checking the link state:
> 
>   vhost_user_update_link_state()
>   --> virtio_user_dev_server_reconnect()
> 
> Under this conditions callback unregistering always fails.  This means
> that listenfd is never unregistered and continue to trigger interrupts.
> For example, if second client will try to connect to the same socket,
> the server will receive interrupts infinitely because it will not
> accept them while listen fd is readable and generates epoll events.
> 
> Fix that by moving reconfiguration of interrupts out of the
> interrupt context to alarm handler.
> 
> 'virtio_user_dev_delayed_handler' renamed to
> 'virtio_user_dev_delayed_disconnect_handler' to better reflect its
> purpose.
> 
> Additionally improved error logging around interrupt management.
> 
> Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
> 
> CC: Zhiyong Yang <zhiyong.yang@intel.com>
> 
>  drivers/net/virtio/virtio_user/vhost_user.c   |  4 +-
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 70 ++++++++++++++-----
>  .../net/virtio/virtio_user/virtio_user_dev.h  |  2 +-
>  3 files changed, 57 insertions(+), 19 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [dpdk-stable] [PATCH 1/4] net/virtio: fix interrupt unregistering for listening socket
  2021-03-25  8:32   ` Maxime Coquelin
@ 2021-04-07  7:21     ` Xia, Chenbo
  0 siblings, 0 replies; 3+ messages in thread
From: Xia, Chenbo @ 2021-04-07  7:21 UTC (permalink / raw)
  To: Maxime Coquelin, Ilya Maximets
  Cc: dev, Adrian Moreno, Stefan Hajnoczi, Julia Suvorova, stable,
	Yang, Zhiyong

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, March 25, 2021 4:32 PM
> To: Ilya Maximets <i.maximets@ovn.org>
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org; Adrian Moreno
> <amorenoz@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>; Julia Suvorova
> <jusual@redhat.com>; stable@dpdk.org; Yang, Zhiyong <zhiyong.yang@intel.com>
> Subject: Re: [PATCH 1/4] net/virtio: fix interrupt unregistering for listening
> socket
> 
> 
> 
> On 3/17/21 9:25 PM, Ilya Maximets wrote:
> > virtio_user_dev_server_reconnect() is typically called from the
> > interrupt context while checking the link state:
> >
> >   vhost_user_update_link_state()
> >   --> virtio_user_dev_server_reconnect()
> >
> > Under this conditions callback unregistering always fails.  This means
> > that listenfd is never unregistered and continue to trigger interrupts.
> > For example, if second client will try to connect to the same socket,
> > the server will receive interrupts infinitely because it will not
> > accept them while listen fd is readable and generates epoll events.
> >
> > Fix that by moving reconfiguration of interrupts out of the
> > interrupt context to alarm handler.
> >
> > 'virtio_user_dev_delayed_handler' renamed to
> > 'virtio_user_dev_delayed_disconnect_handler' to better reflect its
> > purpose.
> >
> > Additionally improved error logging around interrupt management.
> >
> > Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> > ---
> >
> > CC: Zhiyong Yang <zhiyong.yang@intel.com>
> >
> >  drivers/net/virtio/virtio_user/vhost_user.c   |  4 +-
> >  .../net/virtio/virtio_user/virtio_user_dev.c  | 70 ++++++++++++++-----
> >  .../net/virtio/virtio_user/virtio_user_dev.h  |  2 +-
> >  3 files changed, 57 insertions(+), 19 deletions(-)
> >
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks,
> Maxime

This patch is applied to next-virtio/main with below fix:

In virtio_user_dev_delayed_intr_reconfig_handler, change 'dev->port_id' to 'dev->hw.port_id'

Thanks

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-04-07  7:22 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20210317202530.4145673-1-i.maximets@ovn.org>
2021-03-17 20:25 ` [dpdk-stable] [PATCH 1/4] net/virtio: fix interrupt unregistering for listening socket Ilya Maximets
2021-03-25  8:32   ` Maxime Coquelin
2021-04-07  7:21     ` Xia, Chenbo

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git