DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices
@ 2019-12-22 17:55 Stephen Hemminger
  2020-01-17 16:43 ` Ferruh Yigit
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2019-12-22 17:55 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: dev, Stephen Hemminger, stable

This fixes a deadlock when using KNI with bifurcated drivers.
Bringing kni device up always times out when using Mellanox
devices.

The kernel KNI driver sends message to userspace to complete
the request. For the case of bifurcated driver, this may involve
an additional request to kernel to change state. This request
would deadlock because KNI was holding the RTNL mutex.

This was a bad design which goes back to the original code.
A workaround is for KNI driver to drop RTNL while waiting.
To prevent the device from disappearing while the operation
is in progress, it needs to hold reference to network device
while waiting.

As an added benefit, an useless error check can also be removed.

Fixes: 3fc5ca2f6352 ("kni: initial import")
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 kernel/linux/kni/kni_net.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
index 1ba9b1b99f66..b7337c1410b8 100644
--- a/kernel/linux/kni/kni_net.c
+++ b/kernel/linux/kni/kni_net.c
@@ -17,6 +17,7 @@
 #include <linux/skbuff.h>
 #include <linux/kthread.h>
 #include <linux/delay.h>
+#include <linux/rtnetlink.h>
 
 #include <rte_kni_common.h>
 #include <kni_fifo.h>
@@ -102,17 +103,15 @@ get_data_kva(struct kni_dev *kni, void *pkt_kva)
  * It can be called to process the request.
  */
 static int
-kni_net_process_request(struct kni_dev *kni, struct rte_kni_request *req)
+kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
 {
+	struct kni_dev *kni = netdev_priv(dev);
 	int ret = -1;
 	void *resp_va;
 	uint32_t num;
 	int ret_val;
 
-	if (!kni || !req) {
-		pr_err("No kni instance or request\n");
-		return -EINVAL;
-	}
+	ASSERT_RTNL();
 
 	mutex_lock(&kni->sync_lock);
 
@@ -125,8 +124,17 @@ kni_net_process_request(struct kni_dev *kni, struct rte_kni_request *req)
 		goto fail;
 	}
 
+	/* Since we need to wait and RTNL mutex is held
+	 * drop the mutex and hold refernce to keep device
+	 */
+	dev_hold(dev);
+	rtnl_unlock();
+
 	ret_val = wait_event_interruptible_timeout(kni->wq,
 			kni_fifo_count(kni->resp_q), 3 * HZ);
+	rtnl_lock();
+	dev_put(dev);
+
 	if (signal_pending(current) || ret_val <= 0) {
 		ret = -ETIME;
 		goto fail;
@@ -155,7 +163,6 @@ kni_net_open(struct net_device *dev)
 {
 	int ret;
 	struct rte_kni_request req;
-	struct kni_dev *kni = netdev_priv(dev);
 
 	netif_start_queue(dev);
 	if (dflt_carrier == 1)
@@ -168,7 +175,7 @@ kni_net_open(struct net_device *dev)
 
 	/* Setting if_up to non-zero means up */
 	req.if_up = 1;
-	ret = kni_net_process_request(kni, &req);
+	ret = kni_net_process_request(dev, &req);
 
 	return (ret == 0) ? req.result : ret;
 }
@@ -178,7 +185,6 @@ kni_net_release(struct net_device *dev)
 {
 	int ret;
 	struct rte_kni_request req;
-	struct kni_dev *kni = netdev_priv(dev);
 
 	netif_stop_queue(dev); /* can't transmit any more */
 	netif_carrier_off(dev);
@@ -188,7 +194,7 @@ kni_net_release(struct net_device *dev)
 
 	/* Setting if_up to 0 means down */
 	req.if_up = 0;
-	ret = kni_net_process_request(kni, &req);
+	ret = kni_net_process_request(dev, &req);
 
 	return (ret == 0) ? req.result : ret;
 }
@@ -638,14 +644,13 @@ kni_net_change_mtu(struct net_device *dev, int new_mtu)
 {
 	int ret;
 	struct rte_kni_request req;
-	struct kni_dev *kni = netdev_priv(dev);
 
 	pr_debug("kni_net_change_mtu new mtu %d to be set\n", new_mtu);
 
 	memset(&req, 0, sizeof(req));
 	req.req_id = RTE_KNI_REQ_CHANGE_MTU;
 	req.new_mtu = new_mtu;
-	ret = kni_net_process_request(kni, &req);
+	ret = kni_net_process_request(dev, &req);
 	if (ret == 0 && req.result == 0)
 		dev->mtu = new_mtu;
 
@@ -656,7 +661,6 @@ static void
 kni_net_change_rx_flags(struct net_device *netdev, int flags)
 {
 	struct rte_kni_request req;
-	struct kni_dev *kni = netdev_priv(netdev);
 
 	memset(&req, 0, sizeof(req));
 
@@ -678,7 +682,7 @@ kni_net_change_rx_flags(struct net_device *netdev, int flags)
 			req.promiscusity = 0;
 	}
 
-	kni_net_process_request(kni, &req);
+	kni_net_process_request(netdev, &req);
 }
 
 /*
@@ -737,7 +741,6 @@ kni_net_set_mac(struct net_device *netdev, void *p)
 {
 	int ret;
 	struct rte_kni_request req;
-	struct kni_dev *kni;
 	struct sockaddr *addr = p;
 
 	memset(&req, 0, sizeof(req));
@@ -749,8 +752,7 @@ kni_net_set_mac(struct net_device *netdev, void *p)
 	memcpy(req.mac_addr, addr->sa_data, netdev->addr_len);
 	memcpy(netdev->dev_addr, addr->sa_data, netdev->addr_len);
 
-	kni = netdev_priv(netdev);
-	ret = kni_net_process_request(kni, &req);
+	ret = kni_net_process_request(netdev, &req);
 
 	return (ret == 0 ? req.result : ret);
 }
-- 
2.20.1


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

* Re: [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices
  2019-12-22 17:55 [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices Stephen Hemminger
@ 2020-01-17 16:43 ` Ferruh Yigit
  2020-03-18 15:17   ` Thomas Monjalon
  0 siblings, 1 reply; 7+ messages in thread
From: Ferruh Yigit @ 2020-01-17 16:43 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, stable

On 12/22/2019 5:55 PM, Stephen Hemminger wrote:
> This fixes a deadlock when using KNI with bifurcated drivers.
> Bringing kni device up always times out when using Mellanox
> devices.
> 
> The kernel KNI driver sends message to userspace to complete
> the request. For the case of bifurcated driver, this may involve
> an additional request to kernel to change state. This request
> would deadlock because KNI was holding the RTNL mutex.
> 
> This was a bad design which goes back to the original code.
> A workaround is for KNI driver to drop RTNL while waiting.
> To prevent the device from disappearing while the operation
> is in progress, it needs to hold reference to network device
> while waiting.
> 
> As an added benefit, an useless error check can also be removed.
> 
> Fixes: 3fc5ca2f6352 ("kni: initial import")
> Cc: stable@dpdk.org
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  kernel/linux/kni/kni_net.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
> index 1ba9b1b99f66..b7337c1410b8 100644
> --- a/kernel/linux/kni/kni_net.c
> +++ b/kernel/linux/kni/kni_net.c
> @@ -17,6 +17,7 @@
>  #include <linux/skbuff.h>
>  #include <linux/kthread.h>
>  #include <linux/delay.h>
> +#include <linux/rtnetlink.h>
>  
>  #include <rte_kni_common.h>
>  #include <kni_fifo.h>
> @@ -102,17 +103,15 @@ get_data_kva(struct kni_dev *kni, void *pkt_kva)
>   * It can be called to process the request.
>   */
>  static int
> -kni_net_process_request(struct kni_dev *kni, struct rte_kni_request *req)
> +kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
>  {
> +	struct kni_dev *kni = netdev_priv(dev);
>  	int ret = -1;
>  	void *resp_va;
>  	uint32_t num;
>  	int ret_val;
>  
> -	if (!kni || !req) {
> -		pr_err("No kni instance or request\n");
> -		return -EINVAL;
> -	}
> +	ASSERT_RTNL();
>  
>  	mutex_lock(&kni->sync_lock);
>  
> @@ -125,8 +124,17 @@ kni_net_process_request(struct kni_dev *kni, struct rte_kni_request *req)
>  		goto fail;
>  	}
>  
> +	/* Since we need to wait and RTNL mutex is held
> +	 * drop the mutex and hold refernce to keep device
> +	 */
> +	dev_hold(dev);
> +	rtnl_unlock();
> +
>  	ret_val = wait_event_interruptible_timeout(kni->wq,
>  			kni_fifo_count(kni->resp_q), 3 * HZ);
> +	rtnl_lock();
> +	dev_put(dev);
> +
>  	if (signal_pending(current) || ret_val <= 0) {
>  		ret = -ETIME;
>  		goto fail;

<...>

This patch cause a hang on my server, not sure what exactly was the problem but
kernel log was continuously printing "Cannot send to req_q". Will dig more.

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

* Re: [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices
  2020-01-17 16:43 ` Ferruh Yigit
@ 2020-03-18 15:17   ` Thomas Monjalon
  2020-05-06  0:14     ` Stephen Hemminger
  0 siblings, 1 reply; 7+ messages in thread
From: Thomas Monjalon @ 2020-03-18 15:17 UTC (permalink / raw)
  To: Stephen Hemminger, Ferruh Yigit; +Cc: dev, stable

17/01/2020 17:43, Ferruh Yigit:
> On 12/22/2019 5:55 PM, Stephen Hemminger wrote:
> > This fixes a deadlock when using KNI with bifurcated drivers.
> > Bringing kni device up always times out when using Mellanox
> > devices.
> > 
> > The kernel KNI driver sends message to userspace to complete
> > the request. For the case of bifurcated driver, this may involve
> > an additional request to kernel to change state. This request
> > would deadlock because KNI was holding the RTNL mutex.
> > 
> > This was a bad design which goes back to the original code.
> > A workaround is for KNI driver to drop RTNL while waiting.
> > To prevent the device from disappearing while the operation
> > is in progress, it needs to hold reference to network device
> > while waiting.
> > 
> > As an added benefit, an useless error check can also be removed.
> > 
> > Fixes: 3fc5ca2f6352 ("kni: initial import")
> > Cc: stable@dpdk.org
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> 
> This patch cause a hang on my server, not sure what exactly was the problem but
> kernel log was continuously printing "Cannot send to req_q". Will dig more.

Ferruh, did you have a chance to check what is hanging?
Stephen, is there any news on your side?



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

* Re: [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices
  2020-03-18 15:17   ` Thomas Monjalon
@ 2020-05-06  0:14     ` Stephen Hemminger
  2020-07-27 17:33       ` Ferruh Yigit
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2020-05-06  0:14 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Ferruh Yigit, dev, stable

On Wed, 18 Mar 2020 16:17:57 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 17/01/2020 17:43, Ferruh Yigit:
> > On 12/22/2019 5:55 PM, Stephen Hemminger wrote:  
> > > This fixes a deadlock when using KNI with bifurcated drivers.
> > > Bringing kni device up always times out when using Mellanox
> > > devices.
> > > 
> > > The kernel KNI driver sends message to userspace to complete
> > > the request. For the case of bifurcated driver, this may involve
> > > an additional request to kernel to change state. This request
> > > would deadlock because KNI was holding the RTNL mutex.
> > > 
> > > This was a bad design which goes back to the original code.
> > > A workaround is for KNI driver to drop RTNL while waiting.
> > > To prevent the device from disappearing while the operation
> > > is in progress, it needs to hold reference to network device
> > > while waiting.
> > > 
> > > As an added benefit, an useless error check can also be removed.
> > > 
> > > Fixes: 3fc5ca2f6352 ("kni: initial import")
> > > Cc: stable@dpdk.org
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---  
> > 
> > This patch cause a hang on my server, not sure what exactly was the problem but
> > kernel log was continuously printing "Cannot send to req_q". Will dig more.  
> 
> Ferruh, did you have a chance to check what is hanging?
> Stephen, is there any news on your side?
> 
> 

It did not hang when I tested it. The bug report is still open

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

* Re: [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices
  2020-05-06  0:14     ` Stephen Hemminger
@ 2020-07-27 17:33       ` Ferruh Yigit
  2020-07-27 17:52         ` Stephen Hemminger
  0 siblings, 1 reply; 7+ messages in thread
From: Ferruh Yigit @ 2020-07-27 17:33 UTC (permalink / raw)
  To: Stephen Hemminger, Thomas Monjalon; +Cc: dev, stable

On 5/6/2020 1:14 AM, Stephen Hemminger wrote:
> On Wed, 18 Mar 2020 16:17:57 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
>> 17/01/2020 17:43, Ferruh Yigit:
>>> On 12/22/2019 5:55 PM, Stephen Hemminger wrote:  
>>>> This fixes a deadlock when using KNI with bifurcated drivers.
>>>> Bringing kni device up always times out when using Mellanox
>>>> devices.
>>>>
>>>> The kernel KNI driver sends message to userspace to complete
>>>> the request. For the case of bifurcated driver, this may involve
>>>> an additional request to kernel to change state. This request
>>>> would deadlock because KNI was holding the RTNL mutex.
>>>>
>>>> This was a bad design which goes back to the original code.
>>>> A workaround is for KNI driver to drop RTNL while waiting.
>>>> To prevent the device from disappearing while the operation
>>>> is in progress, it needs to hold reference to network device
>>>> while waiting.
>>>>
>>>> As an added benefit, an useless error check can also be removed.
>>>>
>>>> Fixes: 3fc5ca2f6352 ("kni: initial import")
>>>> Cc: stable@dpdk.org
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>> ---  
>>>
>>> This patch cause a hang on my server, not sure what exactly was the problem but
>>> kernel log was continuously printing "Cannot send to req_q". Will dig more.  
>>
>> Ferruh, did you have a chance to check what is hanging?
>> Stephen, is there any news on your side?
>>
>>
> 
> It did not hang when I tested it. The bug report is still open
> 

Sorry for the delay, since I am working remotely I was worried about loosing the
connection to my server, finally I did create a virtual environment to test again.

I confirm the hang observed %100 when two different process updates the kni
interface, like two different process sets the mtu. Without this patch this
works fine.

I understand the motivation of the patch, but with change there is a possibility
to hang the server, which we can't allow, need to find another way. Can updating
mlx interface wait KNI interface operation to complete?

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

* Re: [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices
  2020-07-27 17:33       ` Ferruh Yigit
@ 2020-07-27 17:52         ` Stephen Hemminger
  2020-07-28  8:56           ` Igor Ryzhov
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2020-07-27 17:52 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Thomas Monjalon, dev, stable

On Mon, 27 Jul 2020 18:33:08 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 5/6/2020 1:14 AM, Stephen Hemminger wrote:
> > On Wed, 18 Mar 2020 16:17:57 +0100
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> >   
> >> 17/01/2020 17:43, Ferruh Yigit:  
> >>> On 12/22/2019 5:55 PM, Stephen Hemminger wrote:    
> >>>> This fixes a deadlock when using KNI with bifurcated drivers.
> >>>> Bringing kni device up always times out when using Mellanox
> >>>> devices.
> >>>>
> >>>> The kernel KNI driver sends message to userspace to complete
> >>>> the request. For the case of bifurcated driver, this may involve
> >>>> an additional request to kernel to change state. This request
> >>>> would deadlock because KNI was holding the RTNL mutex.
> >>>>
> >>>> This was a bad design which goes back to the original code.
> >>>> A workaround is for KNI driver to drop RTNL while waiting.
> >>>> To prevent the device from disappearing while the operation
> >>>> is in progress, it needs to hold reference to network device
> >>>> while waiting.
> >>>>
> >>>> As an added benefit, an useless error check can also be removed.
> >>>>
> >>>> Fixes: 3fc5ca2f6352 ("kni: initial import")
> >>>> Cc: stable@dpdk.org
> >>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >>>> ---    
> >>>
> >>> This patch cause a hang on my server, not sure what exactly was the problem but
> >>> kernel log was continuously printing "Cannot send to req_q". Will dig more.    
> >>
> >> Ferruh, did you have a chance to check what is hanging?
> >> Stephen, is there any news on your side?
> >>
> >>  
> > 
> > It did not hang when I tested it. The bug report is still open
> >   
> 
> Sorry for the delay, since I am working remotely I was worried about loosing the
> connection to my server, finally I did create a virtual environment to test again.
> 
> I confirm the hang observed %100 when two different process updates the kni
> interface, like two different process sets the mtu. Without this patch this
> works fine.
> 
> I understand the motivation of the patch, but with change there is a possibility
> to hang the server, which we can't allow, need to find another way. Can updating
> mlx interface wait KNI interface operation to complete?

Still KNI driver is broken. Calling userspace with RTNL held is fundamentally
broken design. If KNI were to be incorporated in upstream kernel, then the netdev
developer would see this.

What ever solution you think is best.
I will continue to recommend against anyone using KNI.

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

* Re: [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices
  2020-07-27 17:52         ` Stephen Hemminger
@ 2020-07-28  8:56           ` Igor Ryzhov
  0 siblings, 0 replies; 7+ messages in thread
From: Igor Ryzhov @ 2020-07-28  8:56 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Ferruh Yigit, Thomas Monjalon, dev, dpdk stable

Hi,

Didn't see this patch previously, but we came up with the same idea
internally and also faced a hang during the application shutdown.
We didn't dig deep, but it occurred in kni_release function.

Igor

On Mon, Jul 27, 2020 at 8:53 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Mon, 27 Jul 2020 18:33:08 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> > On 5/6/2020 1:14 AM, Stephen Hemminger wrote:
> > > On Wed, 18 Mar 2020 16:17:57 +0100
> > > Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > >> 17/01/2020 17:43, Ferruh Yigit:
> > >>> On 12/22/2019 5:55 PM, Stephen Hemminger wrote:
> > >>>> This fixes a deadlock when using KNI with bifurcated drivers.
> > >>>> Bringing kni device up always times out when using Mellanox
> > >>>> devices.
> > >>>>
> > >>>> The kernel KNI driver sends message to userspace to complete
> > >>>> the request. For the case of bifurcated driver, this may involve
> > >>>> an additional request to kernel to change state. This request
> > >>>> would deadlock because KNI was holding the RTNL mutex.
> > >>>>
> > >>>> This was a bad design which goes back to the original code.
> > >>>> A workaround is for KNI driver to drop RTNL while waiting.
> > >>>> To prevent the device from disappearing while the operation
> > >>>> is in progress, it needs to hold reference to network device
> > >>>> while waiting.
> > >>>>
> > >>>> As an added benefit, an useless error check can also be removed.
> > >>>>
> > >>>> Fixes: 3fc5ca2f6352 ("kni: initial import")
> > >>>> Cc: stable@dpdk.org
> > >>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > >>>> ---
> > >>>
> > >>> This patch cause a hang on my server, not sure what exactly was the
> problem but
> > >>> kernel log was continuously printing "Cannot send to req_q". Will
> dig more.
> > >>
> > >> Ferruh, did you have a chance to check what is hanging?
> > >> Stephen, is there any news on your side?
> > >>
> > >>
> > >
> > > It did not hang when I tested it. The bug report is still open
> > >
> >
> > Sorry for the delay, since I am working remotely I was worried about
> loosing the
> > connection to my server, finally I did create a virtual environment to
> test again.
> >
> > I confirm the hang observed %100 when two different process updates the
> kni
> > interface, like two different process sets the mtu. Without this patch
> this
> > works fine.
> >
> > I understand the motivation of the patch, but with change there is a
> possibility
> > to hang the server, which we can't allow, need to find another way. Can
> updating
> > mlx interface wait KNI interface operation to complete?
>
> Still KNI driver is broken. Calling userspace with RTNL held is
> fundamentally
> broken design. If KNI were to be incorporated in upstream kernel, then the
> netdev
> developer would see this.
>
> What ever solution you think is best.
> I will continue to recommend against anyone using KNI.
>

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

end of thread, other threads:[~2020-07-28  8:56 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-22 17:55 [dpdk-dev] [PATCH] kni: fix kernel deadlock when using mlx devices Stephen Hemminger
2020-01-17 16:43 ` Ferruh Yigit
2020-03-18 15:17   ` Thomas Monjalon
2020-05-06  0:14     ` Stephen Hemminger
2020-07-27 17:33       ` Ferruh Yigit
2020-07-27 17:52         ` Stephen Hemminger
2020-07-28  8:56           ` Igor Ryzhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).