From: Jian-Hong Pan <jian-hong@endlessm.com>
To: David Laight <David.Laight@aculab.com>
Cc: Yan-Hsuan Chuang <yhchuang@realtek.com>,
Kalle Valo <kvalo@codeaurora.org>,
"David S . Miller" <davem@davemloft.net>,
Larry Finger <Larry.Finger@lwfinger.net>,
Christoph Hellwig <hch@infradead.org>,
"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux@endlessm.com" <linux@endlessm.com>,
Daniel Drake <drake@endlessm.com>,
"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH v3 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR
Date: Thu, 11 Jul 2019 11:50:50 +0800 [thread overview]
Message-ID: <CAPpJ_edDcaBq+0DocPmS-yYM10B4MkWvBn=f6wwbYdqzSGmp_g@mail.gmail.com> (raw)
In-Reply-To: <81a2b91c4b084617bab8656fca932f6d@AcuMS.aculab.com>
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)
>
next prev parent reply other threads:[~2019-07-11 3:51 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-08 6:32 [PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR Jian-Hong Pan
2019-07-08 7:23 ` Tony Chuang
2019-07-08 8:07 ` Jian-Hong Pan
2019-07-08 9:00 ` Tony Chuang
2019-07-08 9:18 ` David Laight
2019-07-08 8:36 ` David Laight
2019-07-08 18:01 ` Larry Finger
2019-07-09 10:20 ` [PATCH v2 1/2] rtw88: pci: " Jian-Hong Pan
2019-07-10 8:36 ` Tony Chuang
2019-07-10 8:54 ` Jian-Hong Pan
2019-07-09 10:21 ` [PATCH v2 2/2] rtw88: pci: Use DMA sync instead of remapping " Jian-Hong Pan
2019-07-09 16:15 ` Christoph Hellwig
2019-07-10 8:38 ` [PATCH v3 1/2] rtw88: pci: Rearrange the memory usage for skb " Jian-Hong Pan
2019-07-10 8:57 ` David Laight
2019-07-11 3:50 ` Jian-Hong Pan [this message]
2019-07-11 5:24 ` [PATCH v4 " Jian-Hong Pan
2019-07-11 5:24 ` [PATCH v4 2/2] rtw88: pci: Use DMA sync instead of remapping " Jian-Hong Pan
2019-07-11 5:30 ` Jian-Hong Pan
2019-07-11 5:28 ` [PATCH v4 1/2] rtw88: pci: Rearrange the memory usage for skb " Jian-Hong Pan
2019-07-24 6:13 ` Jian-Hong Pan
2019-07-24 11:49 ` Kalle Valo
2019-08-15 20:25 ` [PATCH v3 " Brian Norris
2019-07-10 8:38 ` [PATCH v3 2/2] rtw88: pci: Use DMA sync instead of remapping " Jian-Hong Pan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPpJ_edDcaBq+0DocPmS-yYM10B4MkWvBn=f6wwbYdqzSGmp_g@mail.gmail.com' \
--to=jian-hong@endlessm.com \
--cc=David.Laight@aculab.com \
--cc=Larry.Finger@lwfinger.net \
--cc=davem@davemloft.net \
--cc=drake@endlessm.com \
--cc=hch@infradead.org \
--cc=kvalo@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=linux@endlessm.com \
--cc=netdev@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=yhchuang@realtek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).