[PATCH RFC] gianfar: Make polling safe with IRQs disabled

[PATCH RFC] gianfar: Make polling safe with IRQs disabled

[Anton Vorontsov]

When using KGDBoE, gianfar driver spits 'Interrupt problem' messages,
which appears to be a legitimate warning, i.e. we may end up calling
netif_receive_skb() or vlan_hwaccel_receive_skb() with IRQs disabled.

This patch reworks the RX path so that if netpoll is enabled (the
only case when the driver don't know from what context the polling
may be called), we check whether IRQs are disabled, and if so we
fall back to safe variants of skb receiving functions.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

I'm not sure if this is suitable for mainline since it doesn't
have KGDBoE support. Jason, if the patch is OK, would you like
to merge it into KGDB tree?

 drivers/net/gianfar.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 197b358..024ca4a 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -2412,9 +2412,17 @@ static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
 {
 	struct gfar_private *priv = netdev_priv(dev);
 	struct rxfcb *fcb = NULL;
-
+	int irqs_dis = 0;
 	int ret;
 
+	/*
+	 * With netpoll we don't know from what context we're called (e.g
+	 * KGDBoE may call us from an exception handler), otherwise we're
+	 * pretty sure that IRQs are enabled.
+	 */
+#ifdef CONFIG_NETPOLL
+	irqs_dis = irqs_disabled();
+#endif
 	/* fcb is at the beginning if exists */
 	fcb = (struct rxfcb *)skb->data;
 
@@ -2432,7 +2440,10 @@ static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
 
 	/* Send the packet up the stack */
 	if (unlikely(priv->vlgrp && (fcb->flags & RXFCB_VLN)))
-		ret = vlan_hwaccel_receive_skb(skb, priv->vlgrp, fcb->vlctl);
+		ret = __vlan_hwaccel_rx(skb, priv->vlgrp, fcb->vlctl,
+					!irqs_dis);
+	else if (irqs_dis)
+		ret = netif_rx(skb);
 	else
 		ret = netif_receive_skb(skb);
 
@@ -2504,8 +2515,6 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit)
 				skb_put(skb, pkt_len);
 				dev->stats.rx_bytes += pkt_len;
 
-				if (in_irq() || irqs_disabled())
-					printk("Interrupt problem!\n");
 				gfar_process_frame(dev, skb, amount_pull);
 
 			} else {
-- 
1.6.3.3

Re: [PATCH RFC] gianfar: Make polling safe with IRQs disabled

[Jon Loeliger]

> When using KGDBoE, gianfar driver spits 'Interrupt problem' messages,
> which appears to be a legitimate warning, i.e. we may end up calling
> netif_receive_skb() or vlan_hwaccel_receive_skb() with IRQs disabled.
> 
> This patch reworks the RX path so that if netpoll is enabled (the
> only case when the driver don't know from what context the polling
> may be called), we check whether IRQs are disabled, and if so we
> fall back to safe variants of skb receiving functions.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> 
> I'm not sure if this is suitable for mainline since it doesn't
> have KGDBoE support. Jason, if the patch is OK, would you like
> to merge it into KGDB tree?

It's a legitimate problem with or without KGDBoE.  I see it
occasionally when conn_track is enabled as well, for example.

jdl

Re: [PATCH RFC] gianfar: Make polling safe with IRQs disabled

[Anton Vorontsov]

On Thu, Nov 05, 2009 at 08:01:10AM -0600, Jon Loeliger wrote:
> > When using KGDBoE, gianfar driver spits 'Interrupt problem' messages,
> > which appears to be a legitimate warning, i.e. we may end up calling
> > netif_receive_skb() or vlan_hwaccel_receive_skb() with IRQs disabled.
> > 
> > This patch reworks the RX path so that if netpoll is enabled (the
> > only case when the driver don't know from what context the polling
> > may be called), we check whether IRQs are disabled, and if so we
> > fall back to safe variants of skb receiving functions.
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> > 
> > I'm not sure if this is suitable for mainline since it doesn't
> > have KGDBoE support. Jason, if the patch is OK, would you like
> > to merge it into KGDB tree?
> 
> It's a legitimate problem with or without KGDBoE.  I see it
> occasionally when conn_track is enabled as well, for example.

Hm, then I'd better remove the #ifdef CONFIG_NETPOLL.

Interestingly though, why conn_track does the polling with irqs
disabled, could be a bug in the conn_track? Because pretty much
drivers assume that polling is called with IRQs enabled.

If it's easily reproducible, could you replace the printk() with
WARN_ON(1) and post the backtrace? Or I can try to reproduce the
issue if you tell me how.

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH RFC] gianfar: Make polling safe with IRQs disabled

[Jon Loeliger]

> 
> Hm, then I'd better remove the #ifdef CONFIG_NETPOLL.
> 
> Interestingly though, why conn_track does the polling with irqs
> disabled, could be a bug in the conn_track? Because pretty much
> drivers assume that polling is called with IRQs enabled.
> 
> If it's easily reproducible, could you replace the printk() with
> WARN_ON(1) and post the backtrace? Or I can try to reproduce the
> issue if you tell me how.
> 
> Thanks!

Yeah, I can reproduce it.  I'll try and get that for you.

jdl

Re: [PATCH RFC] gianfar: Make polling safe with IRQs disabled

[Jon Loeliger]

> > 
> > If it's easily reproducible, could you replace the printk() with
> > WARN_ON(1) and post the backtrace? Or I can try to reproduce the
> > issue if you tell me how.
> > 
> > Thanks!
> 
> Yeah, I can reproduce it.  I'll try and get that for you.
> 
> jdl

This is with essentially a stock 2.6.31 kernel for an 8315.
I've seen the problem for both task 'insmod' and 'iptables'.
Um, conn_track is a module being loaded here.  Our brief analysis
runs like this:

    This is an issue with the gianfar driver.  In gfar_poll(), irqs are
    disabled for the handling of gfar_clean_tx_ring(dev) (line 1928).
    In this call, skbs can call skb_recycle_check, which can release
    head state (net/core/skbuff.c@506), which can cause conntrack
    cleanup (net/core/skbuff.c@402->include/linux/skbuff.h@1923), which
    cannot be done with IRQs disabled...that is the badness.

HTH,
jdl


[   34.775619] nf_conntrack version 0.5.0 (1008 buckets, 4032 max)
[   34.963135] ------------[ cut here ]------------
[   34.967804] Badness at kernel/softirq.c:143
[   34.972016] NIP: c003e3c4 LR: c423a528 CTR: c003e344
[   34.977018] REGS: c15d1ab0 TRAP: 0700   Not tainted  (2.6.31-xeno)
[   34.983236] MSR: 00021032 <ME,CE,IR,DR>  CR: 24000284  XER: 20000000
[   34.989689] TASK = c343a060[977] 'insmod' THREAD: c15d0000
[   34.995032] GPR00: 00000001 c15d1b60 c343a060 00000001 000000a4 00000052 00000001 00000000 
[   35.003501] GPR08: 00000101 c0450000 c3572d20 c003e344 24000282 100c5288 00000001 00000040 
[   35.011971] GPR16: c2e5c2f0 00009032 c2e5c2c0 c2e5c000 00000100 00000000 c2e5c340 00000098 
[   35.020440] GPR24: 00000260 c2428760 00000800 c153b800 00000000 c1018c98 c15d0000 c15d1b60 
[   35.029120] NIP [c003e3c4] local_bh_enable+0x80/0xc4
[   35.034174] LR [c423a528] destroy_conntrack+0xd4/0x13c [nf_conntrack]
[   35.040652] Call Trace:
[   35.043128] [c15d1b60] [c003e32c] local_bh_disable+0x1c/0x34 (unreliable)
[   35.050001] [c15d1b70] [c423a528] destroy_conntrack+0xd4/0x13c [nf_conntrack]
[   35.057205] [c15d1b80] [c02c6370] nf_conntrack_destroy+0x3c/0x70
[   35.063263] --- Exception: c428168c at 0xc15d1c50
[   35.063272]     LR = 0xc15d1c40
[   35.071165] [c15d1ba0] [c0286f3c] skb_release_head_state+0x100/0x104 (unreliable)
[   35.078718] [c15d1bb0] [c0288340] skb_recycle_check+0x8c/0x10c
[   35.084611] [c15d1bc0] [c01e1688] gfar_poll+0x190/0x384
[   35.089887] [c15d1c10] [c02935ac] net_rx_action+0xec/0x22c
[   35.095432] [c15d1c50] [c003dd8c] __do_softirq+0xe8/0x224
[   35.100885] [c15d1ca0] [c000624c] do_softirq+0x78/0x80
[   35.106071] [c15d1cb0] [c003d868] irq_exit+0x60/0x78
[   35.111082] [c15d1cc0] [c0006714] do_IRQ+0x98/0xb0
[   35.115921] [c15d1ce0] [c0014af8] ret_from_except+0x0/0x14
[   35.121474] --- Exception: 501 at strcmp+0xc/0x24
[   35.121483]     LR = find_symbol_in_section+0x38/0xc0
[   35.131285] [c15d1da0] [00000000] (null) (unreliable)
[   35.136390] [c15d1dc0] [c0064e08] each_symbol_in_section+0x7c/0xb4
[   35.142623] [c15d1df0] [c0065340] each_symbol+0x34/0x148
[   35.147983] [c15d1e70] [c0065488] find_symbol+0x34/0x78
[   35.153257] [c15d1ea0] [c00681d8] load_module+0x8e4/0x12ec
[   35.158792] [c15d1f20] [c0068c60] sys_init_module+0x80/0x208
[   35.164501] [c15d1f40] [c0014460] ret_from_syscall+0x0/0x38
[   35.170124] --- Exception: c01 at 0xfea6a8c
[   35.170132]     LR = 0x10016dcc
[   35.177485] Instruction dump:
[   35.180477] 70090004 40820068 81610000 800b0004 bbcbfff8 7d615b78 7c0803a6 4e800020 
[   35.188332] 3d20c045 8009b3c0 7c000034 5400d97e <0f000000> 2f800000 419effa8 38000001 

[PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs

[Anton Vorontsov]

Before calling gfar_clean_tx_ring() we grab an irqsave spinlock, and
then try to recycle an skb, which requires IRQs to be enabled. This
leads to the following badness:

nf_conntrack version 0.5.0 (1008 buckets, 4032 max)
------------[ cut here ]------------
Badness at kernel/softirq.c:143
NIP: c003e3c4 LR: c423a528 CTR: c003e344
...
NIP [c003e3c4] local_bh_enable+0x80/0xc4
LR [c423a528] destroy_conntrack+0xd4/0x13c [nf_conntrack]
Call Trace:
[c15d1b60] [c003e32c] local_bh_disable+0x1c/0x34 (unreliable)
[c15d1b70] [c423a528] destroy_conntrack+0xd4/0x13c [nf_conntrack]
[c15d1b80] [c02c6370] nf_conntrack_destroy+0x3c/0x70
--- Exception: c428168c at 0xc15d1c50
LR = 0xc15d1c40
[c15d1ba0] [c0286f3c] skb_release_head_state+0x100/0x104 (unreliable)
[c15d1bb0] [c0288340] skb_recycle_check+0x8c/0x10c
[c15d1bc0] [c01e1688] gfar_poll+0x190/0x384
[c15d1c10] [c02935ac] net_rx_action+0xec/0x22c
[c15d1c50] [c003dd8c] __do_softirq+0xe8/0x224
[c15d1ca0] [c000624c] do_softirq+0x78/0x80
[c15d1cb0] [c003d868] irq_exit+0x60/0x78
...

We can't easily get rid of the irqsave spinlock, because we must
guard ourselves from start_xmit.

So, fix this by dropping the irqsave spinlock from both xmit_start
and clean_tx_ring routines. Instead, lock the whole tx queue via
netif_tx_lock_bh() in clean_tx_ring().

Reported-by: Jon Loeliger <jdl@jdl.com>
Not-yet-Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

On Thu, Nov 05, 2009 at 09:43:18AM -0600, Jon Loeliger wrote:
[...]
> This is with essentially a stock 2.6.31 kernel for an 8315.
> I've seen the problem for both task 'insmod' and 'iptables'.
> Um, conn_track is a module being loaded here.  Our brief analysis
> runs like this:
> 
>     This is an issue with the gianfar driver.  In gfar_poll(), irqs are
>     disabled for the handling of gfar_clean_tx_ring(dev) (line 1928).
>     In this call, skbs can call skb_recycle_check, which can release
>     head state (net/core/skbuff.c@506), which can cause conntrack
>     cleanup (net/core/skbuff.c@402->include/linux/skbuff.h@1923), which
>     cannot be done with IRQs disabled...that is the badness.

Ugh.

We may try to call netif_tx_lock_bh() in gfar_clean_tx_ring() and
drop the irqsave spinlock start_xmit (sky2-like scheme).

But that basically means that with skb recycling we can't safely
use KGDBoE, though we can add something like this:

| diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
| index a00ec63..4d82bd7 100644
| --- a/drivers/net/gianfar.c
| +++ b/drivers/net/gianfar.c
| @@ -1619,7 +1619,8 @@ static int gfar_clean_tx_ring(struct net_device *dev)
|  		 * If there's room in the queue (limit it to rx_buffer_size)
|  		 * we add this skb back into the pool, if it's the right size
|  		 */
| -		if (skb_queue_len(&priv->rx_recycle) < priv->rx_ring_size &&
| +		if (!irqs_disabled() &&
| +			skb_queue_len(&priv->rx_recycle) < priv->rx_ring_size &&
|  				skb_recycle_check(skb, priv->rx_buffer_size +
|  					RXBUF_ALIGNMENT))
| 			__skb_queue_head(&priv->rx_recycle, skb);

So we won't recycle skbs with irqs disabled.

Anyway, apart from KGDBoE, the following patch might fix the conntrack
issue, can you try it?

With this patch the kernel boots via NFS, and survives netperf, though
I'd like to audit changes a little more and run some netperf tests, I
might also put the netif_tx_lock_bh() stuff out of the loop.

So this patch isn't for the merge.

 drivers/net/gianfar.c |   19 +++----------------
 1 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 197b358..a0ae604 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1899,10 +1899,8 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u32 lstatus;
 	int i, rq = 0;
 	u32 bufaddr;
-	unsigned long flags;
 	unsigned int nr_frags, length;
 
-
 	rq = skb->queue_mapping;
 	tx_queue = priv->tx_queue[rq];
 	txq = netdev_get_tx_queue(dev, rq);
@@ -1928,14 +1926,11 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* total number of fragments in the SKB */
 	nr_frags = skb_shinfo(skb)->nr_frags;
 
-	spin_lock_irqsave(&tx_queue->txlock, flags);
-
 	/* check if there is space to queue this packet */
 	if ((nr_frags+1) > tx_queue->num_txbdfree) {
 		/* no space, stop the queue */
 		netif_tx_stop_queue(txq);
 		dev->stats.tx_fifo_errors++;
-		spin_unlock_irqrestore(&tx_queue->txlock, flags);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -2033,9 +2028,6 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Tell the DMA to go go go */
 	gfar_write(&regs->tstat, TSTAT_CLEAR_THALT >> tx_queue->qindex);
 
-	/* Unlock priv */
-	spin_unlock_irqrestore(&tx_queue->txlock, flags);
-
 	return NETDEV_TX_OK;
 }
 
@@ -2550,7 +2542,6 @@ static int gfar_poll(struct napi_struct *napi, int budget)
 	int tx_cleaned = 0, i, left_over_budget = budget;
 	unsigned long serviced_queues = 0;
 	int num_queues = 0;
-	unsigned long flags;
 
 	num_queues = gfargrp->num_rx_queues;
 	budget_per_queue = budget/num_queues;
@@ -2570,13 +2561,9 @@ static int gfar_poll(struct napi_struct *napi, int budget)
 			rx_queue = priv->rx_queue[i];
 			tx_queue = priv->tx_queue[rx_queue->qindex];
 
-			/* If we fail to get the lock,
-			 * don't bother with the TX BDs */
-			if (spin_trylock_irqsave(&tx_queue->txlock, flags)) {
-				tx_cleaned += gfar_clean_tx_ring(tx_queue);
-				spin_unlock_irqrestore(&tx_queue->txlock,
-							flags);
-			}
+			netif_tx_lock_bh(priv->ndev);
+			tx_cleaned += gfar_clean_tx_ring(tx_queue);
+			netif_tx_unlock_bh(priv->ndev);
 
 			rx_cleaned_per_queue = gfar_clean_rx_ring(rx_queue,
 							budget_per_queue);
-- 
1.6.3.3

RE: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs

[Kumar Gopalpet-B05799]

=20


[.....]
> drivers/net/gianfar.c |   19 +++----------------
> 1 files changed, 3 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c=20
>index 197b358..a0ae604 100644
>--- a/drivers/net/gianfar.c
>+++ b/drivers/net/gianfar.c
>@@ -1899,10 +1899,8 @@ static int gfar_start_xmit(struct=20
>sk_buff *skb, struct net_device *dev)
> 	u32 lstatus;
> 	int i, rq =3D 0;
> 	u32 bufaddr;
>-	unsigned long flags;
> 	unsigned int nr_frags, length;
>=20
>-
> 	rq =3D skb->queue_mapping;
> 	tx_queue =3D priv->tx_queue[rq];
> 	txq =3D netdev_get_tx_queue(dev, rq);
>@@ -1928,14 +1926,11 @@ static int gfar_start_xmit(struct=20
>sk_buff *skb, struct net_device *dev)
> 	/* total number of fragments in the SKB */
> 	nr_frags =3D skb_shinfo(skb)->nr_frags;
>=20
>-	spin_lock_irqsave(&tx_queue->txlock, flags);
>-
> 	/* check if there is space to queue this packet */
> 	if ((nr_frags+1) > tx_queue->num_txbdfree) {
> 		/* no space, stop the queue */
> 		netif_tx_stop_queue(txq);
> 		dev->stats.tx_fifo_errors++;
>-		spin_unlock_irqrestore(&tx_queue->txlock, flags);
> 		return NETDEV_TX_BUSY;
> 	}
>=20
>@@ -2033,9 +2028,6 @@ static int gfar_start_xmit(struct=20
>sk_buff *skb, struct net_device *dev)
> 	/* Tell the DMA to go go go */
> 	gfar_write(&regs->tstat, TSTAT_CLEAR_THALT >> tx_queue->qindex);
>=20
>-	/* Unlock priv */
>-	spin_unlock_irqrestore(&tx_queue->txlock, flags);
>-
> 	return NETDEV_TX_OK;
> }
>=20
>@@ -2550,7 +2542,6 @@ static int gfar_poll(struct napi_struct=20
>*napi, int budget)
> 	int tx_cleaned =3D 0, i, left_over_budget =3D budget;
> 	unsigned long serviced_queues =3D 0;
> 	int num_queues =3D 0;
>-	unsigned long flags;
>=20
> 	num_queues =3D gfargrp->num_rx_queues;
> 	budget_per_queue =3D budget/num_queues;
>@@ -2570,13 +2561,9 @@ static int gfar_poll(struct napi_struct=20
>*napi, int budget)
> 			rx_queue =3D priv->rx_queue[i];
> 			tx_queue =3D priv->tx_queue[rx_queue->qindex];
>=20
>-			/* If we fail to get the lock,
>-			 * don't bother with the TX BDs */
>-			if=20
>(spin_trylock_irqsave(&tx_queue->txlock, flags)) {
>-				tx_cleaned +=3D=20
>gfar_clean_tx_ring(tx_queue);
>-			=09
>spin_unlock_irqrestore(&tx_queue->txlock,
>-							flags);
>-			}
>+			netif_tx_lock_bh(priv->ndev);

Will this not lead to locking all the tx queues even though at this
point we are working on a "particular queue" ?

>+			tx_cleaned +=3D gfar_clean_tx_ring(tx_queue);
>+			netif_tx_unlock_bh(priv->ndev);
>=20
> 			rx_cleaned_per_queue =3D=20
>gfar_clean_rx_ring(rx_queue,
> 						=09
>budget_per_queue);
>--

--

Thanks
Sandeep

Re: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs

[Anton Vorontsov]

On Thu, Nov 05, 2009 at 10:53:08PM +0530, Kumar Gopalpet-B05799 wrote:
[...]
> >(spin_trylock_irqsave(&tx_queue->txlock, flags)) {
> >-				tx_cleaned += 
> >gfar_clean_tx_ring(tx_queue);
> >-				
> >spin_unlock_irqrestore(&tx_queue->txlock,
> >-							flags);
> >-			}
> >+			netif_tx_lock_bh(priv->ndev);
> 
> Will this not lead to locking all the tx queues even though at this
> point we are working on a "particular queue" ?

Yeah, per-txq locking would be better (or not.. I need to netperf
it).

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs

[Jon Loeliger]

> 
> [.....]
> > drivers/net/gianfar.c |   19 +++----------------
> > 1 files changed, 3 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c=20
> >index 197b358..a0ae604 100644
> >--- a/drivers/net/gianfar.c
> >+++ b/drivers/net/gianfar.c
> >@@ -1899,10 +1899,8 @@ static int gfar_start_xmit(struct=20
> >sk_buff *skb, struct net_device *dev)
> > 	u32 lstatus;
> > 	int i, rq = 0;
> > 	u32 bufaddr;
> >-	unsigned long flags;
> > 	unsigned int nr_frags, length;
> > 
> >-
> > 	rq = skb->queue_mapping;
> > 	tx_queue = priv->tx_queue[rq];
> > 	txq = netdev_get_tx_queue(dev, rq);
> >@@ -1928,14 +1926,11 @@ static int gfar_start_xmit(struct 
> >sk_buff *skb, struct net_device *dev)
> > 	/* total number of fragments in the SKB */
> > 	nr_frags = skb_shinfo(skb)->nr_frags;
> > 
> >-	spin_lock_irqsave(&tx_queue->txlock, flags);
> >-
> > 	/* check if there is space to queue this packet */
> > 	if ((nr_frags+1) > tx_queue->num_txbdfree) {
> > 		/* no space, stop the queue */
> > 		netif_tx_stop_queue(txq);
> > 		dev->stats.tx_fifo_errors++;
> >-		spin_unlock_irqrestore(&tx_queue->txlock, flags);
> > 		return NETDEV_TX_BUSY;
> > 	}
> > 
> >@@ -2033,9 +2028,6 @@ static int gfar_start_xmit(struct 
> >sk_buff *skb, struct net_device *dev)
> > 	/* Tell the DMA to go go go */
> > 	gfar_write(&regs->tstat, TSTAT_CLEAR_THALT >> tx_queue->qindex);
> > 
> >-	/* Unlock priv */
> >-	spin_unlock_irqrestore(&tx_queue->txlock, flags);
> >-
> > 	return NETDEV_TX_OK;
> > }
> > 
> >@@ -2550,7 +2542,6 @@ static int gfar_poll(struct napi_struct 
> >*napi, int budget)
> > 	int tx_cleaned = 0, i, left_over_budget = budget;
> > 	unsigned long serviced_queues = 0;
> > 	int num_queues = 0;
> >-	unsigned long flags;
> > 
> > 	num_queues = gfargrp->num_rx_queues;
> > 	budget_per_queue = budget/num_queues;
> >@@ -2570,13 +2561,9 @@ static int gfar_poll(struct napi_struct 
> >*napi, int budget)
> > 			rx_queue = priv->rx_queue[i];
> > 			tx_queue = priv->tx_queue[rx_queue->qindex];
> > 
> >-			/* If we fail to get the lock,
> >-			 * don't bother with the TX BDs */
> >-			if 
> >(spin_trylock_irqsave(&tx_queue->txlock, flags)) {
> >-				tx_cleaned += 
> >gfar_clean_tx_ring(tx_queue);
> >-			=09
> >spin_unlock_irqrestore(&tx_queue->txlock,
> >-							flags);
> >-			}
> >+			netif_tx_lock_bh(priv->ndev);
> 
> Will this not lead to locking all the tx queues even though at this
> point we are working on a "particular queue" ?

Similar but different question:  What version is this patch based upon?
I can't find:

    > >index 197b358..a0ae604 100644

My search of 2.6.31 up through current linux head (v2.6.32-rc6-26-g91d3f9b)
and benh's current head (94a8d5caba74211ec76dac80fc6e2d5c391530df) do not
have a version of this code with separate txqueues.

I confess I'm not too familiar with the history here, so I don't know
if that feature is in flux, or came, or went, or what.

That boils down to:  I can't directly apply your patch, but I could
hand-fudge its intent, but only on a single tx queue variant.

jdl

Re: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs

[Anton Vorontsov]

On Thu, Nov 05, 2009 at 11:40:21AM -0600, Jon Loeliger wrote:
[...]
> > >+			netif_tx_lock_bh(priv->ndev);
> > 
> > Will this not lead to locking all the tx queues even though at this
> > point we are working on a "particular queue" ?
> 
> Similar but different question:  What version is this patch based upon?
> I can't find:
> 
>     > >index 197b358..a0ae604 100644
> 
> My search of 2.6.31 up through current linux head (v2.6.32-rc6-26-g91d3f9b)
> and benh's current head (94a8d5caba74211ec76dac80fc6e2d5c391530df) do not
> have a version of this code with separate txqueues.
> 
> I confess I'm not too familiar with the history here, so I don't know
> if that feature is in flux, or came, or went, or what.

Sorry for not mentioning, it's based on

git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git
+ compiler/sparse warnings fixes that I sent yesterday.

> That boils down to:  I can't directly apply your patch, but I could
> hand-fudge its intent, but only on a single tx queue variant.

Here is the patch on top of the Linus' git tree, if you haven't
already 'back-ported' the previous patch.

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 5bf31f1..5dca99c 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1274,7 +1274,6 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u32 lstatus;
 	int i;
 	u32 bufaddr;
-	unsigned long flags;
 	unsigned int nr_frags, length;
 
 	base = priv->tx_bd_base;
@@ -1298,14 +1297,11 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* total number of fragments in the SKB */
 	nr_frags = skb_shinfo(skb)->nr_frags;
 
-	spin_lock_irqsave(&priv->txlock, flags);
-
 	/* check if there is space to queue this packet */
 	if ((nr_frags+1) > priv->num_txbdfree) {
 		/* no space, stop the queue */
 		netif_stop_queue(dev);
 		dev->stats.tx_fifo_errors++;
-		spin_unlock_irqrestore(&priv->txlock, flags);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -1403,9 +1399,6 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Tell the DMA to go go go */
 	gfar_write(&priv->regs->tstat, TSTAT_CLEAR_THALT);
 
-	/* Unlock priv */
-	spin_unlock_irqrestore(&priv->txlock, flags);
-
 	return NETDEV_TX_OK;
 }
 
@@ -1915,17 +1908,14 @@ static int gfar_poll(struct napi_struct *napi, int budget)
 	struct net_device *dev = priv->ndev;
 	int tx_cleaned = 0;
 	int rx_cleaned = 0;
-	unsigned long flags;
 
 	/* Clear IEVENT, so interrupts aren't called again
 	 * because of the packets that have already arrived */
 	gfar_write(&priv->regs->ievent, IEVENT_RTX_MASK);
 
-	/* If we fail to get the lock, don't bother with the TX BDs */
-	if (spin_trylock_irqsave(&priv->txlock, flags)) {
-		tx_cleaned = gfar_clean_tx_ring(dev);
-		spin_unlock_irqrestore(&priv->txlock, flags);
-	}
+	netif_tx_lock_bh(priv->ndev);
+	tx_cleaned = gfar_clean_tx_ring(dev);
+	netif_tx_unlock_bh(priv->ndev);
 
 	rx_cleaned = gfar_clean_rx_ring(dev, budget);
 

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs

[Jon Loeliger]

> 
> Here is the patch on top of the Linus' git tree, if you haven't
> already 'back-ported' the previous patch.

This back-ported patch has been running in my (2.6.31) kernel
for a couple days now without showing any sign of problem.

Maybe throw a

Tested-by: Jon Loeliger <jdl@jdl.com>

at it?

jdl



> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> index 5bf31f1..5dca99c 100644
> --- a/drivers/net/gianfar.c
> +++ b/drivers/net/gianfar.c
> @@ -1274,7 +1274,6 @@ static int gfar_start_xmit(struct sk_buff *skb, struct 
> net_device *dev)
>  	u32 lstatus;
>  	int i;
>  	u32 bufaddr;
> -	unsigned long flags;
>  	unsigned int nr_frags, length;
>  
>  	base = priv->tx_bd_base;
> @@ -1298,14 +1297,11 @@ static int gfar_start_xmit(struct sk_buff *skb, struc
> t net_device *dev)
>  	/* total number of fragments in the SKB */
>  	nr_frags = skb_shinfo(skb)->nr_frags;
>  
> -	spin_lock_irqsave(&priv->txlock, flags);
> -
>  	/* check if there is space to queue this packet */
>  	if ((nr_frags+1) > priv->num_txbdfree) {
>  		/* no space, stop the queue */
>  		netif_stop_queue(dev);
>  		dev->stats.tx_fifo_errors++;
> -		spin_unlock_irqrestore(&priv->txlock, flags);
>  		return NETDEV_TX_BUSY;
>  	}
>  
> @@ -1403,9 +1399,6 @@ static int gfar_start_xmit(struct sk_buff *skb, struct 
> net_device *dev)
>  	/* Tell the DMA to go go go */
>  	gfar_write(&priv->regs->tstat, TSTAT_CLEAR_THALT);
>  
> -	/* Unlock priv */
> -	spin_unlock_irqrestore(&priv->txlock, flags);
> -
>  	return NETDEV_TX_OK;
>  }
>  
> @@ -1915,17 +1908,14 @@ static int gfar_poll(struct napi_struct *napi, int bu
> dget)
>  	struct net_device *dev = priv->ndev;
>  	int tx_cleaned = 0;
>  	int rx_cleaned = 0;
> -	unsigned long flags;
>  
>  	/* Clear IEVENT, so interrupts aren't called again
>  	 * because of the packets that have already arrived */
>  	gfar_write(&priv->regs->ievent, IEVENT_RTX_MASK);
>  
> -	/* If we fail to get the lock, don't bother with the TX BDs */
> -	if (spin_trylock_irqsave(&priv->txlock, flags)) {
> -		tx_cleaned = gfar_clean_tx_ring(dev);
> -		spin_unlock_irqrestore(&priv->txlock, flags);
> -	}
> +	netif_tx_lock_bh(priv->ndev);
> +	tx_cleaned = gfar_clean_tx_ring(dev);
> +	netif_tx_unlock_bh(priv->ndev);
>  
>  	rx_cleaned = gfar_clean_rx_ring(dev, budget);
>  
> 
> -- 
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2

RE: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs

[Kumar Gopalpet-B05799]

=20
>
>Similar but different question:  What version is this patch based upon?
>I can't find:
>
>    > >index 197b358..a0ae604 100644
>
>My search of 2.6.31 up through current linux head=20
>(v2.6.32-rc6-26-g91d3f9b) and benh's current head=20
>(94a8d5caba74211ec76dac80fc6e2d5c391530df) do not have a=20
>version of this code with separate txqueues.
>
>I confess I'm not too familiar with the history here, so I=20
>don't know if that feature is in flux, or came, or went, or what.
>
>That boils down to:  I can't directly apply your patch, but I=20
>could hand-fudge its intent, but only on a single tx queue variant.
>

You might want to get the code base from the top of davem/net-next-2.6
tree

git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git

The multi queue is a feature available in etsec2.0 version. On older
versions you might want to=20
work in a single queue mode itself.


--

Thanks
Sandeep

Re: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs

[David Miller]

From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Thu, 5 Nov 2009 19:57:38 +0300

> But that basically means that with skb recycling we can't safely
> use KGDBoE, though we can add something like this:

Please stop adding special logic only to your driver to handle these
things.

Either it's a non-issue, or it's going to potentially be an issue for
everyone using skb_recycle_check() in a NAPI driver, right?

So why not add the "in_interrupt()" or whatever check to
skb_recycle_check() and if the context is unsuitable return false (0)
ok?

Re: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs

[Anton Vorontsov]

On Sun, Nov 08, 2009 at 01:08:48AM -0800, David Miller wrote:
> From: Anton Vorontsov <avorontsov@ru.mvista.com>
> Date: Thu, 5 Nov 2009 19:57:38 +0300
> 
> > But that basically means that with skb recycling we can't safely
> > use KGDBoE, though we can add something like this:
> 
> Please stop adding special logic only to your driver to handle these
> things.
> 
> Either it's a non-issue, or it's going to potentially be an issue for
> everyone using skb_recycle_check() in a NAPI driver, right?
> 
> So why not add the "in_interrupt()" or whatever check to
> skb_recycle_check() and if the context is unsuitable return false (0)
> ok?

I think the check is needed, yes. But if we add just that check,
then we'll never do skb recycling in the tx path (since gianfar
always grabs irqsave spinlock during tx ring cleanup, so the check
will always return false).

So we must add the check to keep polling with disabled IRQs safe,
but we also should get rid of irqsave spinlock in the gianfar
driver (to make the skb recycling actually work in most cases).

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH RFC] gianfar: Make polling safe with IRQs disabled

[David Miller]

From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Thu, 5 Nov 2009 01:57:11 +0300

> When using KGDBoE, gianfar driver spits 'Interrupt problem' messages,
> which appears to be a legitimate warning, i.e. we may end up calling
> netif_receive_skb() or vlan_hwaccel_receive_skb() with IRQs disabled.
> 
> This patch reworks the RX path so that if netpoll is enabled (the
> only case when the driver don't know from what context the polling
> may be called), we check whether IRQs are disabled, and if so we
> fall back to safe variants of skb receiving functions.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

This is bogus, I'll tell you why.

When you go into netif_receive_skb() we have a special check,
"if (netpoll_receive_skb(..." that takes care of all of the
details concerning doing a ->poll() from IRQ disabled context
via netpoll.

So this code you're adding should not be necessary.

Or, explain to me why no other driver needs special logic in their
->poll() handler like this and does not run into any kinds of netpoll
problems :-)

Re: [PATCH RFC] gianfar: Make polling safe with IRQs disabled

[Anton Vorontsov]

On Sun, Nov 08, 2009 at 01:05:33AM -0800, David Miller wrote:
...
> > When using KGDBoE, gianfar driver spits 'Interrupt problem' messages,
> > which appears to be a legitimate warning, i.e. we may end up calling
> > netif_receive_skb() or vlan_hwaccel_receive_skb() with IRQs disabled.
> > 
> > This patch reworks the RX path so that if netpoll is enabled (the
> > only case when the driver don't know from what context the polling
> > may be called), we check whether IRQs are disabled, and if so we
> > fall back to safe variants of skb receiving functions.
> 
> This is bogus, I'll tell you why.
> 
> When you go into netif_receive_skb() we have a special check,
> "if (netpoll_receive_skb(..." that takes care of all of the
> details concerning doing a ->poll() from IRQ disabled context
> via netpoll.
> 
> So this code you're adding should not be necessary.
> 
> Or, explain to me why no other driver needs special logic in their
> ->poll() handler like this and does not run into any kinds of netpoll
> problems :-)

Hm, I was confused by the following note:

/**
 *      netif_receive_skb - process receive buffer from network
 *      @skb: buffer to process
...
 *      This function may only be called from softirq context and interrupts
 *      should be enabled.


Looking into the code though, I can indeed see that there
are netpoll checks, and __netpoll_rx() is actually called with
irqs disabled. So, in the end it appears that we should just
remove the 'Interrupt problem' message.


Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2