[dpdk-dev] Potential deadlock in KNI RX path

[dpdk-dev] Potential deadlock in KNI RX path

[Jay Rolette]

Over a year ago, Neil pointed out that calling netif_rx() from
kni_net_rx_normal() was a bug and could cause lockups. Here's the comment:

http://dpdk.org/ml/archives/dev/2015-March/015783.html

Looking at the current code base, it is still calling netif_rx() instead of
netif_rx_ni() as suggested.

Did that fall through the cracks or is there disagreement about whether it
was the right thing to do?

Thanks,
Jay

Re: [dpdk-dev] Potential deadlock in KNI RX path

[Ferruh Yigit]

On 4/6/2016 9:22 PM, Jay Rolette wrote:
> Over a year ago, Neil pointed out that calling netif_rx() from
> kni_net_rx_normal() was a bug and could cause lockups. Here's the comment:
> 
> http://dpdk.org/ml/archives/dev/2015-March/015783.html
> 
> Looking at the current code base, it is still calling netif_rx() instead of
> netif_rx_ni() as suggested.
> 
> Did that fall through the cracks or is there disagreement about whether it
> was the right thing to do?
> 
> Thanks,
> Jay
> 
Hi Jay,

Using netif_rx_ni() looks like correct thing to do. I will send a patch
for it.

But we poll on this function, this means doing lots of
preempt_disable/enable(); which may have a negative effect on
performance. Although I did very brief test and did not observed any
performance degradation.

It can be great if somebody out already using KNI do some performance
analysis with the patch.

Another observation, for multiple kthread mode, the ksoftirq threads CPU
consumption removed when switched to netif_rx_ni(), I remember in mail
list somebody complained about ksoftirq CPU usage increase for this case.

Regards,
ferruh

[dpdk-dev] [PATCH] kni: fix possible deadlock

[Ferruh Yigit]

netif_rx() should be used in interrupt context. Replace it with
netif_rx_ni() which is safe to use in process context.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/librte_eal/linuxapp/kni/kni_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c b/lib/librte_eal/linuxapp/kni/kni_net.c
index e02edcb..cfa8339 100644
--- a/lib/librte_eal/linuxapp/kni/kni_net.c
+++ b/lib/librte_eal/linuxapp/kni/kni_net.c
@@ -175,7 +175,7 @@ kni_net_rx_normal(struct kni_dev *kni)
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 			/* Call netif interface */
-			netif_rx(skb);
+			netif_rx_ni(skb);
 
 			/* Update statistics */
 			kni->stats.rx_bytes += len;
-- 
2.5.5

Re: [dpdk-dev] [PATCH] kni: fix possible deadlock

[Thomas Monjalon]

2016-04-07 16:55, Ferruh Yigit:
> netif_rx() should be used in interrupt context. Replace it with
> netif_rx_ni() which is safe to use in process context.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied, thanks

Re: [dpdk-dev] Potential deadlock in KNI RX path

[Jay Rolette]

On Thu, Apr 7, 2016 at 9:33 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 4/6/2016 9:22 PM, Jay Rolette wrote:
> > Over a year ago, Neil pointed out that calling netif_rx() from
> > kni_net_rx_normal() was a bug and could cause lockups. Here's the
> comment:
> >
> > http://dpdk.org/ml/archives/dev/2015-March/015783.html
> >
> > Looking at the current code base, it is still calling netif_rx() instead
> of
> > netif_rx_ni() as suggested.
> >
> > Did that fall through the cracks or is there disagreement about whether
> it
> > was the right thing to do?
> >
> > Thanks,
> > Jay
> >
> Hi Jay,
>
> Using netif_rx_ni() looks like correct thing to do. I will send a patch
> for it.
>
> But we poll on this function, this means doing lots of
> preempt_disable/enable(); which may have a negative effect on
> performance. Although I did very brief test and did not observed any
> performance degradation.
>
> It can be great if somebody out already using KNI do some performance
> analysis with the patch.
>
> Another observation, for multiple kthread mode, the ksoftirq threads CPU
> consumption removed when switched to netif_rx_ni(), I remember in mail
> list somebody complained about ksoftirq CPU usage increase for this case.
>

Thanks Ferruh. We'll make that change in our local repo and run it through
its paces.

Jay