RE: [PATCH] rtw88: pci: Use general byte arrays as the elements of RX ring
From: Jian-Hong Pan > Sent: 25 July 2019 09:09 > Each skb as the element in RX ring was expected with sized buffer 8216 > (RTK_PCI_RX_BUF_SIZE) bytes. However, the skb buffer's true size is > 16640 bytes for alignment after allocated, x86_64 for example. And, the > difference will be enlarged 512 times (RTK_MAX_RX_DESC_NUM). > To prevent that much wasted memory, this patch follows David's > suggestion [1] and uses general buffer arrays, instead of skbs as the > elements in RX ring. ... > for (i = 0; i < len; i++) { > - skb = dev_alloc_skb(buf_sz); > - if (!skb) { > + buf = devm_kzalloc(rtwdev->dev, buf_sz, GFP_ATOMIC); You should do this allocation somewhere than can sleep. So you don't need GFP_ATOMIC, making the allocate (and dma map) much less likely to fail. If they do fail using a smaller ring might be better than failing completely. I suspect that buf_sz gets rounded up somewhat. Also you almost certainly want 'buf' to be cache-line aligned. I don't think devm_kzalloc() guarantees that at all. While allocating all 512 buffers in one block (just over 4MB) is probably not a good idea, you may need to allocated (and dma map) then in groups. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] rtw88: pci: Use general byte arrays as the elements of RX ring
David Laight 於 2019年7月25日 週四 下午5:21寫道: > > From: Jian-Hong Pan > > Sent: 25 July 2019 09:09 > > Each skb as the element in RX ring was expected with sized buffer 8216 > > (RTK_PCI_RX_BUF_SIZE) bytes. However, the skb buffer's true size is > > 16640 bytes for alignment after allocated, x86_64 for example. And, the > > difference will be enlarged 512 times (RTK_MAX_RX_DESC_NUM). > > To prevent that much wasted memory, this patch follows David's > > suggestion [1] and uses general buffer arrays, instead of skbs as the > > elements in RX ring. > ... > > for (i = 0; i < len; i++) { > > - skb = dev_alloc_skb(buf_sz); > > - if (!skb) { > > + buf = devm_kzalloc(rtwdev->dev, buf_sz, GFP_ATOMIC); > > You should do this allocation somewhere than can sleep. > So you don't need GFP_ATOMIC, making the allocate (and dma map) > much less likely to fail. > If they do fail using a smaller ring might be better than failing > completely. Ok, I can tweak and try it. > I suspect that buf_sz gets rounded up somewhat. > Also you almost certainly want 'buf' to be cache-line aligned. > I don't think devm_kzalloc() guarantees that at all. Sure > While allocating all 512 buffers in one block (just over 4MB) > is probably not a good idea, you may need to allocated (and dma map) > then in groups. Thanks for reviewing. But got questions here to double confirm the idea. According to original code, it allocates 512 skbs for RX ring and dma mapping one by one. So, the new code allocates memory buffer 512 times to get 512 buffer arrays. Will the 512 buffers arrays be in one block? Do you mean aggregate the buffers as a scatterlist and use dma_map_sg? Thank you, Jain-Hong Pan
RE: [PATCH] rtw88: pci: Use general byte arrays as the elements of RX ring
From: Jian-Hong Pan > Sent: 26 July 2019 07:18 ... > > While allocating all 512 buffers in one block (just over 4MB) > > is probably not a good idea, you may need to allocated (and dma map) > > then in groups. > > Thanks for reviewing. But got questions here to double confirm the idea. > According to original code, it allocates 512 skbs for RX ring and dma > mapping one by one. So, the new code allocates memory buffer 512 > times to get 512 buffer arrays. Will the 512 buffers arrays be in one > block? Do you mean aggregate the buffers as a scatterlist and use > dma_map_sg? If you malloc a buffer of size (8192+32) the allocator will either round it up to a whole number of (often 4k) pages or to a power of 2 of pages - so either 12k of 16k. I think the Linux allocator does the latter. Some of the allocators also 'steal' a bit from the front of the buffer for 'red tape'. OTOH malloc the space 15 buffers and the allocator will round the 15*(8192 + 32) up to 32*4k - and you waste under 8k across all the buffers. You then dma_map the large buffer and split into the actual rx buffers. Repeat until you've filled the entire ring. The only complication is remembering the base address (and size) for the dma_unmap and free. Although there is plenty of padding to extend the buffer structure significantly without using more memory. Allocate in 15's and you (probably) have 512 bytes per buffer. Allocate in 31's and you have 256 bytes. The problem is that larger allocates are more likely to fail (especially if the system has been running for some time). So you almost certainly want to be able to fall back to smaller allocates even though they use more memory. I also wonder if you actually need 512 8k rx buffers to cover interrupt latency? I've not done any measurements for 20 years! David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] rtw88: pci: Use general byte arrays as the elements of RX ring
David Laight 於 2019年7月26日 週五 下午5:23寫道: > > From: Jian-Hong Pan > > Sent: 26 July 2019 07:18 > ... > > > While allocating all 512 buffers in one block (just over 4MB) > > > is probably not a good idea, you may need to allocated (and dma map) > > > then in groups. > > > > Thanks for reviewing. But got questions here to double confirm the idea. > > According to original code, it allocates 512 skbs for RX ring and dma > > mapping one by one. So, the new code allocates memory buffer 512 > > times to get 512 buffer arrays. Will the 512 buffers arrays be in one > > block? Do you mean aggregate the buffers as a scatterlist and use > > dma_map_sg? > > If you malloc a buffer of size (8192+32) the allocator will either > round it up to a whole number of (often 4k) pages or to a power of > 2 of pages - so either 12k of 16k. > I think the Linux allocator does the latter. > Some of the allocators also 'steal' a bit from the front of the buffer > for 'red tape'. > > OTOH malloc the space 15 buffers and the allocator will round the > 15*(8192 + 32) up to 32*4k - and you waste under 8k across all the > buffers. > > You then dma_map the large buffer and split into the actual rx buffers. > Repeat until you've filled the entire ring. > The only complication is remembering the base address (and size) for > the dma_unmap and free. > Although there is plenty of padding to extend the buffer structure > significantly without using more memory. > Allocate in 15's and you (probably) have 512 bytes per buffer. > Allocate in 31's and you have 256 bytes. > > The problem is that larger allocates are more likely to fail > (especially if the system has been running for some time). > So you almost certainly want to be able to fall back to smaller > allocates even though they use more memory. > > I also wonder if you actually need 512 8k rx buffers to cover > interrupt latency? > I've not done any measurements for 20 years! Thanks for the explanation. I am not sure the combination of 512 8k RX buffers. Maybe Realtek folks can give us some idea. Tony Chuang any comment? Jian-Hong Pan
RE: [PATCH] rtw88: pci: Use general byte arrays as the elements of RX ring
> > > > While allocating all 512 buffers in one block (just over 4MB) > > > > is probably not a good idea, you may need to allocated (and dma map) > > > > then in groups. > > > > > > Thanks for reviewing. But got questions here to double confirm the > idea. > > > According to original code, it allocates 512 skbs for RX ring and dma > > > mapping one by one. So, the new code allocates memory buffer 512 > > > times to get 512 buffer arrays. Will the 512 buffers arrays be in one > > > block? Do you mean aggregate the buffers as a scatterlist and use > > > dma_map_sg? > > > > If you malloc a buffer of size (8192+32) the allocator will either > > round it up to a whole number of (often 4k) pages or to a power of > > 2 of pages - so either 12k of 16k. > > I think the Linux allocator does the latter. > > Some of the allocators also 'steal' a bit from the front of the buffer > > for 'red tape'. > > > > OTOH malloc the space 15 buffers and the allocator will round the > > 15*(8192 + 32) up to 32*4k - and you waste under 8k across all the > > buffers. > > > > You then dma_map the large buffer and split into the actual rx buffers. > > Repeat until you've filled the entire ring. > > The only complication is remembering the base address (and size) for > > the dma_unmap and free. > > Although there is plenty of padding to extend the buffer structure > > significantly without using more memory. > > Allocate in 15's and you (probably) have 512 bytes per buffer. > > Allocate in 31's and you have 256 bytes. > > > > The problem is that larger allocates are more likely to fail > > (especially if the system has been running for some time). > > So you almost certainly want to be able to fall back to smaller > > allocates even though they use more memory. > > > > I also wonder if you actually need 512 8k rx buffers to cover > > interrupt latency? > > I've not done any measurements for 20 years! > > Thanks for the explanation. > I am not sure the combination of 512 8k RX buffers. Maybe Realtek > folks can give us some idea. > Tony Chuang any comment? > > Jian-Hong Pan > 512 RX buffers is not necessary I think. But I haven't had a chance to test if reduce the number of RX SKBs could affect the latency. I can run some throughput tests and then decide a minimum numbers that RX ring requires. Or if you can try it. Thanks. Yan-Hsuan
Re: [PATCH] rtw88: pci: Use general byte arrays as the elements of RX ring
On Thu, Jul 25, 2019 at 04:09:26PM +0800, Jian-Hong Pan wrote: > Each skb as the element in RX ring was expected with sized buffer 8216 > (RTK_PCI_RX_BUF_SIZE) bytes. However, the skb buffer's true size is > 16640 bytes for alignment after allocated, x86_64 for example. And, the rtw88 advertise IEEE80211_VHT_CAP_MAX_MPDU_LENGTH_11454, so maximum AMSDU packet can be approximately 12kB. This might be accidental, but having 16kB skb's allow to handle such big AMSDUs. If you shrink buf size, you can probably override memory after buffer end. > difference will be enlarged 512 times (RTK_MAX_RX_DESC_NUM). > To prevent that much wasted memory, this patch follows David's > suggestion [1] and uses general buffer arrays, instead of skbs as the > elements in RX ring. > > [1] https://www.spinics.net/lists/linux-wireless/msg187870.html > > Signed-off-by: Jian-Hong Pan > Cc: This does not fix any serious problem, it actually most likely introduce memory corruption problem described above. Should not be targeted to stable anyway. > - dev_kfree_skb_any(skb); > + devm_kfree(rtwdev->dev, buf); For what this is needed? devm_ allocations are used exactly to avoid manual freeing. > + len = pkt_stat.pkt_len + pkt_offset; > + skb = dev_alloc_skb(len); > + if (WARN_ONCE(!skb, "rx routine starvation\n")) > goto next_rp; > > /* put the DMA data including rx_desc from phy to new skb */ > - skb_put_data(new, skb->data, new_len); > + skb_put_data(skb, rx_desc, len); Coping big packets it quite inefficient. What drivers usually do is copy only for small packets and for big ones allocate new rx buf (drop packet alloc if fail) and pas old buf to network stack via skb_add_rx_frag(). See iwlmvm as example. Stanislaw
RE: [PATCH] rtw88: pci: Use general byte arrays as the elements of RX ring
From: Stanislaw Gruszka > Sent: 30 July 2019 10:36 ... > > + len = pkt_stat.pkt_len + pkt_offset; > > + skb = dev_alloc_skb(len); > > + if (WARN_ONCE(!skb, "rx routine starvation\n")) > > goto next_rp; > > > > /* put the DMA data including rx_desc from phy to new skb */ > > - skb_put_data(new, skb->data, new_len); > > + skb_put_data(skb, rx_desc, len); > > Coping big packets it quite inefficient. What drivers usually do is > copy only for small packets and for big ones allocate new rx buf > (drop packet alloc if fail) and pas old buf to network stack via > skb_add_rx_frag(). See iwlmvm as example. If you have to do iommu setup/teardown then the breakeven point for (not) copying may be surprisingly large. You do need to do the measurements on a range of hardware. Coping is also likely to affect the L1 cache - unless you can copy quickly without polluting the cache. It is all 'swings and roundabouts'. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)