[PATCH] net: forcedeth: add xmit_more support

[PATCH] net: forcedeth: add xmit_more support

[Zhu Yanjun]

This change adds support for xmit_more based on the igb commit 6f19e12f6230
("igb: flush when in xmit_more mode and under descriptor pressure") and
commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data") that
were made to igb to support this feature. The function netif_xmit_stopped
is called to check if transmit queue on device is currently unable to send
to determine if we must write the tail because we can add no further
buffers.
When normal packets and/or xmit_more packets fill up tx_desc, it is
necessary to trigger NIC tx reg.

Tested:
  - pktgen (xmit_more packets) SMP x86_64 ->
    Test command:
    ./pktgen_sample03_burst_single_flow.sh ... -b 8 -n 1000000
    Test results:
    Params:
    ...
    burst: 8
    ...
    Result: OK: 12194004(c12188996+d5007) usec, 1000001 (1500byte,0frags)
    82007pps 984Mb/sec (984084000bps) errors: 0

  - iperf (normal packets) SMP x86_64 ->
    Test command:
    Server: iperf -s
    Client: iperf -c serverip
    Result:
    TCP window size: 85.0 KByte (default)
    ------------------------------------------------------------
    [ ID] Interval       Transfer     Bandwidth
    [  3]  0.0-10.0 sec  1.10 GBytes   942 Mbits/sec

CC: Joe Jin <joe.jin@oracle.com>
CC: JUNXIAO_BI <junxiao.bi@oracle.com>
Reported-and-tested-by: Nan san <nan.1986san@gmail.com>
Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
---
 drivers/net/ethernet/nvidia/forcedeth.c | 37 +++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 05d2b47..7417bac 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -2225,6 +2225,7 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct nv_skb_map *prev_tx_ctx;
 	struct nv_skb_map *tmp_tx_ctx = NULL, *start_tx_ctx = NULL;
 	unsigned long flags;
+	netdev_tx_t ret = NETDEV_TX_OK;
 
 	/* add fragments to entries count */
 	for (i = 0; i < fragments; i++) {
@@ -2240,7 +2241,12 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		netif_stop_queue(dev);
 		np->tx_stop = 1;
 		spin_unlock_irqrestore(&np->lock, flags);
-		return NETDEV_TX_BUSY;
+
+		/* When normal packets and/or xmit_more packets fill up
+		 * tx_desc, it is necessary to trigger NIC tx reg.
+		 */
+		ret = NETDEV_TX_BUSY;
+		goto TXKICK;
 	}
 	spin_unlock_irqrestore(&np->lock, flags);
 
@@ -2357,8 +2363,14 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	spin_unlock_irqrestore(&np->lock, flags);
 
-	writel(NVREG_TXRXCTL_KICK|np->txrxctl_bits, get_hwbase(dev) + NvRegTxRxControl);
-	return NETDEV_TX_OK;
+TXKICK:
+	if (netif_queue_stopped(dev) || !netdev_xmit_more()) {
+		u32 txrxctl_kick = NVREG_TXRXCTL_KICK | np->txrxctl_bits;
+
+		writel(txrxctl_kick, get_hwbase(dev) + NvRegTxRxControl);
+	}
+
+	return ret;
 }
 
 static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
@@ -2381,6 +2393,7 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
 	struct nv_skb_map *start_tx_ctx = NULL;
 	struct nv_skb_map *tmp_tx_ctx = NULL;
 	unsigned long flags;
+	netdev_tx_t ret = NETDEV_TX_OK;
 
 	/* add fragments to entries count */
 	for (i = 0; i < fragments; i++) {
@@ -2396,7 +2409,13 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
 		netif_stop_queue(dev);
 		np->tx_stop = 1;
 		spin_unlock_irqrestore(&np->lock, flags);
-		return NETDEV_TX_BUSY;
+
+		/* When normal packets and/or xmit_more packets fill up
+		 * tx_desc, it is necessary to trigger NIC tx reg.
+		 */
+		ret = NETDEV_TX_BUSY;
+
+		goto TXKICK;
 	}
 	spin_unlock_irqrestore(&np->lock, flags);
 
@@ -2542,8 +2561,14 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
 
 	spin_unlock_irqrestore(&np->lock, flags);
 
-	writel(NVREG_TXRXCTL_KICK|np->txrxctl_bits, get_hwbase(dev) + NvRegTxRxControl);
-	return NETDEV_TX_OK;
+TXKICK:
+	if (netif_queue_stopped(dev) || !netdev_xmit_more()) {
+		u32 txrxctl_kick = NVREG_TXRXCTL_KICK | np->txrxctl_bits;
+
+		writel(txrxctl_kick, get_hwbase(dev) + NvRegTxRxControl);
+	}
+
+	return ret;
 }
 
 static inline void nv_tx_flip_ownership(struct net_device *dev)
-- 
2.7.4

Re: [PATCH] net: forcedeth: add xmit_more support

[Jakub Kicinski]

On Fri, 18 Oct 2019 06:01:25 -0400, Zhu Yanjun wrote:
> This change adds support for xmit_more based on the igb commit 6f19e12f6230
> ("igb: flush when in xmit_more mode and under descriptor pressure") and
> commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data") that
> were made to igb to support this feature. The function netif_xmit_stopped
> is called to check if transmit queue on device is currently unable to send
> to determine if we must write the tail because we can add no further
> buffers.
> When normal packets and/or xmit_more packets fill up tx_desc, it is
> necessary to trigger NIC tx reg.

Looks broken. You gotta make sure you check the kick on _every_ return
path. There are 4 return statements in each function, you only touched
2.

Also the labels should be lower case.

Re: [PATCH] net: forcedeth: add xmit_more support

[Zhu Yanjun]

On 2019/10/19 6:48, Jakub Kicinski wrote:
> On Fri, 18 Oct 2019 06:01:25 -0400, Zhu Yanjun wrote:
>> This change adds support for xmit_more based on the igb commit 6f19e12f6230
>> ("igb: flush when in xmit_more mode and under descriptor pressure") and
>> commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data") that
>> were made to igb to support this feature. The function netif_xmit_stopped
>> is called to check if transmit queue on device is currently unable to send
>> to determine if we must write the tail because we can add no further
>> buffers.
>> When normal packets and/or xmit_more packets fill up tx_desc, it is
>> necessary to trigger NIC tx reg.
> Looks broken. You gotta make sure you check the kick on _every_ return
> path. There are 4 return statements in each function, you only touched
> 2.

In nv_start_xmit,

2240         if (unlikely(empty_slots <= entries)) {
2241                 netif_stop_queue(dev);
2242                 np->tx_stop = 1;
2243                 spin_unlock_irqrestore(&np->lock, flags);
2244
2245                 /* When normal packets and/or xmit_more packets fill up
2246                  * tx_desc, it is necessary to trigger NIC tx reg.
2247                  */
2248                 ret = NETDEV_TX_BUSY;
2249                 goto TXKICK;
2250         }
The above indicates tx_desc is full, it is necessary to trigger NIC HW xmit.

2261                 if (unlikely(dma_mapping_error(&np->pci_dev->dev,
2262 np->put_tx_ctx->dma))) {
2263                         /* on DMA mapping error - drop the packet */
2264                         dev_kfree_skb_any(skb);
2265 u64_stats_update_begin(&np->swstats_tx_syncp);
2266                         nv_txrx_stats_inc(stat_tx_dropped);
2267 u64_stats_update_end(&np->swstats_tx_syncp);
2268                         return NETDEV_TX_OK;
2269                 }

and

2300                         if 
(unlikely(dma_mapping_error(&np->pci_dev->dev,
2301 np->put_tx_ctx->dma))) {
2302
2303                                 /* Unwind the mapped fragments */
2304                                 do {
2305                                         nv_unmap_txskb(np, 
start_tx_ctx);
2306                                         if (unlikely(tmp_tx_ctx++ 
== np->last_tx_ctx))
2307                                                 tmp_tx_ctx = 
np->tx_skb;
2308                                 } while (tmp_tx_ctx != np->put_tx_ctx);
2309                                 dev_kfree_skb_any(skb);
2310                                 np->put_tx_ctx = start_tx_ctx;
2311 u64_stats_update_begin(&np->swstats_tx_syncp);
2312 nv_txrx_stats_inc(stat_tx_dropped);
2313 u64_stats_update_end(&np->swstats_tx_syncp);
2314                                 return NETDEV_TX_OK;
2315                         }

The above are dma_mapping_error. It seems that triggering NIC HW xmit is 
not needed.

So when "tx_desc full" error, HW NIC xmit is triggerred. When 
dma_mapping_error,

NIC HW xmit is not triggerred.

That is why only 2 "return" are touched.

>
> Also the labels should be lower case.

This patch passes checkpatch.pl. It seems that "not lower case" is not a 
problem?

If you think it is a problem, please show me where it is defined.

Zhu Yanjun

>

Re: [PATCH] net: forcedeth: add xmit_more support

[Jakub Kicinski]

On Mon, 21 Oct 2019 17:56:06 +0800, Zhu Yanjun wrote:
> On 2019/10/19 6:48, Jakub Kicinski wrote:
> > On Fri, 18 Oct 2019 06:01:25 -0400, Zhu Yanjun wrote:  
> >> This change adds support for xmit_more based on the igb commit 6f19e12f6230
> >> ("igb: flush when in xmit_more mode and under descriptor pressure") and
> >> commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data") that
> >> were made to igb to support this feature. The function netif_xmit_stopped
> >> is called to check if transmit queue on device is currently unable to send
> >> to determine if we must write the tail because we can add no further
> >> buffers.
> >> When normal packets and/or xmit_more packets fill up tx_desc, it is
> >> necessary to trigger NIC tx reg.  
> > Looks broken. You gotta make sure you check the kick on _every_ return
> > path. There are 4 return statements in each function, you only touched
> > 2.  
> 
> In nv_start_xmit,
> 
> [...]
> 
> The above are dma_mapping_error. It seems that triggering NIC HW xmit is 
> not needed.
> 
> So when "tx_desc full" error, HW NIC xmit is triggerred. When 
> dma_mapping_error,
> 
> NIC HW xmit is not triggerred.
> 
> That is why only 2 "return" are touched.

Imagine you have the following sequence of frames:

	skbA  | xmit_more() == true
	skbB  | xmit_more() == true
	skbC  | xmit_more() == true
	skbD  | xmit_more() == false

A, B, and C got queued successfully but the driver didn't kick the
queue because of xmit_more(). Now D gets dropped due to a DMA error.
Queue never gets kicked.

> > Also the labels should be lower case.  
> 
> This patch passes checkpatch.pl. It seems that "not lower case" is not a 
> problem?
> 
> If you think it is a problem, please show me where it is defined.

Look at this driver and at other kernel code. Labels are lower case,
upper case is for constants and macros.

Re: [PATCH] net: forcedeth: add xmit_more support

[Zhu Yanjun]

On 2019/10/21 23:33, Jakub Kicinski wrote:
> On Mon, 21 Oct 2019 17:56:06 +0800, Zhu Yanjun wrote:
>> On 2019/10/19 6:48, Jakub Kicinski wrote:
>>> On Fri, 18 Oct 2019 06:01:25 -0400, Zhu Yanjun wrote:
>>>> This change adds support for xmit_more based on the igb commit 6f19e12f6230
>>>> ("igb: flush when in xmit_more mode and under descriptor pressure") and
>>>> commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data") that
>>>> were made to igb to support this feature. The function netif_xmit_stopped
>>>> is called to check if transmit queue on device is currently unable to send
>>>> to determine if we must write the tail because we can add no further
>>>> buffers.
>>>> When normal packets and/or xmit_more packets fill up tx_desc, it is
>>>> necessary to trigger NIC tx reg.
>>> Looks broken. You gotta make sure you check the kick on _every_ return
>>> path. There are 4 return statements in each function, you only touched
>>> 2.
>> In nv_start_xmit,
>>
>> [...]
>>
>> The above are dma_mapping_error. It seems that triggering NIC HW xmit is
>> not needed.
>>
>> So when "tx_desc full" error, HW NIC xmit is triggerred. When
>> dma_mapping_error,
>>
>> NIC HW xmit is not triggerred.
>>
>> That is why only 2 "return" are touched.
> Imagine you have the following sequence of frames:
>
> 	skbA  | xmit_more() == true
> 	skbB  | xmit_more() == true
> 	skbC  | xmit_more() == true
> 	skbD  | xmit_more() == false
>
> A, B, and C got queued successfully but the driver didn't kick the
> queue because of xmit_more(). Now D gets dropped due to a DMA error.
> Queue never gets kicked.

DMA error is a complicated problem. We will delve into this problem later.

 From the above commit log, this commit is based on the igb commit 
6f19e12f6230
("igb: flush when in xmit_more mode and under descriptor pressure") and
commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data").

It seems that the 2 commits did not consider the DMA errors that you 
mentioned.

>
>>> Also the labels should be lower case.
>> This patch passes checkpatch.pl. It seems that "not lower case" is not a
>> problem?
>>
>> If you think it is a problem, please show me where it is defined.
> Look at this driver and at other kernel code. Labels are lower case,
> upper case is for constants and macros.

It sounds reasonable. I will send V2 to fix this problem. Thanks.

Zhu Yanjun

>

Re: [PATCH] net: forcedeth: add xmit_more support

[Jakub Kicinski]

On Tue, 22 Oct 2019 13:32:35 +0800, Zhu Yanjun wrote:
> On 2019/10/21 23:33, Jakub Kicinski wrote:
> > On Mon, 21 Oct 2019 17:56:06 +0800, Zhu Yanjun wrote:  
> >> On 2019/10/19 6:48, Jakub Kicinski wrote:  
> >>> On Fri, 18 Oct 2019 06:01:25 -0400, Zhu Yanjun wrote:  
> >>>> This change adds support for xmit_more based on the igb commit 6f19e12f6230
> >>>> ("igb: flush when in xmit_more mode and under descriptor pressure") and
> >>>> commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data") that
> >>>> were made to igb to support this feature. The function netif_xmit_stopped
> >>>> is called to check if transmit queue on device is currently unable to send
> >>>> to determine if we must write the tail because we can add no further
> >>>> buffers.
> >>>> When normal packets and/or xmit_more packets fill up tx_desc, it is
> >>>> necessary to trigger NIC tx reg.  
> >>> Looks broken. You gotta make sure you check the kick on _every_ return
> >>> path. There are 4 return statements in each function, you only touched
> >>> 2.  
> >> In nv_start_xmit,
> >>
> >> [...]
> >>
> >> The above are dma_mapping_error. It seems that triggering NIC HW xmit is
> >> not needed.
> >>
> >> So when "tx_desc full" error, HW NIC xmit is triggerred. When
> >> dma_mapping_error,
> >>
> >> NIC HW xmit is not triggerred.
> >>
> >> That is why only 2 "return" are touched.  
> > Imagine you have the following sequence of frames:
> >
> > 	skbA  | xmit_more() == true
> > 	skbB  | xmit_more() == true
> > 	skbC  | xmit_more() == true
> > 	skbD  | xmit_more() == false
> >
> > A, B, and C got queued successfully but the driver didn't kick the
> > queue because of xmit_more(). Now D gets dropped due to a DMA error.
> > Queue never gets kicked.  
> 
> DMA error is a complicated problem. We will delve into this problem later.
> 
>  From the above commit log, this commit is based on the igb commit 
> 6f19e12f6230
> ("igb: flush when in xmit_more mode and under descriptor pressure") and
> commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data").
> 
> It seems that the 2 commits did not consider the DMA errors that you 
> mentioned.

Then igb is buggy, too.

Re: [PATCH] net: forcedeth: add xmit_more support

[Zhu Yanjun]

On 2019/10/22 23:40, Jakub Kicinski wrote:
> On Tue, 22 Oct 2019 13:32:35 +0800, Zhu Yanjun wrote:
>> On 2019/10/21 23:33, Jakub Kicinski wrote:
>>> On Mon, 21 Oct 2019 17:56:06 +0800, Zhu Yanjun wrote:
>>>> On 2019/10/19 6:48, Jakub Kicinski wrote:
>>>>> On Fri, 18 Oct 2019 06:01:25 -0400, Zhu Yanjun wrote:
>>>>>> This change adds support for xmit_more based on the igb commit 6f19e12f6230
>>>>>> ("igb: flush when in xmit_more mode and under descriptor pressure") and
>>>>>> commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data") that
>>>>>> were made to igb to support this feature. The function netif_xmit_stopped
>>>>>> is called to check if transmit queue on device is currently unable to send
>>>>>> to determine if we must write the tail because we can add no further
>>>>>> buffers.
>>>>>> When normal packets and/or xmit_more packets fill up tx_desc, it is
>>>>>> necessary to trigger NIC tx reg.
>>>>> Looks broken. You gotta make sure you check the kick on _every_ return
>>>>> path. There are 4 return statements in each function, you only touched
>>>>> 2.
>>>> In nv_start_xmit,
>>>>
>>>> [...]
>>>>
>>>> The above are dma_mapping_error. It seems that triggering NIC HW xmit is
>>>> not needed.
>>>>
>>>> So when "tx_desc full" error, HW NIC xmit is triggerred. When
>>>> dma_mapping_error,
>>>>
>>>> NIC HW xmit is not triggerred.
>>>>
>>>> That is why only 2 "return" are touched.
>>> Imagine you have the following sequence of frames:
>>>
>>> 	skbA  | xmit_more() == true
>>> 	skbB  | xmit_more() == true
>>> 	skbC  | xmit_more() == true
>>> 	skbD  | xmit_more() == false
>>>
>>> A, B, and C got queued successfully but the driver didn't kick the
>>> queue because of xmit_more(). Now D gets dropped due to a DMA error.
>>> Queue never gets kicked.
>> DMA error is a complicated problem. We will delve into this problem later.
>>
>>   From the above commit log, this commit is based on the igb commit
>> 6f19e12f6230
>> ("igb: flush when in xmit_more mode and under descriptor pressure") and
>> commit 6b16f9ee89b8 ("net: move skb->xmit_more hint to softnet data").
>>
>> It seems that the 2 commits did not consider the DMA errors that you
>> mentioned.
> Then igb is buggy, too.

Then if igb problem is fixed, I will follow.;-)

Zhu Yanjun

>