* [PATCH] net: forcedeth: add xmit_more support
@ 2019-10-18 10:01 Zhu Yanjun
2019-10-18 22:48 ` Jakub Kicinski
0 siblings, 1 reply; 7+ messages in thread
From: Zhu Yanjun @ 2019-10-18 10:01 UTC (permalink / raw)
To: rain.1986.08.12, yanjun.zhu, davem, netdev
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: forcedeth: add xmit_more support
2019-10-18 10:01 [PATCH] net: forcedeth: add xmit_more support Zhu Yanjun
@ 2019-10-18 22:48 ` Jakub Kicinski
2019-10-21 9:56 ` Zhu Yanjun
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2019-10-18 22:48 UTC (permalink / raw)
To: Zhu Yanjun; +Cc: rain.1986.08.12, davem, netdev
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: forcedeth: add xmit_more support
2019-10-18 22:48 ` Jakub Kicinski
@ 2019-10-21 9:56 ` Zhu Yanjun
2019-10-21 15:33 ` Jakub Kicinski
0 siblings, 1 reply; 7+ messages in thread
From: Zhu Yanjun @ 2019-10-21 9:56 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: rain.1986.08.12, davem, netdev
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: forcedeth: add xmit_more support
2019-10-21 9:56 ` Zhu Yanjun
@ 2019-10-21 15:33 ` Jakub Kicinski
2019-10-22 5:32 ` Zhu Yanjun
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2019-10-21 15:33 UTC (permalink / raw)
To: Zhu Yanjun; +Cc: rain.1986.08.12, davem, netdev
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: forcedeth: add xmit_more support
2019-10-21 15:33 ` Jakub Kicinski
@ 2019-10-22 5:32 ` Zhu Yanjun
2019-10-22 15:40 ` Jakub Kicinski
0 siblings, 1 reply; 7+ messages in thread
From: Zhu Yanjun @ 2019-10-22 5:32 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: rain.1986.08.12, davem, netdev
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: forcedeth: add xmit_more support
2019-10-22 5:32 ` Zhu Yanjun
@ 2019-10-22 15:40 ` Jakub Kicinski
2019-10-23 0:45 ` Zhu Yanjun
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2019-10-22 15:40 UTC (permalink / raw)
To: Zhu Yanjun; +Cc: rain.1986.08.12, davem, netdev, Alexander Duyck
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: forcedeth: add xmit_more support
2019-10-22 15:40 ` Jakub Kicinski
@ 2019-10-23 0:45 ` Zhu Yanjun
0 siblings, 0 replies; 7+ messages in thread
From: Zhu Yanjun @ 2019-10-23 0:45 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: rain.1986.08.12, davem, netdev, Alexander Duyck
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-23 0:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18 10:01 [PATCH] net: forcedeth: add xmit_more support Zhu Yanjun
2019-10-18 22:48 ` Jakub Kicinski
2019-10-21 9:56 ` Zhu Yanjun
2019-10-21 15:33 ` Jakub Kicinski
2019-10-22 5:32 ` Zhu Yanjun
2019-10-22 15:40 ` Jakub Kicinski
2019-10-23 0:45 ` Zhu Yanjun
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.