| Message ID | 20201206034408.31492-2-TheSven73@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net,v1,1/2] lan743x: improve performance: fix rx_napi_poll/interrupt ping-pong | expand |
On Sat, 5 Dec 2020 22:44:08 -0500 Sven Van Asbroeck wrote: > From: Sven Van Asbroeck <thesven73@gmail.com> > > To support jumbo frames, each rx ring dma buffer is 9K in size. > But the chip only stores a single frame per dma buffer. > > When the chip is working with the default 1500 byte MTU, a 9K > dma buffer goes from chip -> cpu per 1500 byte frame. This means > that to get 1G/s ethernet bandwidth, we need 6G/s PCIe bandwidth ! > > Fix by limiting the rx ring dma buffer size to the current MTU > size. I'd guess this is a memory allocate issue, not a bandwidth thing. for 9K frames the driver needs to do order-2 allocations of 16K. For 1500 2K allocations are sufficient (which is < 1 page, hence a lot cheaper). > Tested with iperf3 on a freescale imx6 + lan7430, both sides > set to mtu 1500 bytes. > > Before: > [ ID] Interval Transfer Bandwidth Retr > [ 4] 0.00-20.00 sec 483 MBytes 203 Mbits/sec 0 > After: > [ ID] Interval Transfer Bandwidth Retr > [ 4] 0.00-20.00 sec 1.15 GBytes 496 Mbits/sec 0 > > And with both sides set to MTU 9000 bytes: > Before: > [ ID] Interval Transfer Bandwidth Retr > [ 4] 0.00-20.00 sec 1.87 GBytes 803 Mbits/sec 27 > After: > [ ID] Interval Transfer Bandwidth Retr > [ 4] 0.00-20.00 sec 1.98 GBytes 849 Mbits/sec 0 > > Tested-by: Sven Van Asbroeck <thesven73@gmail.com> # lan7430 > Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com> This is a performance improvement, not a fix, it really needs to target net-next. > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c > index ebb5e0bc516b..2bded1c46784 100644 > --- a/drivers/net/ethernet/microchip/lan743x_main.c > +++ b/drivers/net/ethernet/microchip/lan743x_main.c > @@ -1957,11 +1957,11 @@ static int lan743x_rx_next_index(struct lan743x_rx *rx, int index) > > static struct sk_buff *lan743x_rx_allocate_skb(struct lan743x_rx *rx) > { > - int length = 0; > + struct net_device *netdev = rx->adapter->netdev; > > - length = (LAN743X_MAX_FRAME_SIZE + ETH_HLEN + 4 + RX_HEAD_PADDING); > - return __netdev_alloc_skb(rx->adapter->netdev, > - length, GFP_ATOMIC | GFP_DMA); > + return __netdev_alloc_skb(netdev, > + netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING, > + GFP_ATOMIC | GFP_DMA); > } > > static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index, > @@ -1969,9 +1969,10 @@ static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index, > { > struct lan743x_rx_buffer_info *buffer_info; > struct lan743x_rx_descriptor *descriptor; > - int length = 0; > + struct net_device *netdev = rx->adapter->netdev; > + int length; > > - length = (LAN743X_MAX_FRAME_SIZE + ETH_HLEN + 4 + RX_HEAD_PADDING); > + length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING; > descriptor = &rx->ring_cpu_ptr[index]; > buffer_info = &rx->buffer_info[index]; > buffer_info->skb = skb; > @@ -2157,8 +2158,8 @@ static int lan743x_rx_process_packet(struct lan743x_rx *rx) > int index = first_index; > > /* multi buffer packet not supported */ > - /* this should not happen since > - * buffers are allocated to be at least jumbo size > + /* this should not happen since buffers are allocated > + * to be at least the mtu size configured in the mac. > */ > > /* clean up buffers */ > @@ -2632,9 +2633,13 @@ static int lan743x_netdev_change_mtu(struct net_device *netdev, int new_mtu) > struct lan743x_adapter *adapter = netdev_priv(netdev); > int ret = 0; > > + if (netif_running(netdev)) > + return -EBUSY; That may cause a regression to users of the driver who expect to be able to set the MTU when the device is running. You need to disable the NAPI, pause the device, swap the buffers for smaller / bigger ones and restart the device. > ret = lan743x_mac_set_mtu(adapter, new_mtu); > if (!ret) > netdev->mtu = new_mtu; > + > return ret; > } >
Hi Jakub, thank you so much for reviewing this patchset ! On Tue, Dec 8, 2020 at 2:43 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > When the chip is working with the default 1500 byte MTU, a 9K > > dma buffer goes from chip -> cpu per 1500 byte frame. This means > > that to get 1G/s ethernet bandwidth, we need 6G/s PCIe bandwidth ! > > > > Fix by limiting the rx ring dma buffer size to the current MTU > > size. > > I'd guess this is a memory allocate issue, not a bandwidth thing. > for 9K frames the driver needs to do order-2 allocations of 16K. > For 1500 2K allocations are sufficient (which is < 1 page, hence > a lot cheaper). That's a good question. I used perf to create a flame graph of what the cpu was doing when receiving data at high speed. It showed that __dma_page_dev_to_cpu took up most of the cpu time. Which is triggered by dma_unmap_single(9K, DMA_FROM_DEVICE). So I assumed that it's a PCIe dma bandwidth issue, but I could be wrong - I didn't do any PCIe bandwidth measurements. > > > Tested with iperf3 on a freescale imx6 + lan7430, both sides > > set to mtu 1500 bytes. > > > > Before: > > [ ID] Interval Transfer Bandwidth Retr > > [ 4] 0.00-20.00 sec 483 MBytes 203 Mbits/sec 0 > > After: > > [ ID] Interval Transfer Bandwidth Retr > > [ 4] 0.00-20.00 sec 1.15 GBytes 496 Mbits/sec 0 > > > > And with both sides set to MTU 9000 bytes: > > Before: > > [ ID] Interval Transfer Bandwidth Retr > > [ 4] 0.00-20.00 sec 1.87 GBytes 803 Mbits/sec 27 > > After: > > [ ID] Interval Transfer Bandwidth Retr > > [ 4] 0.00-20.00 sec 1.98 GBytes 849 Mbits/sec 0 > > > > Tested-by: Sven Van Asbroeck <thesven73@gmail.com> # lan7430 > > Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com> > > This is a performance improvement, not a fix, it really needs to target > net-next. I thought it'd be cool if 'historic' kernels could benefit from this performance improvement too, but yeah if it's against policy it should go into net-next. What about the other patch in the patchset (ping-pong). Should it go into net-next as well? > > > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c > > index ebb5e0bc516b..2bded1c46784 100644 > > --- a/drivers/net/ethernet/microchip/lan743x_main.c > > +++ b/drivers/net/ethernet/microchip/lan743x_main.c > > @@ -1957,11 +1957,11 @@ static int lan743x_rx_next_index(struct lan743x_rx *rx, int index) > > > > static struct sk_buff *lan743x_rx_allocate_skb(struct lan743x_rx *rx) > > { > > - int length = 0; > > + struct net_device *netdev = rx->adapter->netdev; > > > > - length = (LAN743X_MAX_FRAME_SIZE + ETH_HLEN + 4 + RX_HEAD_PADDING); > > - return __netdev_alloc_skb(rx->adapter->netdev, > > - length, GFP_ATOMIC | GFP_DMA); > > + return __netdev_alloc_skb(netdev, > > + netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING, > > + GFP_ATOMIC | GFP_DMA); > > } > > > > static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index, > > @@ -1969,9 +1969,10 @@ static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index, > > { > > struct lan743x_rx_buffer_info *buffer_info; > > struct lan743x_rx_descriptor *descriptor; > > - int length = 0; > > + struct net_device *netdev = rx->adapter->netdev; > > + int length; > > > > - length = (LAN743X_MAX_FRAME_SIZE + ETH_HLEN + 4 + RX_HEAD_PADDING); > > + length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING; > > descriptor = &rx->ring_cpu_ptr[index]; > > buffer_info = &rx->buffer_info[index]; > > buffer_info->skb = skb; > > @@ -2157,8 +2158,8 @@ static int lan743x_rx_process_packet(struct lan743x_rx *rx) > > int index = first_index; > > > > /* multi buffer packet not supported */ > > - /* this should not happen since > > - * buffers are allocated to be at least jumbo size > > + /* this should not happen since buffers are allocated > > + * to be at least the mtu size configured in the mac. > > */ > > > > /* clean up buffers */ > > @@ -2632,9 +2633,13 @@ static int lan743x_netdev_change_mtu(struct net_device *netdev, int new_mtu) > > struct lan743x_adapter *adapter = netdev_priv(netdev); > > int ret = 0; > > > > + if (netif_running(netdev)) > > + return -EBUSY; > > That may cause a regression to users of the driver who expect to be > able to set the MTU when the device is running. You need to disable > the NAPI, pause the device, swap the buffers for smaller / bigger ones > and restart the device. That's what I tried first, but I quickly ran into a spot of trouble: restarting the device may fail (unlikely but possible). So when the user tries to change the mtu and that errors out, they might end up with a stopped device. Is that acceptable behaviour? If so, I'll add it to the patch. > > > ret = lan743x_mac_set_mtu(adapter, new_mtu); > > if (!ret) > > netdev->mtu = new_mtu; > > + > > return ret; > > } > > >
> That's a good question. I used perf to create a flame graph of what > the cpu was doing when receiving data at high speed. It showed that > __dma_page_dev_to_cpu took up most of the cpu time. Which is triggered > by dma_unmap_single(9K, DMA_FROM_DEVICE). > > So I assumed that it's a PCIe dma bandwidth issue, but I could be wrong - > I didn't do any PCIe bandwidth measurements. Sometimes it is actually cache operations which take all the time. This needs to invalidate the cache, so that when the memory is then accessed, it get fetched from RAM. On SMP machines, cache invalidation can be expensive, due to all the cross CPU operations. I've actually got better performance by building a UP kernel on some low core count ARM CPUs. There are some tricks which can be played. Do you actually need all 9K? Does the descriptor tell you actually how much is used? You can get a nice speed up if you just unmap 64 bytes for a TCP ACK, rather than the full 9K. Andrew
Hi Andrew, On Tue, Dec 8, 2020 at 5:51 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > So I assumed that it's a PCIe dma bandwidth issue, but I could be wrong - > > I didn't do any PCIe bandwidth measurements. > > Sometimes it is actually cache operations which take all the > time. This needs to invalidate the cache, so that when the memory is > then accessed, it get fetched from RAM. On SMP machines, cache > invalidation can be expensive, due to all the cross CPU operations. > I've actually got better performance by building a UP kernel on some > low core count ARM CPUs. > > There are some tricks which can be played. Do you actually need all > 9K? Does the descriptor tell you actually how much is used? You can > get a nice speed up if you just unmap 64 bytes for a TCP ACK, rather > than the full 9K. > Thank you for the suggestion! The original driver developer chose 9K because presumably that's the largest frame size supported by the chip. Yes, I believe the chip will tell us via the descriptor how much it has written, I would have to double-check. I was already looking for a "trick" to transfer only the required number of bytes, but I was led to believe that dma_map_single() and dma_unmap_single() always needed to match. So: dma_map_single(9K) followed by dma_unmap_single(9K) is correct, and dma_map_single(9K) followed by dma_unmap_single(1500 bytes) means trouble. How can we get around that?
On Tue, 8 Dec 2020 18:02:30 -0500 Sven Van Asbroeck wrote: > On Tue, Dec 8, 2020 at 5:51 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > So I assumed that it's a PCIe dma bandwidth issue, but I could be wrong - > > > I didn't do any PCIe bandwidth measurements. > > > > Sometimes it is actually cache operations which take all the > > time. This needs to invalidate the cache, so that when the memory is > > then accessed, it get fetched from RAM. On SMP machines, cache > > invalidation can be expensive, due to all the cross CPU operations. > > I've actually got better performance by building a UP kernel on some > > low core count ARM CPUs. > > > > There are some tricks which can be played. Do you actually need all > > 9K? Does the descriptor tell you actually how much is used? You can > > get a nice speed up if you just unmap 64 bytes for a TCP ACK, rather > > than the full 9K. Good point! > Thank you for the suggestion! The original driver developer chose 9K because > presumably that's the largest frame size supported by the chip. > > Yes, I believe the chip will tell us via the descriptor how much it has > written, I would have to double-check. I was already looking for a > "trick" to transfer only the required number of bytes, but I was led to > believe that dma_map_single() and dma_unmap_single() always needed to match. > > So: > dma_map_single(9K) followed by dma_unmap_single(9K) is correct, and > dma_map_single(9K) followed by dma_unmap_single(1500 bytes) means trouble. > > How can we get around that? You can set DMA_ATTR_SKIP_CPU_SYNC and then sync only the part of the buffer that got written.
On Tue, 8 Dec 2020 16:54:33 -0500 Sven Van Asbroeck wrote: > > > Tested with iperf3 on a freescale imx6 + lan7430, both sides > > > set to mtu 1500 bytes. > > > > > > Before: > > > [ ID] Interval Transfer Bandwidth Retr > > > [ 4] 0.00-20.00 sec 483 MBytes 203 Mbits/sec 0 > > > After: > > > [ ID] Interval Transfer Bandwidth Retr > > > [ 4] 0.00-20.00 sec 1.15 GBytes 496 Mbits/sec 0 > > > > > > And with both sides set to MTU 9000 bytes: > > > Before: > > > [ ID] Interval Transfer Bandwidth Retr > > > [ 4] 0.00-20.00 sec 1.87 GBytes 803 Mbits/sec 27 > > > After: > > > [ ID] Interval Transfer Bandwidth Retr > > > [ 4] 0.00-20.00 sec 1.98 GBytes 849 Mbits/sec 0 > > > > > > Tested-by: Sven Van Asbroeck <thesven73@gmail.com> # lan7430 > > > Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com> > > > > This is a performance improvement, not a fix, it really needs to target > > net-next. > > I thought it'd be cool if 'historic' kernels could benefit from this performance > improvement too, but yeah if it's against policy it should go into net-next. > > What about the other patch in the patchset (ping-pong). Should it go into > net-next as well? The jury is out on that one. Using ring size for netif_napi_add() and updating RX_TAIL at the end of NAPI is pretty broken. So that one can qualify as a fix IMHO. > > > @@ -2632,9 +2633,13 @@ static int lan743x_netdev_change_mtu(struct net_device *netdev, int new_mtu) > > > struct lan743x_adapter *adapter = netdev_priv(netdev); > > > int ret = 0; > > > > > > + if (netif_running(netdev)) > > > + return -EBUSY; > > > > That may cause a regression to users of the driver who expect to be > > able to set the MTU when the device is running. You need to disable > > the NAPI, pause the device, swap the buffers for smaller / bigger ones > > and restart the device. > > That's what I tried first, but I quickly ran into a spot of trouble: > restarting the device may fail (unlikely but possible). So when the user tries > to change the mtu and that errors out, they might end up with a stopped device. > Is that acceptable behaviour? If so, I'll add it to the patch. Fail because of memory allocation failures? The best way to work around that is to allocate all the memory for new configuration before you free the old memory. This also makes the change a lot less disturbing to the traffic because you can do all the allocations before the device is disabled, do the swap, start the device, and then free the old set.
On 12/8/20 3:02 PM, Sven Van Asbroeck wrote: > Hi Andrew, > > On Tue, Dec 8, 2020 at 5:51 PM Andrew Lunn <andrew@lunn.ch> wrote: >> >>> >>> So I assumed that it's a PCIe dma bandwidth issue, but I could be wrong - >>> I didn't do any PCIe bandwidth measurements. >> >> Sometimes it is actually cache operations which take all the >> time. This needs to invalidate the cache, so that when the memory is >> then accessed, it get fetched from RAM. On SMP machines, cache >> invalidation can be expensive, due to all the cross CPU operations. >> I've actually got better performance by building a UP kernel on some >> low core count ARM CPUs. >> >> There are some tricks which can be played. Do you actually need all >> 9K? Does the descriptor tell you actually how much is used? You can >> get a nice speed up if you just unmap 64 bytes for a TCP ACK, rather >> than the full 9K. >> > > Thank you for the suggestion! The original driver developer chose 9K because > presumably that's the largest frame size supported by the chip. > > Yes, I believe the chip will tell us via the descriptor how much it has > written, I would have to double-check. I was already looking for a > "trick" to transfer only the required number of bytes, but I was led to > believe that dma_map_single() and dma_unmap_single() always needed to match. > > So: > dma_map_single(9K) followed by dma_unmap_single(9K) is correct, and > dma_map_single(9K) followed by dma_unmap_single(1500 bytes) means trouble. > > How can we get around that? dma_sync_single_for_{cpu,device} is what you would need in order to make a partial cache line invalidation. You would still need to unmap the same address+length pair that was used for the initial mapping otherwise the DMA-API debugging will rightfully complain.
> dma_sync_single_for_{cpu,device} is what you would need in order to make > a partial cache line invalidation. You would still need to unmap the > same address+length pair that was used for the initial mapping otherwise > the DMA-API debugging will rightfully complain. But often you don't unmap it, you call dma_sync_single_for_device and put it back into the ring. Andrew
On Tue, Dec 8, 2020 at 6:36 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > dma_sync_single_for_{cpu,device} is what you would need in order to make > a partial cache line invalidation. You would still need to unmap the > same address+length pair that was used for the initial mapping otherwise > the DMA-API debugging will rightfully complain. I tried replacing dma_unmap_single(9K, DMA_FROM_DEVICE); with dma_sync_single_for_cpu(received_size=1500 bytes, DMA_FROM_DEVICE); dma_unmap_single_attrs(9K, DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC); and that works! But the bandwidth is still pretty bad, because the cpu now spends most of its time doing dma_map_single(9K, DMA_FROM_DEVICE); which spends a lot of time doing __dma_page_cpu_to_dev. When I try and replace that with dma_map_single_attrs(9K, DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC); Then I get lots of dropped packets, which seems to indicate data corruption. Interestingly, when I do dma_map_single_attrs(9K, DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC); dma_sync_single_for_{cpu|device}(9K, DMA_FROM_DEVICE); then the dropped packets disappear, but things are still very slow. What am I missing?
On Tue, Dec 08, 2020 at 10:49:16PM -0500, Sven Van Asbroeck wrote: > On Tue, Dec 8, 2020 at 6:36 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > dma_sync_single_for_{cpu,device} is what you would need in order to make > > a partial cache line invalidation. You would still need to unmap the > > same address+length pair that was used for the initial mapping otherwise > > the DMA-API debugging will rightfully complain. > > I tried replacing > dma_unmap_single(9K, DMA_FROM_DEVICE); > with > dma_sync_single_for_cpu(received_size=1500 bytes, DMA_FROM_DEVICE); > dma_unmap_single_attrs(9K, DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC); > > and that works! But the bandwidth is still pretty bad, because the cpu > now spends most of its time doing > dma_map_single(9K, DMA_FROM_DEVICE); > which spends a lot of time doing __dma_page_cpu_to_dev. 9K is not a nice number, since for each allocation it probably has to find 4 contiguous pages. See what the performance difference is with 2K, 4K and 8K. If there is a big difference, you might want to special case when the MTU is set for jumbo packets, or check if the hardware can do scatter/gather. You also need to be careful with caches and speculation. As you have seen, bad things can happen. And it can be a lot more subtle. If some code is accessing the page before the buffer and gets towards the end of the page, the CPU might speculatively bring in the next page, i.e the start of the buffer. If that happens before the DMA operation, and you don't invalidate the cache correctly, you get hard to find corruption. Andrew
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c index ebb5e0bc516b..2bded1c46784 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.c +++ b/drivers/net/ethernet/microchip/lan743x_main.c @@ -1957,11 +1957,11 @@ static int lan743x_rx_next_index(struct lan743x_rx *rx, int index) static struct sk_buff *lan743x_rx_allocate_skb(struct lan743x_rx *rx) { - int length = 0; + struct net_device *netdev = rx->adapter->netdev; - length = (LAN743X_MAX_FRAME_SIZE + ETH_HLEN + 4 + RX_HEAD_PADDING); - return __netdev_alloc_skb(rx->adapter->netdev, - length, GFP_ATOMIC | GFP_DMA); + return __netdev_alloc_skb(netdev, + netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING, + GFP_ATOMIC | GFP_DMA); } static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index, @@ -1969,9 +1969,10 @@ static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index, { struct lan743x_rx_buffer_info *buffer_info; struct lan743x_rx_descriptor *descriptor; - int length = 0; + struct net_device *netdev = rx->adapter->netdev; + int length; - length = (LAN743X_MAX_FRAME_SIZE + ETH_HLEN + 4 + RX_HEAD_PADDING); + length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING; descriptor = &rx->ring_cpu_ptr[index]; buffer_info = &rx->buffer_info[index]; buffer_info->skb = skb; @@ -2157,8 +2158,8 @@ static int lan743x_rx_process_packet(struct lan743x_rx *rx) int index = first_index; /* multi buffer packet not supported */ - /* this should not happen since - * buffers are allocated to be at least jumbo size + /* this should not happen since buffers are allocated + * to be at least the mtu size configured in the mac. */ /* clean up buffers */ @@ -2632,9 +2633,13 @@ static int lan743x_netdev_change_mtu(struct net_device *netdev, int new_mtu) struct lan743x_adapter *adapter = netdev_priv(netdev); int ret = 0; + if (netif_running(netdev)) + return -EBUSY; + ret = lan743x_mac_set_mtu(adapter, new_mtu); if (!ret) netdev->mtu = new_mtu; + return ret; }