| Message ID | 1419996787-17395-1-git-send-email-Larry.Finger@lwfinger.net (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kalle Valo |
| Headers | show |
On Tue, Dec 30, 2014 at 09:33:07PM -0600, Larry Finger wrote:
> v3 - Unmap skb before trying to allocate a new one so as to not leak mapping.
Looks good to me, although I'm not sure about the handling of DMA mapping errors
(perhaps that's something that drivers typically don't even try to handle?).
Anyway, the skb allocation issue appears to be resolved now. I am running your
patch with an extra hack to inject some occasional skb allocation failures, and
I haven't noticed any problems except dropped packets.
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/30/2014 11:07 PM, Eric Biggers wrote: > On Tue, Dec 30, 2014 at 09:33:07PM -0600, Larry Finger wrote: >> v3 - Unmap skb before trying to allocate a new one so as to not leak mapping. > > Looks good to me, although I'm not sure about the handling of DMA mapping errors > (perhaps that's something that drivers typically don't even try to handle?). > Anyway, the skb allocation issue appears to be resolved now. I am running your > patch with an extra hack to inject some occasional skb allocation failures, and > I haven't noticed any problems except dropped packets. The last time I saw any DMA mapping errors were for some early BCM43xx cards that only had 20 bits of DMA addressing space. These Realtek devices have a full 32 bits of addressing, thus any physical address in the first 4GB of RAM will be OK. I suppose that it might be possible to get a physical address outside this range for machines with a lot of RAM, but they are unlikely to have wifi interfaces. Thanks for the testing. The Realtek engineer told me that they are looking at this section, and may do a rewrite. I'm waiting to see what happens there before considering alternatives. If the number of packets dropped due to skb allocation failures is small, then the current code is likely OK. Larry -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Larry Finger <Larry.Finger@lwfinger.net> writes: > These drivers use 9100-byte receive buffers, thus allocating an skb requires > an O(3) memory allocation. Under heavy memory loads and fragmentation, such > a request can fail. Previous versions of the driver have dropped the packet > and reused the old buffer; however, the new version introduced a bug in that > it released the old buffer before trying to allocate a new one. The previous > method is implemented here. The skb is unmapped before any attempt is made to > allocate another. > > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> > Cc: Stable <stable@vger.kernel.org> [v3.18] > Reported-by: Eric Biggers <ebiggers3@gmail.com> > Cc: Eric Biggers <ebiggers3@gmail.com> Thanks, applied to wireless-drivers.git.
On Wed, 2014-12-31 at 15:10 -0600, Larry Finger wrote: > On 12/30/2014 11:07 PM, Eric Biggers wrote: > > On Tue, Dec 30, 2014 at 09:33:07PM -0600, Larry Finger wrote: > >> v3 - Unmap skb before trying to allocate a new one so as to not leak mapping. > > > > Looks good to me, although I'm not sure about the handling of DMA mapping errors > > (perhaps that's something that drivers typically don't even try to handle?). > > Anyway, the skb allocation issue appears to be resolved now. I am running your > > patch with an extra hack to inject some occasional skb allocation failures, and > > I haven't noticed any problems except dropped packets. > > The last time I saw any DMA mapping errors were for some early BCM43xx cards > that only had 20 bits of DMA addressing space. These Realtek devices have a full > 32 bits of addressing, thus any physical address in the first 4GB of RAM will be > OK. I suppose that it might be possible to get a physical address outside this > range for machines with a lot of RAM, but they are unlikely to have wifi interfaces. [...] Really, you don't think laptops and desktops come with 4GB RAM or more? Besides that, DMA mapping may involve calling an IOMMU driver that may fail for some reason. Drivers need to check for DMA mapping errors, it is not optional. Ben.
Index: wireless-drivers/drivers/net/wireless/rtlwifi/pci.c =================================================================== --- wireless-drivers.orig/drivers/net/wireless/rtlwifi/pci.c +++ wireless-drivers/drivers/net/wireless/rtlwifi/pci.c @@ -666,7 +666,8 @@ tx_status_ok: } static int _rtl_pci_init_one_rxdesc(struct ieee80211_hw *hw, - u8 *entry, int rxring_idx, int desc_idx) + struct sk_buff *new_skb, u8 *entry, + int rxring_idx, int desc_idx) { struct rtl_priv *rtlpriv = rtl_priv(hw); struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw)); @@ -674,11 +675,15 @@ static int _rtl_pci_init_one_rxdesc(stru u8 tmp_one = 1; struct sk_buff *skb; + if (likely(new_skb)) { + skb = new_skb; + goto remap; + } skb = dev_alloc_skb(rtlpci->rxbuffersize); if (!skb) return 0; - rtlpci->rx_ring[rxring_idx].rx_buf[desc_idx] = skb; +remap: /* just set skb->cb to mapping addr for pci_unmap_single use */ *((dma_addr_t *)skb->cb) = pci_map_single(rtlpci->pdev, skb_tail_pointer(skb), @@ -686,6 +691,7 @@ static int _rtl_pci_init_one_rxdesc(stru bufferaddress = *((dma_addr_t *)skb->cb); if (pci_dma_mapping_error(rtlpci->pdev, bufferaddress)) return 0; + rtlpci->rx_ring[rxring_idx].rx_buf[desc_idx] = skb; if (rtlpriv->use_new_trx_flow) { rtlpriv->cfg->ops->set_desc(hw, (u8 *)entry, false, HW_DESC_RX_PREPARE, @@ -781,6 +787,7 @@ static void _rtl_pci_rx_interrupt(struct /*rx pkt */ struct sk_buff *skb = rtlpci->rx_ring[rxring_idx].rx_buf[ rtlpci->rx_ring[rxring_idx].idx]; + struct sk_buff *new_skb; if (rtlpriv->use_new_trx_flow) { rx_remained_cnt = @@ -807,6 +814,13 @@ static void _rtl_pci_rx_interrupt(struct pci_unmap_single(rtlpci->pdev, *((dma_addr_t *)skb->cb), rtlpci->rxbuffersize, PCI_DMA_FROMDEVICE); + /* get a new skb - if fail, old one will be reused */ + new_skb = dev_alloc_skb(rtlpci->rxbuffersize); + if (unlikely(!new_skb)) { + pr_err("Allocation of new skb failed in %s\n", + __func__); + goto no_new; + } if (rtlpriv->use_new_trx_flow) { buffer_desc = &rtlpci->rx_ring[rxring_idx].buffer_desc @@ -911,14 +925,16 @@ static void _rtl_pci_rx_interrupt(struct schedule_work(&rtlpriv->works.lps_change_work); } end: + skb = new_skb; +no_new: if (rtlpriv->use_new_trx_flow) { - _rtl_pci_init_one_rxdesc(hw, (u8 *)buffer_desc, + _rtl_pci_init_one_rxdesc(hw, skb, (u8 *)buffer_desc, rxring_idx, - rtlpci->rx_ring[rxring_idx].idx); + rtlpci->rx_ring[rxring_idx].idx); } else { - _rtl_pci_init_one_rxdesc(hw, (u8 *)pdesc, rxring_idx, + _rtl_pci_init_one_rxdesc(hw, skb, (u8 *)pdesc, + rxring_idx, rtlpci->rx_ring[rxring_idx].idx); - if (rtlpci->rx_ring[rxring_idx].idx == rtlpci->rxringcount - 1) rtlpriv->cfg->ops->set_desc(hw, (u8 *)pdesc, @@ -1307,7 +1323,7 @@ static int _rtl_pci_init_rx_ring(struct rtlpci->rx_ring[rxring_idx].idx = 0; for (i = 0; i < rtlpci->rxringcount; i++) { entry = &rtlpci->rx_ring[rxring_idx].buffer_desc[i]; - if (!_rtl_pci_init_one_rxdesc(hw, (u8 *)entry, + if (!_rtl_pci_init_one_rxdesc(hw, NULL, (u8 *)entry, rxring_idx, i)) return -ENOMEM; } @@ -1332,7 +1348,7 @@ static int _rtl_pci_init_rx_ring(struct for (i = 0; i < rtlpci->rxringcount; i++) { entry = &rtlpci->rx_ring[rxring_idx].desc[i]; - if (!_rtl_pci_init_one_rxdesc(hw, (u8 *)entry, + if (!_rtl_pci_init_one_rxdesc(hw, NULL, (u8 *)entry, rxring_idx, i)) return -ENOMEM; }
These drivers use 9100-byte receive buffers, thus allocating an skb requires an O(3) memory allocation. Under heavy memory loads and fragmentation, such a request can fail. Previous versions of the driver have dropped the packet and reused the old buffer; however, the new version introduced a bug in that it released the old buffer before trying to allocate a new one. The previous method is implemented here. The skb is unmapped before any attempt is made to allocate another. Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> Cc: Stable <stable@vger.kernel.org> [v3.18] Reported-by: Eric Biggers <ebiggers3@gmail.com> Cc: Eric Biggers <ebiggers3@gmail.com> --- V2 - Fixes an error in the logic of V1. Realtek is working on a change to the RX buffer allocation, but that is likely to be too invasive for a fix to -rc or stable. In the meantime, this will help. v3 - Unmap skb before trying to allocate a new one so as to not leak mapping. Larry --- drivers/net/wireless/rtlwifi/pci.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html