| Message ID | 20190708063252.4756-1-jian-hong@endlessm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88/pci: Rearrange the memory usage for skb in RX ISR | expand |
> Subject: [PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR nit, "rtw88: pci:" would be better. > > > When skb allocation fails and the "rx routine starvation" is hit, the > function returns immediately without updating the RX ring. At this > point, the RX ring may continue referencing an old skb which was already > handed off to ieee80211_rx_irqsafe(). When it comes to be used again, > bad things happen. > > This patch allocates a new skb first in RX ISR. If we don't have memory > available, we discard the current frame, allowing the existing skb to be > reused in the ring. Otherwise, we simplify the code flow and just hand > over the RX-populated skb over to mac80211. > > In addition, to fixing the kernel crash, the RX routine should now > generally behave better under low memory conditions. > > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053 > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com> > Reviewed-by: Daniel Drake <drake@endlessm.com> > Cc: <stable@vger.kernel.org> > --- > drivers/net/wireless/realtek/rtw88/pci.c | 28 +++++++++++------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > b/drivers/net/wireless/realtek/rtw88/pci.c > index cfe05ba7280d..1bfc99ae6b84 100644 > --- a/drivers/net/wireless/realtek/rtw88/pci.c > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > @@ -786,6 +786,15 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, > struct rtw_pci *rtwpci, > rx_desc = skb->data; > chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status); > > + /* discard current skb if the new skb cannot be allocated as a > + * new one in rx ring later > + * */ nit, comment indentation. > + new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE); > + if (WARN(!new, "rx routine starvation\n")) { > + new = skb; > + goto next_rp; > + } > + > /* offset from rx_desc to payload */ > pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz + > pkt_stat.shift; > @@ -803,25 +812,14 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, > struct rtw_pci *rtwpci, > skb_put(skb, pkt_stat.pkt_len); > skb_reserve(skb, pkt_offset); > > - /* alloc a smaller skb to mac80211 */ > - new = dev_alloc_skb(pkt_stat.pkt_len); > - if (!new) { > - new = skb; > - } else { > - skb_put_data(new, skb->data, skb->len); > - dev_kfree_skb_any(skb); > - } I am not sure if it's fine to deliver every huge SKB to mac80211. Because it will then be delivered to TCP/IP stack. Hence I think either it should be tested to know if the performance would be impacted or find out a more efficient way to send smaller SKB to mac80211 stack. > /* TODO: merge into rx.c */ > rtw_rx_stats(rtwdev, pkt_stat.vif, skb); > - memcpy(new->cb, &rx_status, sizeof(rx_status)); > - ieee80211_rx_irqsafe(rtwdev->hw, new); > + memcpy(skb->cb, &rx_status, sizeof(rx_status)); > + ieee80211_rx_irqsafe(rtwdev->hw, skb); > } > > - /* skb delivered to mac80211, alloc a new one in rx ring */ > - new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE); > - if (WARN(!new, "rx routine starvation\n")) > - return; > - > +next_rp: > + /* skb delivered to mac80211, attach the new one into rx ring */ > ring->buf[cur_rp] = new; > rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz); > -- Yan-Hsuan
Tony Chuang <yhchuang@realtek.com> 於 2019年7月8日 週一 下午3:23寫道： > > > Subject: [PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR > > nit, "rtw88: pci:" would be better. Ok. > > > > When skb allocation fails and the "rx routine starvation" is hit, the > > function returns immediately without updating the RX ring. At this > > point, the RX ring may continue referencing an old skb which was already > > handed off to ieee80211_rx_irqsafe(). When it comes to be used again, > > bad things happen. > > > > This patch allocates a new skb first in RX ISR. If we don't have memory > > available, we discard the current frame, allowing the existing skb to be > > reused in the ring. Otherwise, we simplify the code flow and just hand > > over the RX-populated skb over to mac80211. > > > > In addition, to fixing the kernel crash, the RX routine should now > > generally behave better under low memory conditions. > > > > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053 > > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com> > > Reviewed-by: Daniel Drake <drake@endlessm.com> > > Cc: <stable@vger.kernel.org> > > --- > > drivers/net/wireless/realtek/rtw88/pci.c | 28 +++++++++++------------- > > 1 file changed, 13 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > > b/drivers/net/wireless/realtek/rtw88/pci.c > > index cfe05ba7280d..1bfc99ae6b84 100644 > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > @@ -786,6 +786,15 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, > > struct rtw_pci *rtwpci, > > rx_desc = skb->data; > > chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status); > > > > + /* discard current skb if the new skb cannot be allocated as a > > + * new one in rx ring later > > + * */ > > nit, comment indentation. Thanks. I will fix this. > > + new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE); > > + if (WARN(!new, "rx routine starvation\n")) { > > + new = skb; > > + goto next_rp; > > + } > > + > > /* offset from rx_desc to payload */ > > pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz + > > pkt_stat.shift; > > @@ -803,25 +812,14 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, > > struct rtw_pci *rtwpci, > > skb_put(skb, pkt_stat.pkt_len); > > skb_reserve(skb, pkt_offset); > > > > - /* alloc a smaller skb to mac80211 */ > > - new = dev_alloc_skb(pkt_stat.pkt_len); > > - if (!new) { > > - new = skb; > > - } else { > > - skb_put_data(new, skb->data, skb->len); > > - dev_kfree_skb_any(skb); > > - } > > I am not sure if it's fine to deliver every huge SKB to mac80211. > Because it will then be delivered to TCP/IP stack. > Hence I think either it should be tested to know if the performance > would be impacted or find out a more efficient way to send > smaller SKB to mac80211 stack. I remember network stack only processes the skb with(in) pointers (skb->data) and the skb->len for data part. It also checks real buffer boundary (head and end) of the skb to prevent memory overflow. Therefore, I think using the original skb is the most efficient way. If I misunderstand something, please point out. > > /* TODO: merge into rx.c */ > > rtw_rx_stats(rtwdev, pkt_stat.vif, skb); > > - memcpy(new->cb, &rx_status, sizeof(rx_status)); > > - ieee80211_rx_irqsafe(rtwdev->hw, new); > > + memcpy(skb->cb, &rx_status, sizeof(rx_status)); > > + ieee80211_rx_irqsafe(rtwdev->hw, skb); > > } > > > > - /* skb delivered to mac80211, alloc a new one in rx ring */ > > - new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE); > > - if (WARN(!new, "rx routine starvation\n")) > > - return; > > - > > +next_rp: > > + /* skb delivered to mac80211, attach the new one into rx ring */ > > ring->buf[cur_rp] = new; > > rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz); > > > > -- > > Yan-Hsuan
From: Jian-Hong Pan > Sent: 08 July 2019 07:33 > To: Yan-Hsuan Chuang; Kalle Valo; David S . Miller > > Testing with RTL8822BE hardware, when available memory is low, we > frequently see a kernel panic and system freeze. > > First, rtw_pci_rx_isr encounters a memory allocation failure (trimmed): > > rx routine starvation > WARNING: CPU: 7 PID: 9871 at drivers/net/wireless/realtek/rtw88/pci.c:822 > rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci] > [ 2356.580313] RIP: 0010:rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci] > > Then we see a variety of different error conditions and kernel panics, > such as this one (trimmed): > > rtw_pci 0000:02:00.0: pci bus timeout, check dma status > skbuff: skb_over_panic: text:00000000091b6e66 len:415 put:415 head:00000000d2880c6f > data:000000007a02b1ea tail:0x1df end:0xc0 dev:<NULL> > ------------[ cut here ]------------ > kernel BUG at net/core/skbuff.c:105! > invalid opcode: 0000 [#1] SMP NOPTI > RIP: 0010:skb_panic+0x43/0x45 > > When skb allocation fails and the "rx routine starvation" is hit, the > function returns immediately without updating the RX ring. At this > point, the RX ring may continue referencing an old skb which was already > handed off to ieee80211_rx_irqsafe(). When it comes to be used again, > bad things happen. > > This patch allocates a new skb first in RX ISR. If we don't have memory > available, we discard the current frame, allowing the existing skb to be > reused in the ring. Otherwise, we simplify the code flow and just hand > over the RX-populated skb over to mac80211. > > In addition, to fixing the kernel crash, the RX routine should now > generally behave better under low memory conditions. Under low memory conditions it may be preferable to limit the amount of memory assigned to the receive ring. I also thought it was preferable (DM may correct me here) to do the skb allocates from the 'bh' of the driver rather than from the hardware interrupt. It is also almost certainly preferable (especially on IOMMU systems) to copy small frames into a new skb (of the right size) and then reuse the skb (with its dma-mapped buffer) for a later frame. Allocating a new skb before ay px processing just seems wrong... David > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053 > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com> > Reviewed-by: Daniel Drake <drake@endlessm.com> > Cc: <stable@vger.kernel.org> > --- > drivers/net/wireless/realtek/rtw88/pci.c | 28 +++++++++++------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c > index cfe05ba7280d..1bfc99ae6b84 100644 > --- a/drivers/net/wireless/realtek/rtw88/pci.c > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > @@ -786,6 +786,15 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, > rx_desc = skb->data; > chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status); > > + /* discard current skb if the new skb cannot be allocated as a > + * new one in rx ring later > + * */ > + new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE); > + if (WARN(!new, "rx routine starvation\n")) { > + new = skb; > + goto next_rp; > + } > + > /* offset from rx_desc to payload */ > pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz + > pkt_stat.shift; > @@ -803,25 +812,14 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, > skb_put(skb, pkt_stat.pkt_len); > skb_reserve(skb, pkt_offset); > > - /* alloc a smaller skb to mac80211 */ > - new = dev_alloc_skb(pkt_stat.pkt_len); > - if (!new) { > - new = skb; > - } else { > - skb_put_data(new, skb->data, skb->len); > - dev_kfree_skb_any(skb); > - } > /* TODO: merge into rx.c */ > rtw_rx_stats(rtwdev, pkt_stat.vif, skb); > - memcpy(new->cb, &rx_status, sizeof(rx_status)); > - ieee80211_rx_irqsafe(rtwdev->hw, new); > + memcpy(skb->cb, &rx_status, sizeof(rx_status)); > + ieee80211_rx_irqsafe(rtwdev->hw, skb); > } > > - /* skb delivered to mac80211, alloc a new one in rx ring */ > - new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE); > - if (WARN(!new, "rx routine starvation\n")) > - return; > - > +next_rp: > + /* skb delivered to mac80211, attach the new one into rx ring */ > ring->buf[cur_rp] = new; > rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz); > > -- > 2.22.0 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
> > > @@ -803,25 +812,14 @@ static void rtw_pci_rx_isr(struct rtw_dev > *rtwdev, > > > struct rtw_pci *rtwpci, > > > skb_put(skb, pkt_stat.pkt_len); > > > skb_reserve(skb, pkt_offset); > > > > > > - /* alloc a smaller skb to mac80211 */ > > > - new = dev_alloc_skb(pkt_stat.pkt_len); > > > - if (!new) { > > > - new = skb; > > > - } else { > > > - skb_put_data(new, skb->data, > skb->len); > > > - dev_kfree_skb_any(skb); > > > - } > > > > I am not sure if it's fine to deliver every huge SKB to mac80211. > > Because it will then be delivered to TCP/IP stack. > > Hence I think either it should be tested to know if the performance > > would be impacted or find out a more efficient way to send > > smaller SKB to mac80211 stack. > > I remember network stack only processes the skb with(in) pointers > (skb->data) and the skb->len for data part. It also checks real > buffer boundary (head and end) of the skb to prevent memory overflow. > Therefore, I think using the original skb is the most efficient way. > > If I misunderstand something, please point out. > It means if we still use a huge SKB (~8K) for every RX packet (~1.5K). There is about 6.5K not used. And even more if we ping with large packet size "eg. $ ping -s 65536", I am not sure if those huge SKBs will eat all of the SKB mem pool, and then ping fails. BTW, the original design of RTK_PCI_RX_BUF_SIZE to be (8192 + 24) is to receive AMSDU packet in one SKB. (Could probably enlarge it to RX VHT AMSDU ~11K) Yan-Hsuan
From: Tony Chuang > Sent: 08 July 2019 10:00 > > > > @@ -803,25 +812,14 @@ static void rtw_pci_rx_isr(struct rtw_dev > > *rtwdev, > > > > struct rtw_pci *rtwpci, > > > > skb_put(skb, pkt_stat.pkt_len); > > > > skb_reserve(skb, pkt_offset); > > > > > > > > - /* alloc a smaller skb to mac80211 */ > > > > - new = dev_alloc_skb(pkt_stat.pkt_len); > > > > - if (!new) { > > > > - new = skb; > > > > - } else { > > > > - skb_put_data(new, skb->data, > > skb->len); > > > > - dev_kfree_skb_any(skb); > > > > - } > > > > > > I am not sure if it's fine to deliver every huge SKB to mac80211. > > > Because it will then be delivered to TCP/IP stack. > > > Hence I think either it should be tested to know if the performance > > > would be impacted or find out a more efficient way to send > > > smaller SKB to mac80211 stack. > > > > I remember network stack only processes the skb with(in) pointers > > (skb->data) and the skb->len for data part. It also checks real > > buffer boundary (head and end) of the skb to prevent memory overflow. > > Therefore, I think using the original skb is the most efficient way. > > > > If I misunderstand something, please point out. > > > > It means if we still use a huge SKB (~8K) for every RX packet (~1.5K). > There is about 6.5K not used. And even more if we ping with large packet > size "eg. $ ping -s 65536", I am not sure if those huge SKBs will eat all of > the SKB mem pool, and then ping fails. > > BTW, the original design of RTK_PCI_RX_BUF_SIZE to be (8192 + 24) is to > receive AMSDU packet in one SKB. > (Could probably enlarge it to RX VHT AMSDU ~11K) If you allocate 8192+24 the memory allocated will be either 12k or 16k and the skb truesize set appropriately. (Probably 16k if dma memory.) If this is fed into IP it is quite likely that a single byte of data will end up queued on the socket in 16k of dma-able memory. The 'truesize' stops this using all the system memory, but it isn't good for memory usage. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 7/8/19 1:32 AM, Jian-Hong Pan wrote: > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c > index cfe05ba7280d..1bfc99ae6b84 100644 > --- a/drivers/net/wireless/realtek/rtw88/pci.c > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > @@ -786,6 +786,15 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, > rx_desc = skb->data; > chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status); > > + /* discard current skb if the new skb cannot be allocated as a > + * new one in rx ring later > + * */ > + new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE); > + if (WARN(!new, "rx routine starvation\n")) { > + new = skb; > + goto next_rp; This should probably be a WARN_ONCE() rather than WARN(), otherwise the logs will be flooded once this condition triggers. Larry
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index cfe05ba7280d..1bfc99ae6b84 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -786,6 +786,15 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, rx_desc = skb->data; chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status); + /* discard current skb if the new skb cannot be allocated as a + * new one in rx ring later + * */ + new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE); + if (WARN(!new, "rx routine starvation\n")) { + new = skb; + goto next_rp; + } + /* offset from rx_desc to payload */ pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz + pkt_stat.shift; @@ -803,25 +812,14 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, skb_put(skb, pkt_stat.pkt_len); skb_reserve(skb, pkt_offset); - /* alloc a smaller skb to mac80211 */ - new = dev_alloc_skb(pkt_stat.pkt_len); - if (!new) { - new = skb; - } else { - skb_put_data(new, skb->data, skb->len); - dev_kfree_skb_any(skb); - } /* TODO: merge into rx.c */ rtw_rx_stats(rtwdev, pkt_stat.vif, skb); - memcpy(new->cb, &rx_status, sizeof(rx_status)); - ieee80211_rx_irqsafe(rtwdev->hw, new); + memcpy(skb->cb, &rx_status, sizeof(rx_status)); + ieee80211_rx_irqsafe(rtwdev->hw, skb); } - /* skb delivered to mac80211, alloc a new one in rx ring */ - new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE); - if (WARN(!new, "rx routine starvation\n")) - return; - +next_rp: + /* skb delivered to mac80211, attach the new one into rx ring */ ring->buf[cur_rp] = new; rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz);