: Fix myri10ge NAPI oops & warnings

PATCH In review Andrew Gallatin 2 participants 3 messages Oct 31, 2007

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Andrew Gallatin

author

2007-10-31 21:40 UTC #0 lore raw

PATCH 1/1 : Fix myri10ge NAPI oops & warnings

When testing the myri10ge driver with 2.6.24-rc1, I found that the machine crashed under heavy load:

Unable to handle kernel paging request at 0000000000100108 RIP:
 [<ffffffff803cc8dd>] net_rx_action+0x11b/0x184

The address corresponds to the list_move_tail() in netif_rx_complete():

                    if (unlikely(work == weight))
                            list_move_tail(&n->poll_list, list);

Eventually, I traced the crashes to calling netif_rx_complete() with work_done == budget. From looking at other drivers, it appears that one should only call netif_rx_complete() when work_done < budget.

To fix it, I changed the test in myri10ge_poll() so that it refers to to work_done rather than looking at the rx ring status. If work_done is < budget, then that implies we have no more packets to process. Any races will be resolved by the NIC when the write to irq_claim is made.

In myri10ge_clean_rx_done(), if we ever exceeded our budget, it would report a work_done one larger than was acutally done. This is because the increment was done in the conditional, so work_done would be incremented regardless of whether or not the test passed or failed. This would lead to the WARN_ON_ONCE(work > weight); warning in net_rx_action triggering. I've moved the increment of work_done inside the loop. Note that this would only be a problem when we had exceeded our budget.

Signed off by: Andrew Gallatin <[email protected]>

Andrew Gallatin Myricom Inc

drivers/net/myri10ge/myri10ge.c +3 -3

index 366e62a..0f306dd 100644
@@ -1151,7 +1151,7 @@ static inline int myri10ge_clean_rx_done
 	u16 length;
 	__wsum checksum;
 
-	while (rx_done->entry[idx].length != 0 && work_done++ < budget) {
+	while (rx_done->entry[idx].length != 0 && work_done < budget) {
 		length = ntohs(rx_done->entry[idx].length);
 		rx_done->entry[idx].length = 0;
 		checksum = csum_unfold(rx_done->entry[idx].checksum);

@@ -1167,6 +1167,7 @@ static inline int myri10ge_clean_rx_done
 		rx_bytes += rx_ok * (unsigned long)length;
 		cnt++;
 		idx = cnt & (myri10ge_max_intr_slots - 1);
+		work_done++;
 	}
 	rx_done->idx = idx;
 	rx_done->cnt = cnt;

@@ -1233,13 +1234,12 @@ static int myri10ge_poll(struct napi_str
 	struct myri10ge_priv *mgp =
 	    container_of(napi, struct myri10ge_priv, napi);
 	struct net_device *netdev = mgp->dev;
-	struct myri10ge_rx_done *rx_done = &mgp->rx_done;
 	int work_done;
 
 	/* process as many rx events as NAPI will allow */
 	work_done = myri10ge_clean_rx_done(mgp, budget);
 
-	if (rx_done->entry[rx_done->idx].length == 0 || !netif_running(netdev)) {
+	if (work_done < budget || !netif_running(netdev)) {
 		netif_rx_complete(netdev, napi);
 		put_be32(htonl(3), mgp->irq_claim);
 	}

Stephen Hemminger

2007-10-31 21:45 UTC #1 lore raw

Quoted 31 lines · On Wed, 31 Oct 2007 17:40:06 -0400 Andrew Gallatin <[email protected]> wrote:

When testing the myri10ge driver with 2.6.24-rc1, I found that the machine crashed under heavy load:
Unable to handle kernel paging request at 0000000000100108 RIP:
  [<ffffffff803cc8dd>] net_rx_action+0x11b/0x184
The address corresponds to the list_move_tail() in netif_rx_complete():
                     if (unlikely(work == weight))
                             list_move_tail(&n->poll_list, list);
Eventually, I traced the crashes to calling netif_rx_complete() with work_done == budget. From looking at other drivers, it appears that one should only call netif_rx_complete() when work_done < budget.
To fix it, I changed the test in myri10ge_poll() so that it refers to to work_done rather than looking at the rx ring status. If work_done is < budget, then that implies we have no more packets to process. Any races will be resolved by the NIC when the write to irq_claim is made.
In myri10ge_clean_rx_done(), if we ever exceeded our budget, it would report a work_done one larger than was acutally done. This is because the increment was done in the conditional, so work_done would be incremented regardless of whether or not the test passed or failed. This would lead to the WARN_ON_ONCE(work > weight); warning in net_rx_action triggering. I've moved the increment of work_done inside the loop. Note that this would only be a problem when we had exceeded our budget.

Quoted 5 lines

Signed off by: Andrew Gallatin <[email protected]>
Andrew Gallatin Myricom Inc

Yes, this looks right.
How could the check in netif_rx_complete be changed to catch this better?

Signature · 1 line

Stephen Hemminger <[email protected]>

Andrew Gallatin

author

2007-10-31 22:54 UTC #2 lore raw

Quoted 33 lines · Stephen Hemminger

Quoted 30 lines · On Wed, 31 Oct 2007 17:40:06 -0400 Andrew Gallatin <[email protected]> wrote:
When testing the myri10ge driver with 2.6.24-rc1, I found that the machine crashed under heavy load:
Unable to handle kernel paging request at 0000000000100108 RIP:
  [<ffffffff803cc8dd>] net_rx_action+0x11b/0x184
The address corresponds to the list_move_tail() in netif_rx_complete():
                     if (unlikely(work == weight))
                             list_move_tail(&n->poll_list, list);
Eventually, I traced the crashes to calling netif_rx_complete() with work_done == budget. From looking at other drivers, it appears that one should only call netif_rx_complete() when work_done < budget.
To fix it, I changed the test in myri10ge_poll() so that it refers to to work_done rather than looking at the rx ring status. If work_done is < budget, then that implies we have no more packets to process. Any races will be resolved by the NIC when the write to irq_claim is made.
In myri10ge_clean_rx_done(), if we ever exceeded our budget, it would report a work_done one larger than was acutally done. This is because the increment was done in the conditional, so work_done would be incremented regardless of whether or not the test passed or failed. This would lead to the WARN_ON_ONCE(work > weight); warning in net_rx_action triggering. I've moved the increment of work_done inside the loop. Note that this would only be a problem when we had exceeded our budget.

Quoted 8 lines

Quoted 5 lines
Signed off by: Andrew Gallatin <[email protected]>
Andrew Gallatin Myricom Inc
Yes, this looks right.
How could the check in netif_rx_complete be changed to catch this better?

I'm sorry, but I don't really know. I'm afraid that I am rather clueless about the new NAPI, and found this by stumbling around in the dark. Naively, it seems like some global option to override all napi weights (eg, to 1) would be helpful so that if a driver had this problem, it could be easily reproduced by the first packet received. At least I found setting our weight to one made the bug rather obvious to me.

Drew