| Message ID | 1571392885-32706-1-git-send-email-yanjun.zhu@oracle.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: forcedeth: add xmit_more support | expand |
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.
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 >
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.
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 >
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.
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 >
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)
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(-)