[PATCH v1] mv643xx_eth: only account for work done in rxq_process in poll callback.

[PATCH v1] mv643xx_eth: only account for work done in rxq_process in poll callback.

[Nicolas Schichan]

Link events, txq_reclaim() and rxq_refill() work are no longer
accounted against budget. As a result, also change txq_reclaim() and
rxq_refill() not to take a budget parameter anymore.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 1c75829..52bc56b 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -610,13 +610,13 @@ err:
 	return rx;
 }
 
-static int rxq_refill(struct rx_queue *rxq, int budget)
+static int rxq_refill(struct rx_queue *rxq)
 {
 	struct mv643xx_eth_private *mp = rxq_to_mp(rxq);
 	int refilled;
 
 	refilled = 0;
-	while (refilled < budget && rxq->rx_desc_count < rxq->rx_ring_size) {
+	while (rxq->rx_desc_count < rxq->rx_ring_size) {
 		struct sk_buff *skb;
 		int rx;
 		struct rx_desc *rx_desc;
@@ -659,8 +659,7 @@ static int rxq_refill(struct rx_queue *rxq, int budget)
 		skb_reserve(skb, 2);
 	}
 
-	if (refilled < budget)
-		mp->work_rx_refill &= ~(1 << rxq->index);
+	mp->work_rx_refill &= ~(1 << rxq->index);
 
 oom:
 	return refilled;
@@ -1041,7 +1040,7 @@ out:
 	mp->work_tx_end &= ~(1 << txq->index);
 }
 
-static int txq_reclaim(struct tx_queue *txq, int budget, int force)
+static int 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);
@@ -1050,7 +1049,7 @@ static int txq_reclaim(struct tx_queue *txq, int budget, int force)
 	__netif_tx_lock_bh(nq);
 
 	reclaimed = 0;
-	while (reclaimed < budget && txq->tx_desc_count > 0) {
+	while (txq->tx_desc_count > 0) {
 		int tx_index;
 		struct tx_desc *desc;
 		u32 cmd_sts;
@@ -1105,8 +1104,7 @@ static int txq_reclaim(struct tx_queue *txq, int budget, int force)
 
 	__netif_tx_unlock_bh(nq);
 
-	if (reclaimed < budget)
-		mp->work_tx &= ~(1 << txq->index);
+	mp->work_tx &= ~(1 << txq->index);
 
 	return reclaimed;
 }
@@ -2104,7 +2102,7 @@ static void txq_deinit(struct tx_queue *txq)
 	struct mv643xx_eth_private *mp = txq_to_mp(txq);
 
 	txq_disable(txq);
-	txq_reclaim(txq, txq->tx_ring_size, 1);
+	txq_reclaim(txq, 1);
 
 	BUG_ON(txq->tx_used_desc != txq->tx_curr_desc);
 
@@ -2191,7 +2189,7 @@ static void handle_link_event(struct mv643xx_eth_private *mp)
 			for (i = 0; i < mp->txq_count; i++) {
 				struct tx_queue *txq = mp->txq + i;
 
-				txq_reclaim(txq, txq->tx_ring_size, 1);
+				txq_reclaim(txq, 1);
 				txq_reset_hw_ptr(txq);
 			}
 		}
@@ -2243,7 +2241,6 @@ static int mv643xx_eth_poll(struct napi_struct *napi, int budget)
 		if (mp->work_link) {
 			mp->work_link = 0;
 			handle_link_event(mp);
-			work_done++;
 			continue;
 		}
 
@@ -2267,12 +2264,12 @@ static int mv643xx_eth_poll(struct napi_struct *napi, int budget)
 		if (mp->work_tx_end & queue_mask) {
 			txq_kick(mp->txq + queue);
 		} else if (mp->work_tx & queue_mask) {
-			work_done += txq_reclaim(mp->txq + queue, work_tbd, 0);
+			txq_reclaim(mp->txq + queue, 0);
 			txq_maybe_wake(mp->txq + queue);
 		} else if (mp->work_rx & queue_mask) {
 			work_done += rxq_process(mp->rxq + queue, work_tbd);
 		} else if (!mp->oom && (mp->work_rx_refill & queue_mask)) {
-			work_done += rxq_refill(mp->rxq + queue, work_tbd);
+			rxq_refill(mp->rxq + queue);
 		} else {
 			BUG();
 		}
@@ -2428,7 +2425,7 @@ static int mv643xx_eth_open(struct net_device *dev)
 			goto out;
 		}
 
-		rxq_refill(mp->rxq + i, INT_MAX);
+		rxq_refill(mp->rxq + i);
 		mp->int_mask |= INT_RX_0 << i;
 	}
 
-- 
1.9.1

Re: [PATCH v1] mv643xx_eth: only account for work done in rxq_process in poll callback.

[Francois Romieu]

Nicolas Schichan <nschichan@freebox.fr> :
[...]
> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
> index 1c75829..52bc56b 100644
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
[...]
> @@ -1050,7 +1049,7 @@ static int txq_reclaim(struct tx_queue *txq, int budget, int force)
>  	__netif_tx_lock_bh(nq);
>  
>  	reclaimed = 0;
> -	while (reclaimed < budget && txq->tx_desc_count > 0) {
> +	while (txq->tx_desc_count > 0) {
>  		int tx_index;
>  		struct tx_desc *desc;
>  		u32 cmd_sts;

You may use a local 'int count = txq->tx_desc_count' variable then
perform a single update at the end of the locked section. 
txq->tx_used_desc could be reworked in a similar way.

> @@ -1105,8 +1104,7 @@ static int txq_reclaim(struct tx_queue *txq, int budget, int force)
>  
>  	__netif_tx_unlock_bh(nq);
>  
> -	if (reclaimed < budget)
> -		mp->work_tx &= ~(1 << txq->index);
> +	mp->work_tx &= ~(1 << txq->index);
>  
>  	return reclaimed;
>  }

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.

-- 
Ueimor

Re: [PATCH v1] mv643xx_eth: only account for work done in rxq_process in poll callback.

[Nicolas Schichan]

On 03/04/2015 10:32 PM, Francois Romieu wrote:
> Nicolas Schichan <nschichan@freebox.fr> :
>> @@ -1050,7 +1049,7 @@ static int txq_reclaim(struct tx_queue *txq, int budget, int force)
>>  	__netif_tx_lock_bh(nq);
>>  
>>  	reclaimed = 0;
>> -	while (reclaimed < budget && txq->tx_desc_count > 0) {
>> +	while (txq->tx_desc_count > 0) {
>>  		int tx_index;
>>  		struct tx_desc *desc;
>>  		u32 cmd_sts;
> 
> You may use a local 'int count = txq->tx_desc_count' variable then
> perform a single update at the end of the locked section. 
> txq->tx_used_desc could be reworked in a similar way.

Hello Francois,

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.

>> @@ -1105,8 +1104,7 @@ static int txq_reclaim(struct tx_queue *txq, int budget, int force)
>>  
>>  	__netif_tx_unlock_bh(nq);
>>  
>> -	if (reclaimed < budget)
>> -		mp->work_tx &= ~(1 << txq->index);
>> +	mp->work_tx &= ~(1 << txq->index);
>>  
>>  	return reclaimed;
>>  }
> 
> 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.

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 ?

In that case wouldn't be something like clear_bit/set_bit preferred compared
to the direct bitwise operations ?

Regards,


-- 
Nicolas Schichan
Freebox SAS

Re: [PATCH v1] mv643xx_eth: only account for work done in rxq_process in poll callback.

[Francois Romieu]

Nicolas Schichan <nschichan@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