[PATCH] net: mvneta: fix handling of the Tx descriptor counter

[PATCH] net: mvneta: fix handling of the Tx descriptor counter

[Simon Guinot]

The mvneta controller provides a 8-bit register to update the pending
Tx descriptor counter. Then, a maximum of 255 Tx descriptors can be
added at once. In the current code the mvneta_txq_pend_desc_add function
assumes the caller takes care of this limit. But it is not the case. In
some situations (xmit_more flag), more than 255 descriptors are added.
When this happens, the Tx descriptor counter register is updated with a
wrong value, which breaks the whole Tx queue management.

This patch fixes the issue by allowing the mvneta_txq_pend_desc_add
function to process more than 255 Tx descriptors.

Fixes: 2a90f7e1d5d0 ("net: mvneta: add xmit_more support")
Cc: stable@vger.kernel.org # 4.11+
Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
---
 drivers/net/ethernet/marvell/mvneta.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 64a04975bcf8..027c08ce4e5d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -816,11 +816,14 @@ static void mvneta_txq_pend_desc_add(struct mvneta_port *pp,
 {
 	u32 val;
 
-	/* Only 255 descriptors can be added at once ; Assume caller
-	 * process TX desriptors in quanta less than 256
-	 */
-	val = pend_desc + txq->pending;
-	mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
+	pend_desc += txq->pending;
+
+	/* Only 255 Tx descriptors can be added at once */
+	while (pend_desc > 0) {
+		val = min(pend_desc, 255);
+		mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
+		pend_desc -= val;
+	}
 	txq->pending = 0;
 }
 
@@ -2413,8 +2416,7 @@ static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
 		if (txq->count >= txq->tx_stop_threshold)
 			netif_tx_stop_queue(nq);
 
-		if (!skb->xmit_more || netif_xmit_stopped(nq) ||
-		    txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK)
+		if (!skb->xmit_more || netif_xmit_stopped(nq))
 			mvneta_txq_pend_desc_add(pp, txq, frags);
 		else
 			txq->pending += frags;
-- 
2.9.3

RE: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[David Laight]

From: Simon Guinot
> Sent: 08 November 2017 16:59
> 
> The mvneta controller provides a 8-bit register to update the pending
> Tx descriptor counter. Then, a maximum of 255 Tx descriptors can be
> added at once. In the current code the mvneta_txq_pend_desc_add function
> assumes the caller takes care of this limit. But it is not the case. In
> some situations (xmit_more flag), more than 255 descriptors are added.
> When this happens, the Tx descriptor counter register is updated with a
> wrong value, which breaks the whole Tx queue management.

Even with xmit_more it is usually best to limit the number
(in order to reduce tx latency).

OTOH there are probably paths where it 'just happens'.

> This patch fixes the issue by allowing the mvneta_txq_pend_desc_add
> function to process more than 255 Tx descriptors.
> 
> Fixes: 2a90f7e1d5d0 ("net: mvneta: add xmit_more support")
> Cc: stable@vger.kernel.org # 4.11+
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 64a04975bcf8..027c08ce4e5d 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -816,11 +816,14 @@ static void mvneta_txq_pend_desc_add(struct mvneta_port *pp,
>  {
>  	u32 val;
> 
> -	/* Only 255 descriptors can be added at once ; Assume caller
> -	 * process TX desriptors in quanta less than 256
> -	 */
> -	val = pend_desc + txq->pending;
> -	mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
> +	pend_desc += txq->pending;
> +
> +	/* Only 255 Tx descriptors can be added at once */
> +	while (pend_desc > 0) {
> +		val = min(pend_desc, 255);
> +		mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
> +		pend_desc -= val;
> +	}

do { ... } while () ??

	David

Re: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]

Hi Sven and Andreas,

Please, can you try with this patch ?

Thanks.

Simon

On Wed, Nov 08, 2017 at 05:58:35PM +0100, Simon Guinot wrote:
> The mvneta controller provides a 8-bit register to update the pending
> Tx descriptor counter. Then, a maximum of 255 Tx descriptors can be
> added at once. In the current code the mvneta_txq_pend_desc_add function
> assumes the caller takes care of this limit. But it is not the case. In
> some situations (xmit_more flag), more than 255 descriptors are added.
> When this happens, the Tx descriptor counter register is updated with a
> wrong value, which breaks the whole Tx queue management.
> 
> This patch fixes the issue by allowing the mvneta_txq_pend_desc_add
> function to process more than 255 Tx descriptors.
> 
> Fixes: 2a90f7e1d5d0 ("net: mvneta: add xmit_more support")
> Cc: stable@vger.kernel.org # 4.11+
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 64a04975bcf8..027c08ce4e5d 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -816,11 +816,14 @@ static void mvneta_txq_pend_desc_add(struct mvneta_port *pp,
>  {
>  	u32 val;
>  
> -	/* Only 255 descriptors can be added at once ; Assume caller
> -	 * process TX desriptors in quanta less than 256
> -	 */
> -	val = pend_desc + txq->pending;
> -	mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
> +	pend_desc += txq->pending;
> +
> +	/* Only 255 Tx descriptors can be added at once */
> +	while (pend_desc > 0) {
> +		val = min(pend_desc, 255);
> +		mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
> +		pend_desc -= val;
> +	}
>  	txq->pending = 0;
>  }
>  
> @@ -2413,8 +2416,7 @@ static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
>  		if (txq->count >= txq->tx_stop_threshold)
>  			netif_tx_stop_queue(nq);
>  
> -		if (!skb->xmit_more || netif_xmit_stopped(nq) ||
> -		    txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK)
> +		if (!skb->xmit_more || netif_xmit_stopped(nq))
>  			mvneta_txq_pend_desc_add(pp, txq, frags);
>  		else
>  			txq->pending += frags;
> -- 
> 2.9.3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[Andreas Tobler]

Hi Simon,


On 08.11.17 18:17, Simon Guinot wrote:
> Hi Sven and Andreas,
> 
> Please, can you try with this patch ?

Today we, my son and I, repeated the failing scenario and we were able 
to show that our scenario behaves stable after you patch being applied.

Thanks for taking care of this issue. If you need further testing let me 
know.

Regards,
Andreas

> On Wed, Nov 08, 2017 at 05:58:35PM +0100, Simon Guinot wrote:
>> The mvneta controller provides a 8-bit register to update the pending
>> Tx descriptor counter. Then, a maximum of 255 Tx descriptors can be
>> added at once. In the current code the mvneta_txq_pend_desc_add function
>> assumes the caller takes care of this limit. But it is not the case. In
>> some situations (xmit_more flag), more than 255 descriptors are added.
>> When this happens, the Tx descriptor counter register is updated with a
>> wrong value, which breaks the whole Tx queue management.
>>
>> This patch fixes the issue by allowing the mvneta_txq_pend_desc_add
>> function to process more than 255 Tx descriptors.
>>
>> Fixes: 2a90f7e1d5d0 ("net: mvneta: add xmit_more support")
>> Cc: stable@vger.kernel.org # 4.11+
>> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
>> ---
>>   drivers/net/ethernet/marvell/mvneta.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
>> index 64a04975bcf8..027c08ce4e5d 100644
>> --- a/drivers/net/ethernet/marvell/mvneta.c
>> +++ b/drivers/net/ethernet/marvell/mvneta.c
>> @@ -816,11 +816,14 @@ static void mvneta_txq_pend_desc_add(struct mvneta_port *pp,
>>   {
>>   	u32 val;
>>   
>> -	/* Only 255 descriptors can be added at once ; Assume caller
>> -	 * process TX desriptors in quanta less than 256
>> -	 */
>> -	val = pend_desc + txq->pending;
>> -	mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
>> +	pend_desc += txq->pending;
>> +
>> +	/* Only 255 Tx descriptors can be added at once */
>> +	while (pend_desc > 0) {
>> +		val = min(pend_desc, 255);
>> +		mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
>> +		pend_desc -= val;
>> +	}
>>   	txq->pending = 0;
>>   }
>>   
>> @@ -2413,8 +2416,7 @@ static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
>>   		if (txq->count >= txq->tx_stop_threshold)
>>   			netif_tx_stop_queue(nq);
>>   
>> -		if (!skb->xmit_more || netif_xmit_stopped(nq) ||
>> -		    txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK)
>> +		if (!skb->xmit_more || netif_xmit_stopped(nq))
>>   			mvneta_txq_pend_desc_add(pp, txq, frags);
>>   		else
>>   			txq->pending += frags;
>> -- 
>> 2.9.3

Re: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[David Miller]

From: Simon Guinot <simon.guinot@sequanux.org>
Date: Wed,  8 Nov 2017 17:58:35 +0100

> @@ -2413,8 +2416,7 @@ static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
>  		if (txq->count >= txq->tx_stop_threshold)
>  			netif_tx_stop_queue(nq);
>  
> -		if (!skb->xmit_more || netif_xmit_stopped(nq) ||
> -		    txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK)
> +		if (!skb->xmit_more || netif_xmit_stopped(nq))
>  			mvneta_txq_pend_desc_add(pp, txq, frags);
>  		else
>  			txq->pending += frags;

As David Laight said, you should not allow unlimited amounts of
->xmit_more frames to be queued without a TX doorbell update.

Therefore, please keep some kind of limit here otherwise latency
will spike in some circumstances.

Re: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Sat, Nov 11, 2017 at 06:45:04PM +0900, David Miller wrote:
> From: Simon Guinot <simon.guinot@sequanux.org>
> Date: Wed,  8 Nov 2017 17:58:35 +0100
> 
> > @@ -2413,8 +2416,7 @@ static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
> >  		if (txq->count >= txq->tx_stop_threshold)
> >  			netif_tx_stop_queue(nq);
> >  
> > -		if (!skb->xmit_more || netif_xmit_stopped(nq) ||
> > -		    txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK)
> > +		if (!skb->xmit_more || netif_xmit_stopped(nq))
> >  			mvneta_txq_pend_desc_add(pp, txq, frags);
> >  		else
> >  			txq->pending += frags;
> 
> As David Laight said, you should not allow unlimited amounts of
> ->xmit_more frames to be queued without a TX doorbell update.
> 
> Therefore, please keep some kind of limit here otherwise latency
> will spike in some circumstances.

Hi David,

IIUC the driver stops the queue if a threshold of 316 Tx descriptors is
reached (default and worst value).

Is that a "good enough" kind of limit ? Or do you want me to keep the
condition above ?

Simon

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[David Miller]

From: Simon Guinot <simon.guinot@sequanux.org>
Date: Mon, 13 Nov 2017 15:51:15 +0100

> IIUC the driver stops the queue if a threshold of 316 Tx descriptors is
> reached (default and worst value).

That's a lot of latency.

[PATCH v2] net: mvneta: fix handling of the Tx descriptor counter

[Simon Guinot]

The mvneta controller provides a 8-bit register to update the pending
Tx descriptor counter. Then, a maximum of 255 Tx descriptors can be
added at once. In the current code the mvneta_txq_pend_desc_add function
assumes the caller takes care of this limit. But it is not the case. In
some situations (xmit_more flag), more than 255 descriptors are added.
When this happens, the Tx descriptor counter register is updated with a
wrong value, which breaks the whole Tx queue management.

This patch fixes the issue by allowing the mvneta_txq_pend_desc_add
function to process more than 255 Tx descriptors.

Fixes: 2a90f7e1d5d0 ("net: mvneta: add xmit_more support")
Cc: stable@vger.kernel.org # 4.11+
Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
---
 drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Changes for v2:

- Use do {} while instead of while {}.
- Keep "txq->pending + frags > MVNETA_TXQ_DEC_SENT_MASK" as a flushing
  condition for the Tx queue.

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 64a04975bcf8..bc93b69cfd1e 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -816,11 +816,14 @@ static void mvneta_txq_pend_desc_add(struct mvneta_port *pp,
 {
 	u32 val;
 
-	/* Only 255 descriptors can be added at once ; Assume caller
-	 * process TX desriptors in quanta less than 256
-	 */
-	val = pend_desc + txq->pending;
-	mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
+	pend_desc += txq->pending;
+
+	/* Only 255 Tx descriptors can be added at once */
+	do {
+		val = min(pend_desc, 255);
+		mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
+		pend_desc -= val;
+	} while (pend_desc > 0);
 	txq->pending = 0;
 }
 
-- 
2.15.0

Re: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On Mon, Nov 13, 2017 at 11:54:14PM +0900, David Miller wrote:
> From: Simon Guinot <simon.guinot@sequanux.org>
> Date: Mon, 13 Nov 2017 15:51:15 +0100
> 
> > IIUC the driver stops the queue if a threshold of 316 Tx descriptors is
> > reached (default and worst value).
> 
> That's a lot of latency.

OK, then I'll keep the "tx_pending > 255" flushing condition. But note
there is no other software mechanism to limit the Tx latency inside the
mvneta driver. Should we add something ? And is that not rather the job
of the network stack to keep track of the latency and to limit the txq
size ?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2] net: mvneta: fix handling of the Tx descriptor counter

[David Miller]

From: Simon Guinot <simon.guinot@sequanux.org>
Date: Mon, 13 Nov 2017 16:27:02 +0100

> The mvneta controller provides a 8-bit register to update the pending
> Tx descriptor counter. Then, a maximum of 255 Tx descriptors can be
> added at once. In the current code the mvneta_txq_pend_desc_add function
> assumes the caller takes care of this limit. But it is not the case. In
> some situations (xmit_more flag), more than 255 descriptors are added.
> When this happens, the Tx descriptor counter register is updated with a
> wrong value, which breaks the whole Tx queue management.
> 
> This patch fixes the issue by allowing the mvneta_txq_pend_desc_add
> function to process more than 255 Tx descriptors.
> 
> Fixes: 2a90f7e1d5d0 ("net: mvneta: add xmit_more support")
> Cc: stable@vger.kernel.org # 4.11+
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>

Applied.

RE: [PATCH] net: mvneta: fix handling of the Tx descriptor counter

[David Laight]

From: Simon Guinot
> Sent: 13 November 2017 15:36
> To: David Miller
> Cc: thomas.petazzoni@free-electrons.com; netdev@vger.kernel.org; musv@gmx.de;
> andreas.tobler@cloudguard.ch; gregory.clement@free-electrons.com; antoine.tenart@free-electrons.com;
> mw@semihalf.com; stable@vger.kernel.org
> Subject: Re: [PATCH] net: mvneta: fix handling of the Tx descriptor counter
> 
> On Mon, Nov 13, 2017 at 11:54:14PM +0900, David Miller wrote:
> > From: Simon Guinot <simon.guinot@sequanux.org>
> > Date: Mon, 13 Nov 2017 15:51:15 +0100
> >
> > > IIUC the driver stops the queue if a threshold of 316 Tx descriptors is
> > > reached (default and worst value).
> >
> > That's a lot of latency.
> 
> OK, then I'll keep the "tx_pending > 255" flushing condition. But note
> there is no other software mechanism to limit the Tx latency inside the
> mvneta driver. Should we add something ? And is that not rather the job
> of the network stack to keep track of the latency and to limit the txq
> size ?

This is 'first packet transmit latency'.

If the 'doorbell write' is just a PCIe write then, on most systems,
that is cheap and pipelined/posted.
I'd almost be surprised if you see any 'improvement' from not doing
it every packet.

The overall tx queue size is a different issue - usually needs
limiting by BQL if TSO is done.

	David