From: Stephen Irons [mailto:[email protected]]
Sent: Thursday, April 16, 2009 10:37 AM
This is my preferred solution -- the appended patch is different from
the previous one.
1. I believe that emac_rx_bdproc() and emac_tx_bodproc() only set the
pending flag if there really is more work to do. These functions must
include the test (pkt_processeds == budget) to decide how the while()
loop ended. I think that this is correct as implemented, quite
independently of the test (num_rxpkts==budget) in emac_poll(), though
I
do not understand the implications of ((frame_status &
EMAC_CPPI_OWNERSHIP_BIT) == 0))
That translated to English means: Set pending to true if we have consumed our
entire budget and there is still a frame in the queue and the DMA on that frame
is complete.
IOW, the pending flag currently makes sure work is *really* pending.
2. in emac_poll(), we want to stop polling and re-enable interrupts if
there is more work to do. However, net_rx_action() imposes another
constraint: we must not play around with the NAPI list if
(work==budget). So in this special case, we cannot stop polling.
Yes. So work < budget should be the only condition we are actually interested
in.
3. In the case where we have used up the entire budget and there is
now
no more work to do, we are forced to do another poll by the constraint
imposed by net_rx_action(). But next time around, we should use less
than the budget (unless there has been a sudden flurry of packets). We
can live with the extra poll.
Yes. And this makes me think we can relax the pending condition in
emac_rx_bdproc().
4. In the case where we want to continue polling, emac_poll() does
nothing, the same as e100 device. net_rx_action() will keep calling
the
poll() routine until we explicitly call netif_rx_complete().
Yes. The __netif_rx_schedule() was superfluous.
5. We never want to return the number of tx packets processed. At
present, the Tx budget is less than the Rx budget. But if this were to
change in the future, and we returned the number of tx packets, then
it
is possible that we could return a value bigger than the Rx budget.
This
would earn us a WARN_ON_ONCE warning from net_rx_action() and taint
the
kernel. Therefore, keep num_txpkts separate from num_rxpkts, and
she'll
be right.
Yes, Agreed.
6. I am currently running this code on a busy corporate network with
an
additional change (not shown in this patch) that sensitizes the system
to the error -- reduce EMAC_POLL_WEIGHT, EMAC_DEF_TX_MAX_SERVICE and
EMAC_DEF_RX_MAX_SERVICE to 4. When sensitized, and without the
attached
patch, the kernel panic occurs soon after then EMAC device is
initialised. When sensitized, and with the patch, the kernel has been
running for 24 hours and more.
Yes, surely you have identified a corner case.
Signed-off-by: Stephen Irons <[email protected]>
---
drivers/net/davinci_emac.c | 18 ++++++++----------
1 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index 89ff48b..a5354b0 100755
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -2157,7 +2157,8 @@ static int emac_poll(struct napi_struct *napi,
int
budget)
struct emac_priv *priv = container_of(napi, struct emac_priv,
napi);
struct net_device *ndev = priv->ndev;
u32 status = 0;
- u32 num_pkts = 0;
+ u32 num_txpkts = 0;
+ u32 num_rxpkts = 0;
u32 txpending = 0;
u32 rxpending = 0;
@@ -2173,7 +2174,7 @@ static int emac_poll(struct napi_struct *napi,
int
budget)
mask = EMAC_DM646X_MAC_IN_VECTOR_TX_INT_VEC;
if (status & mask) {
- num_pkts = emac_tx_bdproc(priv, EMAC_DEF_TX_CH,
+ num_txpkts = emac_tx_bdproc(priv, EMAC_DEF_TX_CH,
EMAC_DEF_TX_MAX_SERVICE,
&txpending);
} /* TX processing */
@@ -2184,16 +2185,13 @@ static int emac_poll(struct napi_struct *napi,
int budget)
mask = EMAC_DM646X_MAC_IN_VECTOR_RX_INT_VEC;
if (status & mask) {
- num_pkts = emac_rx_bdproc(priv, EMAC_DEF_RX_CH,
+ num_rxpkts = emac_rx_bdproc(priv, EMAC_DEF_RX_CH,
budget, &rxpending);
} /* RX processing */
- if (txpending || rxpending) {
- if (likely(netif_rx_schedule_prep(&priv->napi))) {
- emac_int_disable(priv);
- __netif_rx_schedule(&priv->napi);
- }
- } else {
+ /* If there is nothing to transmit and nothing to receive, and we
have not consumed the entire budget,
+ then stop polling and re-enable interrupts */
+ if (!txpending && !rxpending && (num_rxpkts != budget)) {
I think just checking for (num_rxpkts < budget) should be sufficient instead of
!rxpending && (num_rxpkts != budget). This is what is being done in other drivers
using NAPI (like 8139too or e100).
Thanks,
Sekhar
