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