[PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR

[PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR

[Jian-Hong Pan]

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.

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

RE: [PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR

[Tony Chuang]

> 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

Re: [PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR

[Jian-Hong Pan]

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

RE: [PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR

[David Laight]

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)

RE: [PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR

[Tony Chuang]

> > > @@ -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

RE: [PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR

[David Laight]

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)

Re: [PATCH] rtw88/pci: Rearrange the memory usage for skb in RX ISR

[Larry Finger]

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

[PATCH v2 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[Jian-Hong Pan]

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, 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>
Cc: <stable@vger.kernel.org>
---
 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
+		 */
+		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)
-- 
2.22.0

[PATCH v2 2/2] rtw88: pci: Use DMA sync instead of remapping in RX ISR

[Jian-Hong Pan]

Since each skb in RX ring is reused instead of new allocation, we can
treat the DMA in a more efficient way by DMA synchronization.

Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
---
 drivers/net/wireless/realtek/rtw88/pci.c | 35 ++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index e9fe3ad896c8..28ca76f71dfe 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -206,6 +206,35 @@ static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
 	return 0;
 }
 
+static int rtw_pci_sync_rx_desc_cpu(struct rtw_dev *rtwdev, dma_addr_t dma)
+{
+	struct device *dev = rtwdev->dev;
+	int buf_sz = RTK_PCI_RX_BUF_SIZE;
+
+	dma_sync_single_for_cpu(dev, dma, buf_sz, PCI_DMA_FROMDEVICE);
+
+	return 0;
+}
+
+static int rtw_pci_sync_rx_desc_device(struct rtw_dev *rtwdev, dma_addr_t dma,
+				       struct rtw_pci_rx_ring *rx_ring,
+				       u32 idx, u32 desc_sz)
+{
+	struct device *dev = rtwdev->dev;
+	struct rtw_pci_rx_buffer_desc *buf_desc;
+	int buf_sz = RTK_PCI_RX_BUF_SIZE;
+
+	dma_sync_single_for_device(dev, dma, buf_sz, PCI_DMA_FROMDEVICE);
+
+	buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
+						     idx * desc_sz);
+	memset(buf_desc, 0, sizeof(*buf_desc));
+	buf_desc->buf_size = cpu_to_le16(RTK_PCI_RX_BUF_SIZE);
+	buf_desc->dma = cpu_to_le32(dma);
+
+	return 0;
+}
+
 static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
 				struct rtw_pci_rx_ring *rx_ring,
 				u8 desc_size, u32 len)
@@ -782,8 +811,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
 		rtw_pci_dma_check(rtwdev, ring, cur_rp);
 		skb = ring->buf[cur_rp];
 		dma = *((dma_addr_t *)skb->cb);
-		pci_unmap_single(rtwpci->pdev, dma, RTK_PCI_RX_BUF_SIZE,
-				 PCI_DMA_FROMDEVICE);
+		rtw_pci_sync_rx_desc_cpu(rtwdev, dma);
 		rx_desc = skb->data;
 		chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status);
 
@@ -818,7 +846,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
 
 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);
+		rtw_pci_sync_rx_desc_device(rtwdev, dma, ring, cur_rp,
+					    buf_desc_sz);
 
 		/* host read next element in ring */
 		if (++cur_rp >= ring->r.len)
-- 
2.22.0

Re: [PATCH v2 2/2] rtw88: pci: Use DMA sync instead of remapping in RX ISR

[Christoph Hellwig]

On Tue, Jul 09, 2019 at 06:21:01PM +0800, Jian-Hong Pan wrote:
> Since each skb in RX ring is reused instead of new allocation, we can
> treat the DMA in a more efficient way by DMA synchronization.
> 
> Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> ---
>  drivers/net/wireless/realtek/rtw88/pci.c | 35 ++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index e9fe3ad896c8..28ca76f71dfe 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -206,6 +206,35 @@ static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static int rtw_pci_sync_rx_desc_cpu(struct rtw_dev *rtwdev, dma_addr_t dma)
> +{
> +	struct device *dev = rtwdev->dev;
> +	int buf_sz = RTK_PCI_RX_BUF_SIZE;
> +
> +	dma_sync_single_for_cpu(dev, dma, buf_sz, PCI_DMA_FROMDEVICE);
> +
> +	return 0;
> +}

No need to return a value from this helper. In fact I'm not even sure
you need the helper at all.  Also please use the DMA_FROM_DEVICE
constant instead of the deprecated PCI variant.

> +static int rtw_pci_sync_rx_desc_device(struct rtw_dev *rtwdev, dma_addr_t dma,
> +				       struct rtw_pci_rx_ring *rx_ring,
> +				       u32 idx, u32 desc_sz)
> +{
> +	struct device *dev = rtwdev->dev;
> +	struct rtw_pci_rx_buffer_desc *buf_desc;
> +	int buf_sz = RTK_PCI_RX_BUF_SIZE;
> +
> +	dma_sync_single_for_device(dev, dma, buf_sz, PCI_DMA_FROMDEVICE);
> +
> +	buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> +						     idx * desc_sz);
> +	memset(buf_desc, 0, sizeof(*buf_desc));
> +	buf_desc->buf_size = cpu_to_le16(RTK_PCI_RX_BUF_SIZE);
> +	buf_desc->dma = cpu_to_le32(dma);
> +
> +	return 0;
> +}

Same comment on the PCI constant and the return value here.

RE: [PATCH v2 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[Tony Chuang]

> Subject: [PATCH v2 1/2] rtw88: pci: Rearrange the memory usage for skb in
> RX ISR
> 
> 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, 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>
> Cc: <stable@vger.kernel.org>
> ---
>  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
> +		 */
> +		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)
> --
> 2.22.0

Now it looks good to me. Thanks.

Acked-by: Yan-Hsuan Chuang <yhchuang@realtek.com>

Yan-Hsuan

[PATCH v3 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[Jian-Hong Pan]

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(-)

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)
-- 
2.22.0

[PATCH v3 2/2] rtw88: pci: Use DMA sync instead of remapping in RX ISR

[Jian-Hong Pan]

Since each skb in RX ring is reused instead of new allocation, we can
treat the DMA in a more efficient way by DMA synchronization.

Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
Cc: <stable@vger.kernel.org>
---
v2:
 - New patch by following [PATCH v3 1/2] rtw88: pci: Rearrange the
   memory usage for skb in RX ISR.

v3:
 - Remove rtw_pci_sync_rx_desc_cpu and call dma_sync_single_for_cpu in
   rtw_pci_rx_isr directly.
 - Remove the return value of rtw_pci_sync_rx_desc_device.
 - Use DMA_FROM_DEVICE instead of PCI_DMA_FROMDEVICE.

 drivers/net/wireless/realtek/rtw88/pci.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index e9fe3ad896c8..485d30c06935 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -206,6 +206,23 @@ static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
 	return 0;
 }
 
+static void rtw_pci_sync_rx_desc_device(struct rtw_dev *rtwdev, dma_addr_t dma,
+					struct rtw_pci_rx_ring *rx_ring,
+					u32 idx, u32 desc_sz)
+{
+	struct device *dev = rtwdev->dev;
+	struct rtw_pci_rx_buffer_desc *buf_desc;
+	int buf_sz = RTK_PCI_RX_BUF_SIZE;
+
+	dma_sync_single_for_device(dev, dma, buf_sz, DMA_FROM_DEVICE);
+
+	buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
+						     idx * desc_sz);
+	memset(buf_desc, 0, sizeof(*buf_desc));
+	buf_desc->buf_size = cpu_to_le16(RTK_PCI_RX_BUF_SIZE);
+	buf_desc->dma = cpu_to_le32(dma);
+}
+
 static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
 				struct rtw_pci_rx_ring *rx_ring,
 				u8 desc_size, u32 len)
@@ -782,8 +799,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
 		rtw_pci_dma_check(rtwdev, ring, cur_rp);
 		skb = ring->buf[cur_rp];
 		dma = *((dma_addr_t *)skb->cb);
-		pci_unmap_single(rtwpci->pdev, dma, RTK_PCI_RX_BUF_SIZE,
-				 PCI_DMA_FROMDEVICE);
+		dma_sync_single_for_cpu(rtwdev->dev, dma, RTK_PCI_RX_BUF_SIZE,
+					DMA_FROM_DEVICE);
 		rx_desc = skb->data;
 		chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status);
 
@@ -818,7 +835,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
 
 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);
+		rtw_pci_sync_rx_desc_device(rtwdev, dma, ring, cur_rp,
+					    buf_desc_sz);
 
 		/* host read next element in ring */
 		if (++cur_rp >= ring->r.len)
-- 
2.22.0

Re: [PATCH v2 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[Jian-Hong Pan]

Tony Chuang <yhchuang@realtek.com> 於 2019年7月10日 週三 下午4:37寫道：
>
> > Subject: [PATCH v2 1/2] rtw88: pci: Rearrange the memory usage for skb in
> > RX ISR
> >
> > 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, 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>
> > Cc: <stable@vger.kernel.org>
> > ---
> >  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
> > +              */
> > +             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)
> > --
> > 2.22.0
>
> Now it looks good to me. Thanks.
>
> Acked-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Yan-Hsuan

Uh!  Thanks for your ack.
But I just sent version 3 patches (including [PATCH v3 2/2] rtw88:
pci: Use DMA sync instead of remapping in RX ISR) by following
Christoph's comment. [1]

Could you please also review the 2 patches of version 3?  Thank you.

[1]: https://lkml.org/lkml/2019/7/9/507

Jian-Hong Pan

RE: [PATCH v3 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[David Laight]

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)

Re: [PATCH v3 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[Jian-Hong Pan]

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)
>

[PATCH v4 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[Jian-Hong Pan]

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, 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>
Cc: <stable@vger.kernel.org>
---
 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..c415f5e94fed 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);
+		/* allocate a new skb for this frame,
+		 * discard the frame 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"))
+			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

[PATCH v4 2/2] rtw88: pci: Use DMA sync instead of remapping in RX ISR

[Jian-Hong Pan]

Since each skb in RX ring is reused instead of new allocation, we can
treat the DMA in a more efficient way by DMA synchronization.

Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
Cc: <stable@vger.kernel.org>
---
 drivers/net/wireless/realtek/rtw88/pci.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index c415f5e94fed..68fae52151dd 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -206,6 +206,23 @@ static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
 	return 0;
 }
 
+static void rtw_pci_sync_rx_desc_device(struct rtw_dev *rtwdev, dma_addr_t dma,
+					struct rtw_pci_rx_ring *rx_ring,
+					u32 idx, u32 desc_sz)
+{
+	struct device *dev = rtwdev->dev;
+	struct rtw_pci_rx_buffer_desc *buf_desc;
+	int buf_sz = RTK_PCI_RX_BUF_SIZE;
+
+	dma_sync_single_for_device(dev, dma, buf_sz, DMA_FROM_DEVICE);
+
+	buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
+						     idx * desc_sz);
+	memset(buf_desc, 0, sizeof(*buf_desc));
+	buf_desc->buf_size = cpu_to_le16(RTK_PCI_RX_BUF_SIZE);
+	buf_desc->dma = cpu_to_le32(dma);
+}
+
 static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
 				struct rtw_pci_rx_ring *rx_ring,
 				u8 desc_size, u32 len)
@@ -782,8 +799,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
 		rtw_pci_dma_check(rtwdev, ring, cur_rp);
 		skb = ring->buf[cur_rp];
 		dma = *((dma_addr_t *)skb->cb);
-		pci_unmap_single(rtwpci->pdev, dma, RTK_PCI_RX_BUF_SIZE,
-				 PCI_DMA_FROMDEVICE);
+		dma_sync_single_for_cpu(rtwdev->dev, dma, RTK_PCI_RX_BUF_SIZE,
+					DMA_FROM_DEVICE);
 		rx_desc = skb->data;
 		chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status);
 
@@ -818,7 +835,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
 
 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);
+		rtw_pci_sync_rx_desc_device(rtwdev, dma, ring, cur_rp,
+					    buf_desc_sz);
 
 		/* host read next element in ring */
 		if (++cur_rp >= ring->r.len)
-- 
2.22.0

Re: [PATCH v4 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[Jian-Hong Pan]

Jian-Hong Pan <jian-hong@endlessm.com> 於 2019年7月11日 週四 下午1:25寫道：
>
> 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, 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>
> Cc: <stable@vger.kernel.org>
> ---

Sorry, I forget to place the version difference here.

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.

v4:
 - Fix comment: allocate a new skb for this frame, discard the frame
if none available

>  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..c415f5e94fed 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);
> +               /* allocate a new skb for this frame,
> +                * discard the frame 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"))
> +                       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
>

Re: [PATCH v4 2/2] rtw88: pci: Use DMA sync instead of remapping in RX ISR

[Jian-Hong Pan]

Jian-Hong Pan <jian-hong@endlessm.com> 於 2019年7月11日 週四 下午1:25寫道：
>
> Since each skb in RX ring is reused instead of new allocation, we can
> treat the DMA in a more efficient way by DMA synchronization.
>
> Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> Cc: <stable@vger.kernel.org>
> ---

Sorry, also forget to place the version difference here

v2:
 - New patch by following [PATCH v3 1/2] rtw88: pci: Rearrange the
   memory usage for skb in RX ISR.

v3:
 - Remove rtw_pci_sync_rx_desc_cpu and call dma_sync_single_for_cpu in
   rtw_pci_rx_isr directly.
 - Remove the return value of rtw_pci_sync_rx_desc_device.
 - Use DMA_FROM_DEVICE instead of PCI_DMA_FROMDEVICE.

v4:
 - Same as v3.

>  drivers/net/wireless/realtek/rtw88/pci.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index c415f5e94fed..68fae52151dd 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -206,6 +206,23 @@ static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
>         return 0;
>  }
>
> +static void rtw_pci_sync_rx_desc_device(struct rtw_dev *rtwdev, dma_addr_t dma,
> +                                       struct rtw_pci_rx_ring *rx_ring,
> +                                       u32 idx, u32 desc_sz)
> +{
> +       struct device *dev = rtwdev->dev;
> +       struct rtw_pci_rx_buffer_desc *buf_desc;
> +       int buf_sz = RTK_PCI_RX_BUF_SIZE;
> +
> +       dma_sync_single_for_device(dev, dma, buf_sz, DMA_FROM_DEVICE);
> +
> +       buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> +                                                    idx * desc_sz);
> +       memset(buf_desc, 0, sizeof(*buf_desc));
> +       buf_desc->buf_size = cpu_to_le16(RTK_PCI_RX_BUF_SIZE);
> +       buf_desc->dma = cpu_to_le32(dma);
> +}
> +
>  static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
>                                 struct rtw_pci_rx_ring *rx_ring,
>                                 u8 desc_size, u32 len)
> @@ -782,8 +799,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
>                 rtw_pci_dma_check(rtwdev, ring, cur_rp);
>                 skb = ring->buf[cur_rp];
>                 dma = *((dma_addr_t *)skb->cb);
> -               pci_unmap_single(rtwpci->pdev, dma, RTK_PCI_RX_BUF_SIZE,
> -                                PCI_DMA_FROMDEVICE);
> +               dma_sync_single_for_cpu(rtwdev->dev, dma, RTK_PCI_RX_BUF_SIZE,
> +                                       DMA_FROM_DEVICE);
>                 rx_desc = skb->data;
>                 chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status);
>
> @@ -818,7 +835,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
>
>  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);
> +               rtw_pci_sync_rx_desc_device(rtwdev, dma, ring, cur_rp,
> +                                           buf_desc_sz);
>
>                 /* host read next element in ring */
>                 if (++cur_rp >= ring->r.len)
> --
> 2.22.0
>

Re: [PATCH v4 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[Jian-Hong Pan]

Jian-Hong Pan <jian-hong@endlessm.com> 於 2019年7月11日 週四 下午1:28寫道：
>
> Jian-Hong Pan <jian-hong@endlessm.com> 於 2019年7月11日 週四 下午1:25寫道：
> >
> > 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, 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>
> > Cc: <stable@vger.kernel.org>
> > ---
>
> Sorry, I forget to place the version difference here.
>
> 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.
>
> v4:
>  - Fix comment: allocate a new skb for this frame, discard the frame
> if none available
>
> >  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..c415f5e94fed 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);
> > +               /* allocate a new skb for this frame,
> > +                * discard the frame 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"))
> > +                       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
> >

Gentle ping!  Any comment for this patch set (v4) will be appreciated.

Jian-Hong Pan

Re: [PATCH v4 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[Kalle Valo]

Jian-Hong Pan <jian-hong@endlessm.com> wrote:

> 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, 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>
> Cc: <stable@vger.kernel.org>

2 patches applied to wireless-drivers-next.git, thanks.

ee6db78f5db9 rtw88: pci: Rearrange the memory usage for skb in RX ISR
29b68a920f6a rtw88: pci: Use DMA sync instead of remapping in RX ISR

-- 
https://patchwork.kernel.org/patch/11039275/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v3 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR

[Brian Norris]

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