| Message ID | 20100429231739.509103394@fluff.org.uk |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Ben Dooks <ben@simtec.co.uk> Date: Fri, 30 Apr 2010 00:16:22 +0100 > From: Tristram Ha <Tristram.Ha@micrel.com> > > This fixes a transmit problem of the ks8851 snl network driver. > > Under heavy TCP traffic the device will stop transmitting. Turning off > the transmit interrupt avoids this issue. A new workqueue was > implemented to replace the functionality of the transmit interrupt processing. > > Signed-off-by: Tristram Ha <Tristram.Ha@micrel.com> Please, try to fix this properly. Unless you have a known chip errata with the TX interrupt that cannot be worked around reasonably, which would need to be detailed and explained completely in the commit log message, you should try to figure out what the real problem is. Otherwise just tossing everything to a workqueue looks like a hack, at best. I suspect you have some kind of race between IRQ processing and the ->ndo_start_xmit() handler, so you end up missing a queue wakeup. Either that or you end up misprogramming the hardware due to the race. There is no way I'm applying this as-is. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: Ben Dooks <ben@simtec.co.uk> > Date: Fri, 30 Apr 2010 00:16:22 +0100 > >> From: Tristram Ha <Tristram.Ha@micrel.com> >> >> This fixes a transmit problem of the ks8851 snl network driver. >> >> Under heavy TCP traffic the device will stop transmitting. Turning off >> the transmit interrupt avoids this issue. A new workqueue was >> implemented to replace the functionality of the transmit interrupt processing. >> >> Signed-off-by: Tristram Ha <Tristram.Ha@micrel.com> > > Please, try to fix this properly. Unless you have a known chip errata with the TX interrupt > that cannot be worked around reasonably, which would need to be detailed and explained > completely in the commit log message, you should try to figure out what the real problem is. > > Otherwise just tossing everything to a workqueue looks like a hack, at best. > > I suspect you have some kind of race between IRQ processing and the > ->ndo_start_xmit() handler, so you end up missing a queue wakeup. > Either that or you end up misprogramming the hardware due to the race. > > There is no way I'm applying this as-is. As I explained a long time ago (last year), this patch is no longer considered as a fix but for performance. The transmit done interrupt in the KSZ8851 chips is not required for normal operation. Turning it off actually will improve transmit performance because the system will not be interrupted every time a packet is sent. This driver runs on SPI bus. Just reading register requires a workqueue because it cannot be done under interrupt context. Processing the transmit interrupt requires scheduling a workqueue anyway. I tested the driver under the Beagle Zippy2 board with a 24 MHz SPI bus clock, which limits the throughput to 10 Mbps. On other systems the transmit performance improvement may be greater, but I do not have that data to back me up. If you feel strongly that this workqueue implementation is not appropriate, then please disregard the patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Ha, Tristram" <Tristram.Ha@Micrel.Com> Date: Mon, 3 May 2010 12:06:21 -0700 > The transmit done interrupt in the KSZ8851 chips is not required for > normal operation. Turning it off actually will improve transmit > performance because the system will not be interrupted every time a > packet is sent. But you only trigger this workqueue when you notice in ->ndo_start_xmit() that you're out of space. This makes the chip sit idle with no packets to send until the workqueue executes asynchronously to the initial transmit path which noticed the queue was full. That doesn't make any sense to me. If anything you should at least try to purge the TX queue and make space directly in the ->ndo_start_xmit() handler. And if that fails trigger an hrtimer to poll the TX state. Without some kind of timer based polling mechanism, if the workqueue finds the TX queue is still full, what's going to do more checks later? You will no longer get ->ndo_start_xmit() calls because the queue has been marked full, so nothing will trigger the workqueue to run any more. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
And btw, your commit message should have explained all of the things you're telling me here. Rather than just mention some vague "problem". Your commit messages should explain everything about why you're making the change, not just say what changes are being made. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: "Ha, Tristram" <Tristram.Ha@Micrel.Com> > Date: Mon, 3 May 2010 12:06:21 -0700 > >> The transmit done interrupt in the KSZ8851 chips is not required for >> normal operation. Turning it off actually will improve transmit >> performance because the system will not be interrupted every time a >> packet is sent. > > But you only trigger this workqueue when you notice in ->ndo_start_xmit() that you're out of > space. > > This makes the chip sit idle with no packets to send until the workqueue executes asynchronously > to the initial transmit path which noticed the queue was full. > > That doesn't make any sense to me. If anything you should at least try to purge the TX queue > and make space directly in the ->ndo_start_xmit() handler. And if that fails trigger an hrtimer > to poll the TX state. > > Without some kind of timer based polling mechanism, if the workqueue finds the TX queue is still > full, what's going to do more checks later? You will no longer get ->ndo_start_xmit() calls > because the queue has been marked full, so nothing will trigger the workqueue to run any more. I thought the transmit check workqueue reschedules itself when the buffer available is still not enough, and this is the part you objected. The buffer has about 8K. Suppose 5 1514-byte packets are sent and the buffer is not enough to hold the next packet and so the transmit queue is stopped. A workqueue is scheduled to read the buffer available register. As previous packets should have been sent the buffer size read is big enough and so the transmit queue is restarted. This happens very often as the network bandwidth is only 10 Mbps but the CPU speed is very fast. I should also read the buffer available register during receive interrupt processing so that this situation does not trigger in slow traffic. This roundabout way of using workqueue is because the register cannot be read directly inside ndo_start_xmit(). Actually in other driver with similar transmit buffer available operation I just return NETDEV_TX_BUSY and let the kernel stack calls ndo_start_xmit again and again. Supposedly it is bad for the whole system but the overall transmit throughput is much better than when the transmit done interrupt is enabled. There should be a way to trigger the transmit interrupt after certain number of packets are sent. That will improve the performance and allay your concern. As I am not the engineer who worked on the Micrel KSZ8851 chip during its development, I am not quite aware of its functions and limitations. I will try the new code and submit a better patch if possible. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Ha, Tristram" <Tristram.Ha@Micrel.Com> Date: Mon, 3 May 2010 14:11:41 -0700 > I thought the transmit check workqueue reschedules itself when the > buffer available is still not enough, and this is the part you objected. If it reschedules itself, it runs immediately. That will just hog a cpu endlessly until the TX packets start to be transmitted by the chip. That's just as bad a polling in a loop. You need to use an hrtimer or similar. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: b/drivers/net/ks8851.c =================================================================== --- a/drivers/net/ks8851.c 2010-04-20 18:13:58.000000000 +0100 +++ b/drivers/net/ks8851.c 2010-04-20 18:38:18.000000000 +0100 @@ -111,11 +111,13 @@ struct ks8851_net { struct mii_if_info mii; struct ks8851_rxctrl rxctrl; + struct work_struct tx_check; struct work_struct tx_work; struct work_struct irq_work; struct work_struct rxctrl_work; struct sk_buff_head txq; + int tx_len; struct spi_message spi_msg1; struct spi_message spi_msg2; @@ -573,19 +575,6 @@ static void ks8851_irq_work(struct work_ if (status & IRQ_RXPSI) handled |= IRQ_RXPSI; - if (status & IRQ_TXI) { - handled |= IRQ_TXI; - - /* no lock here, tx queue should have been stopped */ - - /* update our idea of how much tx space is available to the - * system */ - ks->tx_space = ks8851_rdreg16(ks, KS_TXMIR); - - if (netif_msg_intr(ks)) - ks_dbg(ks, "%s: txspace %d\n", __func__, ks->tx_space); - } - if (status & IRQ_RXI) handled |= IRQ_RXI; @@ -623,9 +612,6 @@ static void ks8851_irq_work(struct work_ mutex_unlock(&ks->lock); - if (status & IRQ_TXI) - netif_wake_queue(ks->netdev); - enable_irq(ks->netdev->irq); } @@ -703,6 +689,17 @@ static void ks8851_done_tx(struct ks8851 dev_kfree_skb(txb); } +static void ks8851_tx_check(struct work_struct *work) +{ + struct ks8851_net *ks = container_of(work, struct ks8851_net, tx_check); + + ks->tx_space = ks8851_rdreg16(ks, KS_TXMIR); + if (ks->tx_space > ks->tx_len) + netif_wake_queue(ks->netdev); + else + schedule_work(&ks->tx_check); +} + /** * ks8851_tx_work - process tx packet(s) * @work: The work strucutre what was scheduled. @@ -814,7 +811,6 @@ static int ks8851_net_open(struct net_de /* clear then enable interrupts */ #define STD_IRQ (IRQ_LCI | /* Link Change */ \ - IRQ_TXI | /* TX done */ \ IRQ_RXI | /* RX done */ \ IRQ_SPIBEI | /* SPI bus error */ \ IRQ_TXPSI | /* TX process stop */ \ @@ -830,6 +826,7 @@ static int ks8851_net_open(struct net_de ks_dbg(ks, "network device %s up\n", dev->name); mutex_unlock(&ks->lock); + ks8851_write_mac_addr(dev); return 0; } @@ -854,6 +851,7 @@ static int ks8851_net_stop(struct net_de /* stop any outstanding work */ flush_work(&ks->irq_work); + flush_work(&ks->tx_check); flush_work(&ks->tx_work); flush_work(&ks->rxctrl_work); @@ -912,14 +910,16 @@ static netdev_tx_t ks8851_start_xmit(str if (needed > ks->tx_space) { netif_stop_queue(dev); + ks->tx_len = needed; + schedule_work(&ks->tx_check); ret = NETDEV_TX_BUSY; } else { ks->tx_space -= needed; skb_queue_tail(&ks->txq, skb); + schedule_work(&ks->tx_work); } spin_unlock(&ks->statelock); - schedule_work(&ks->tx_work); return ret; } @@ -1227,6 +1227,7 @@ static int __devinit ks8851_probe(struct mutex_init(&ks->lock); spin_lock_init(&ks->statelock); + INIT_WORK(&ks->tx_check, ks8851_tx_check); INIT_WORK(&ks->tx_work, ks8851_tx_work); INIT_WORK(&ks->irq_work, ks8851_irq_work); INIT_WORK(&ks->rxctrl_work, ks8851_rxctrl_work); @@ -1279,6 +1280,7 @@ static int __devinit ks8851_probe(struct ks8851_read_selftest(ks); ks8851_init_mac(ks); + ks->tx_space = ks8851_rdreg16(ks, KS_TXMIR); ret = request_irq(spi->irq, ks8851_irq, IRQF_TRIGGER_LOW, ndev->name, ks);