| Message ID | 20190710083825.7115-1-jian-hong@endlessm.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v3,1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR | expand |
From: Jian-Hong Pan > Sent: 10 July 2019 09:38 > > 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, data-sized skb first in RX ISR. After > copying the data in, we pass it to the upper layers. However, if skb > allocation fails, we effectively drop the frame. In both cases, the > original, full size ring skb is reused. > > In addition, by fixing the kernel crash, the RX routine should now > generally behave better under low memory conditions. A couple of minor nits (see below). You may want to do a followup patch that changes the rx buffers (used by the hardware) to by just memory buffers. Nothing (probably) relies on them being skb with all the accociated baggage. David > > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053 > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com> > Cc: <stable@vger.kernel.org> > --- > v2: > - Allocate new data-sized skb and put data into it, then pass it to > mac80211. Reuse the original skb in RX ring by DMA sync. > - Modify the commit message. > - Introduce following [PATCH v3 2/2] rtw88: pci: Use DMA sync instead > of remapping in RX ISR. > > v3: > - Same as v2. > > drivers/net/wireless/realtek/rtw88/pci.c | 49 +++++++++++------------- > 1 file changed, 22 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c > index cfe05ba7280d..e9fe3ad896c8 100644 > --- a/drivers/net/wireless/realtek/rtw88/pci.c > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > @@ -763,6 +763,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, > u32 pkt_offset; > u32 pkt_desc_sz = chip->rx_pkt_desc_sz; > u32 buf_desc_sz = chip->rx_buf_desc_sz; > + u32 new_len; > u8 *rx_desc; > dma_addr_t dma; > > @@ -790,40 +791,34 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, > pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz + > pkt_stat.shift; > > - if (pkt_stat.is_c2h) { > - /* keep rx_desc, halmac needs it */ > - skb_put(skb, pkt_stat.pkt_len + pkt_offset); > + /* discard current skb if the new skb cannot be allocated as a > + * new one in rx ring later > + */ That comment isn't quite right. maybe: "Allocate a new skb for this frame, discard if none available" > + new_len = pkt_stat.pkt_len + pkt_offset; > + new = dev_alloc_skb(new_len); > + if (WARN_ONCE(!new, "rx routine starvation\n")) I think you should count these?? > + goto next_rp; > + > + /* put the DMA data including rx_desc from phy to new skb */ > + skb_put_data(new, skb->data, new_len); > > - /* pass offset for further operation */ > - *((u32 *)skb->cb) = pkt_offset; > - skb_queue_tail(&rtwdev->c2h_queue, skb); > + if (pkt_stat.is_c2h) { > + /* pass rx_desc & offset for further operation */ > + *((u32 *)new->cb) = pkt_offset; > + skb_queue_tail(&rtwdev->c2h_queue, new); > ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work); > } else { > - /* remove rx_desc, maybe use skb_pull? */ > - 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); > + /* remove rx_desc */ > + skb_pull(new, pkt_offset); > + > + rtw_rx_stats(rtwdev, pkt_stat.vif, new); > memcpy(new->cb, &rx_status, sizeof(rx_status)); > ieee80211_rx_irqsafe(rtwdev->hw, new); > } > > - /* 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; > - > - ring->buf[cur_rp] = new; > - rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz); > +next_rp: > + /* new skb delivered to mac80211, re-enable original skb DMA */ > + rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz); > > /* host read next element in ring */ > if (++cur_rp >= ring->r.len) > -- > 2.22.0 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
David Laight <David.Laight@aculab.com> 於 2019年7月10日 週三 下午4:57寫道： > > From: Jian-Hong Pan > > Sent: 10 July 2019 09:38 > > > > 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, data-sized skb first in RX ISR. After > > copying the data in, we pass it to the upper layers. However, if skb > > allocation fails, we effectively drop the frame. In both cases, the > > original, full size ring skb is reused. > > > > In addition, by fixing the kernel crash, the RX routine should now > > generally behave better under low memory conditions. > > A couple of minor nits (see below). > You may want to do a followup patch that changes the rx buffers > (used by the hardware) to by just memory buffers. > Nothing (probably) relies on them being skb with all the accociated > baggage. It is a good idea for later commit. > David > > > > > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053 > > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com> > > Cc: <stable@vger.kernel.org> > > --- > > v2: > > - Allocate new data-sized skb and put data into it, then pass it to > > mac80211. Reuse the original skb in RX ring by DMA sync. > > - Modify the commit message. > > - Introduce following [PATCH v3 2/2] rtw88: pci: Use DMA sync instead > > of remapping in RX ISR. > > > > v3: > > - Same as v2. > > > > drivers/net/wireless/realtek/rtw88/pci.c | 49 +++++++++++------------- > > 1 file changed, 22 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c > > index cfe05ba7280d..e9fe3ad896c8 100644 > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > @@ -763,6 +763,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, > > u32 pkt_offset; > > u32 pkt_desc_sz = chip->rx_pkt_desc_sz; > > u32 buf_desc_sz = chip->rx_buf_desc_sz; > > + u32 new_len; > > u8 *rx_desc; > > dma_addr_t dma; > > > > @@ -790,40 +791,34 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, > > pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz + > > pkt_stat.shift; > > > > - if (pkt_stat.is_c2h) { > > - /* keep rx_desc, halmac needs it */ > > - skb_put(skb, pkt_stat.pkt_len + pkt_offset); > > + /* discard current skb if the new skb cannot be allocated as a > > + * new one in rx ring later > > + */ > > That comment isn't quite right. > maybe: "Allocate a new skb for this frame, discard if none available" Thanks! I will tweak it. > > + new_len = pkt_stat.pkt_len + pkt_offset; > > + new = dev_alloc_skb(new_len); > > + if (WARN_ONCE(!new, "rx routine starvation\n")) > > I think you should count these?? Larry has a different idea here. [1] I agree with Larry that just need to know not enough memory here. [1] https://lkml.org/lkml/2019/7/8/1049 Jian-Hong Pan > > + goto next_rp; > > + > > + /* put the DMA data including rx_desc from phy to new skb */ > > + skb_put_data(new, skb->data, new_len); > > > > - /* pass offset for further operation */ > > - *((u32 *)skb->cb) = pkt_offset; > > - skb_queue_tail(&rtwdev->c2h_queue, skb); > > + if (pkt_stat.is_c2h) { > > + /* pass rx_desc & offset for further operation */ > > + *((u32 *)new->cb) = pkt_offset; > > + skb_queue_tail(&rtwdev->c2h_queue, new); > > ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work); > > } else { > > - /* remove rx_desc, maybe use skb_pull? */ > > - 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); > > + /* remove rx_desc */ > > + skb_pull(new, pkt_offset); > > + > > + rtw_rx_stats(rtwdev, pkt_stat.vif, new); > > memcpy(new->cb, &rx_status, sizeof(rx_status)); > > ieee80211_rx_irqsafe(rtwdev->hw, new); > > } > > > > - /* 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; > > - > > - ring->buf[cur_rp] = new; > > - rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz); > > +next_rp: > > + /* new skb delivered to mac80211, re-enable original skb DMA */ > > + rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz); > > > > /* host read next element in ring */ > > if (++cur_rp >= ring->r.len) > > -- > > 2.22.0 > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
Hi all, I realize this already is merged, and it had some previous review comments that led to the decisions in this patch, but I'd still like to ask here, where I think I'm reaching the relevant parties: On Wed, Jul 10, 2019 at 1:43 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote: ... > This patch allocates a new, data-sized skb first in RX ISR. After > copying the data in, we pass it to the upper layers. However, if skb > allocation fails, we effectively drop the frame. In both cases, the > original, full size ring skb is reused. > > In addition, by 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> > Cc: <stable@vger.kernel.org> > --- > v2: > - Allocate new data-sized skb and put data into it, then pass it to > mac80211. Reuse the original skb in RX ring by DMA sync. Is it really wise to force an extra memcpy() for *every* delivery? Isn't there some other strategy that could be used to properly handle low-memory scenarios while still passing the original buffer up to higher layers most of the time? Or is it really so bad to keep re-allocating RTK_PCI_RX_BUF_SIZE (>8KB) of contiguous memory, to re-fill the RX ring? And if that is so bad, can we reduce the requirement for contiguous memory instead? (e.g., keep with smaller buffers, and perform aggregation / scatter-gather only for frames that are really larger?) Anyway, that's mostly a long-term thought, as this patch is good for fixing the important memory errors, even if it's not necessarily the ideal solution. Regards, Brian
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index cfe05ba7280d..e9fe3ad896c8 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -763,6 +763,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, u32 pkt_offset; u32 pkt_desc_sz = chip->rx_pkt_desc_sz; u32 buf_desc_sz = chip->rx_buf_desc_sz; + u32 new_len; u8 *rx_desc; dma_addr_t dma; @@ -790,40 +791,34 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci, pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz + pkt_stat.shift; - if (pkt_stat.is_c2h) { - /* keep rx_desc, halmac needs it */ - skb_put(skb, pkt_stat.pkt_len + pkt_offset); + /* discard current skb if the new skb cannot be allocated as a + * new one in rx ring later + */ + new_len = pkt_stat.pkt_len + pkt_offset; + new = dev_alloc_skb(new_len); + if (WARN_ONCE(!new, "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); - /* pass offset for further operation */ - *((u32 *)skb->cb) = pkt_offset; - skb_queue_tail(&rtwdev->c2h_queue, skb); + if (pkt_stat.is_c2h) { + /* pass rx_desc & offset for further operation */ + *((u32 *)new->cb) = pkt_offset; + skb_queue_tail(&rtwdev->c2h_queue, new); ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work); } else { - /* remove rx_desc, maybe use skb_pull? */ - 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); + /* remove rx_desc */ + skb_pull(new, pkt_offset); + + rtw_rx_stats(rtwdev, pkt_stat.vif, new); memcpy(new->cb, &rx_status, sizeof(rx_status)); ieee80211_rx_irqsafe(rtwdev->hw, new); } - /* 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; - - ring->buf[cur_rp] = new; - rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz); +next_rp: + /* new skb delivered to mac80211, re-enable original skb DMA */ + rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz); /* host read next element in ring */ if (++cur_rp >= ring->r.len)
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, data-sized skb first in RX ISR. After copying the data in, we pass it to the upper layers. However, if skb allocation fails, we effectively drop the frame. In both cases, the original, full size ring skb is reused. In addition, by 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> Cc: <stable@vger.kernel.org> --- v2: - Allocate new data-sized skb and put data into it, then pass it to mac80211. Reuse the original skb in RX ring by DMA sync. - Modify the commit message. - Introduce following [PATCH v3 2/2] rtw88: pci: Use DMA sync instead of remapping in RX ISR. v3: - Same as v2. drivers/net/wireless/realtek/rtw88/pci.c | 49 +++++++++++------------- 1 file changed, 22 insertions(+), 27 deletions(-)