| Message ID | 1419711457-21469-1-git-send-email-Larry.Finger@lwfinger.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
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. > > 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. 23/23? Where are patches 1-22? I don't see them in patchwork: https://patchwork.kernel.org/project/linux-wireless/list/
On 12/27/2014 11:50 PM, Kalle Valo wrote: > 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. >> >> 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. > > 23/23? Where are patches 1-22? I don't see them in patchwork: > > https://patchwork.kernel.org/project/linux-wireless/list/ Sorry. I forgot to edit the subject line. To get git to format that one patch, I had to generate a total of 23. There is only 1 of 1 that will be submitted. 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
On Sat, Dec 27, 2014 at 02:17:37PM -0600, Larry Finger wrote: > 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. It looks like in the out-of-memory path, pci_map_single() gets called while the skb is still mapped. Won't this leak the IOMMU mapping? -- 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 06:49 PM, Eric Biggers wrote: > On Sat, Dec 27, 2014 at 02:17:37PM -0600, Larry Finger wrote: >> 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. > > It looks like in the out-of-memory path, pci_map_single() gets called while the > skb is still mapped. Won't this leak the IOMMU mapping? Good catch. I do not know much about leaking the IOMMU mapping; however it is easy to do the unmapping before trying to allocate a new skb. Thanks, 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: >> 23/23? Where are patches 1-22? I don't see them in patchwork: >> >> https://patchwork.kernel.org/project/linux-wireless/list/ > > Sorry. I forgot to edit the subject line. To get git to format that > one patch, I had to generate a total of 23. There is only 1 of 1 that > will be submitted. Ok, I was just worried that I had missed the other 22. But you can actually get a single commit like this so no editing is needed: git format-patch -1 2a3e60d37fc6
On 01/05/2015 03:20 AM, Kalle Valo wrote: > Larry Finger <Larry.Finger@lwfinger.net> writes: > >>> 23/23? Where are patches 1-22? I don't see them in patchwork: >>> >>> https://patchwork.kernel.org/project/linux-wireless/list/ >> >> Sorry. I forgot to edit the subject line. To get git to format that >> one patch, I had to generate a total of 23. There is only 1 of 1 that >> will be submitted. > > Ok, I was just worried that I had missed the other 22. But you can > actually get a single commit like this so no editing is needed: > > git format-patch -1 2a3e60d37fc6 Thanks for the git lesson. Some week^H^H^H^Hmonth I should learn to use more parts of that tool, but you know how that goes. I do have an additional question on procedure. If I submit a number of patches in a series, but find that only one of the series needs a V2. Is it better to resubmit all N of them, or only just the one that needs changing? Thanks, 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: > On 01/05/2015 03:20 AM, Kalle Valo wrote: >> Larry Finger <Larry.Finger@lwfinger.net> writes: >> >>>> 23/23? Where are patches 1-22? I don't see them in patchwork: >>>> >>>> https://patchwork.kernel.org/project/linux-wireless/list/ >>> >>> Sorry. I forgot to edit the subject line. To get git to format that >>> one patch, I had to generate a total of 23. There is only 1 of 1 that >>> will be submitted. >> >> Ok, I was just worried that I had missed the other 22. But you can >> actually get a single commit like this so no editing is needed: >> >> git format-patch -1 2a3e60d37fc6 > > Thanks for the git lesson. Some week^H^H^H^Hmonth I should learn to > use more parts of that tool, but you know how that goes. I have the exactly same situation with python :) > I do have an additional question on procedure. If I submit a number of > patches in a series, but find that only one of the series needs a V2. > Is it better to resubmit all N of them, or only just the one that > needs changing? Please resubmit the whole series. It's just so much easier and more reliable for me to handle changes that way.
diff --git a/drivers/net/wireless/rtlwifi/pci.c b/drivers/net/wireless/rtlwifi/pci.c index 846a2e6..cf4e2c6 100644 --- a/drivers/net/wireless/rtlwifi/pci.c +++ b/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(struct ieee80211_hw *hw, 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(struct ieee80211_hw *hw, 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 ieee80211_hw *hw) /*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 = @@ -800,6 +807,13 @@ static void _rtl_pci_rx_interrupt(struct ieee80211_hw *hw) return; } + /* 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; + } /* Reaching this point means: data is filled already * AAAAAAttention !!! * We can NOT access 'skb' before 'pci_unmap_single' @@ -911,14 +925,16 @@ static void _rtl_pci_rx_interrupt(struct ieee80211_hw *hw) 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 ieee80211_hw *hw, int rxring_idx) 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 ieee80211_hw *hw, int rxring_idx) 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. 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. Larry --- drivers/net/wireless/rtlwifi/pci.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-)