Nicolas Schichan <nschic...@freebox.fr> : [...] > I was trying to minimize the code changes wrt the current source, but if you > want that change to be in the same patch, I can certainly respin a V2 with it.
No need to respin, you elegantly minimized the changes. The body of the while loop could be more spartan, especially with bql in sight. Say: @@ -1041,52 +1041,37 @@ out: mp->work_tx_end &= ~(1 << txq->index); } -static int txq_reclaim(struct tx_queue *txq, int budget, int force) +static void txq_reclaim(struct tx_queue *txq, int force) { struct mv643xx_eth_private *mp = txq_to_mp(txq); - struct netdev_queue *nq = netdev_get_tx_queue(mp->dev, txq->index); - int reclaimed; + struct net_device *dev = mp->dev; + struct netdev_queue *nq = netdev_get_tx_queue(dev, txq->index); + int used = txq->tx_used_desc; + int count; - __netif_tx_lock_bh(nq); + mp->work_tx &= ~(1 << txq->index); - reclaimed = 0; - while (reclaimed < budget && txq->tx_desc_count > 0) { - int tx_index; - struct tx_desc *desc; - u32 cmd_sts; - char desc_dma_map; - - tx_index = txq->tx_used_desc; - desc = &txq->tx_desc_area[tx_index]; - desc_dma_map = txq->tx_desc_mapping[tx_index]; + __netif_tx_lock(nq, smp_processor_id()); - cmd_sts = desc->cmd_sts; + for (count = txq->tx_desc_count; count > 0; count--) { + struct tx_desc *desc = &txq->tx_desc_area[used]; + char desc_dma_map = txq->tx_desc_mapping[used]; - if (cmd_sts & BUFFER_OWNED_BY_DMA) { + if (desc->cmd_sts & BUFFER_OWNED_BY_DMA) { if (!force) break; - desc->cmd_sts = cmd_sts & ~BUFFER_OWNED_BY_DMA; + desc->cmd_sts &= ~BUFFER_OWNED_BY_DMA; } - txq->tx_used_desc = tx_index + 1; - if (txq->tx_used_desc == txq->tx_ring_size) - txq->tx_used_desc = 0; - - reclaimed++; - txq->tx_desc_count--; + used = (used + 1) % txq->tx_ring_size; if (!IS_TSO_HEADER(txq, desc->buf_ptr)) { - if (desc_dma_map == DESC_DMA_MAP_PAGE) - dma_unmap_page(mp->dev->dev.parent, - desc->buf_ptr, - desc->byte_cnt, - DMA_TO_DEVICE); + dma_unmap_page(dev->dev.parent, desc->buf_ptr, + desc->byte_cnt, DMA_TO_DEVICE); else - dma_unmap_single(mp->dev->dev.parent, - desc->buf_ptr, - desc->byte_cnt, - DMA_TO_DEVICE); + dma_unmap_single(dev->dev.parent, desc->buf_ptr, + desc->byte_cnt, DMA_TO_DEVICE); } if (cmd_sts & TX_ENABLE_INTERRUPT) { @@ -1097,18 +1082,15 @@ static int txq_reclaim(struct tx_queue *txq, int budget, int force) } if (cmd_sts & ERROR_SUMMARY) { - netdev_info(mp->dev, "tx error\n"); - mp->dev->stats.tx_errors++; + netdev_info(dev, "tx error\n"); + dev->stats.tx_errors++; } - } - __netif_tx_unlock_bh(nq); - - if (reclaimed < budget) - mp->work_tx &= ~(1 << txq->index); + txq->tx_used_desc = used; + txq->tx_desc_count = count; - return reclaimed; + __netif_tx_unlock(nq); } Nobody uses txq_reclaim return status code. You can turn it void. [...] > > work_tx is also updated in irq context. I'd rather see "clear_flag() then > > reclaim()" than "reclaim() then clear_flag()" in a subsequent patch. > > Just to be sure that I understand the issue here, under normal conditions, > work_tx is updated in irq context via mv643xx_eth_collect_events() and then > the mv643xx interrupts are masked and napi_schedule() is called. Only once all > the work has been completed in the poll callback and the work flags have been > cleared, are the interrupt unmasked and napi_complete() is called. As far as I > can see there should be no issue here. IRQF_SHARED and mv643xx_eth_netpoll make me marginally nervous. My concern was mostly stylistic given the current code. Things get easier - call me lazy - if the bit is cleared before reclaiming. > The only problem I can see is in OOM condition when napi_schedule is called > from a timer callback (oom_timer_wrapper()) which will result in the poll > callback being called with the interrupts unmasked and if an interrupt fires > (possibly in an other CPU) at the wrong time, mv643xx_eth_collect_events() > will race with the various flags clear in txq_reclaim, rxq_process and > rxq_refill ? <knee jerk> You can ignore OOM: the driver should drop received packets immediately instead of poking holes in its receive descriptor ring. </knee jerk> > In that case wouldn't be something like clear_bit/set_bit preferred compared > to the direct bitwise operations ? I don't have any strong feeling about it. Thanks. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/