| Message ID | 1408976542-15624-1-git-send-email-jonas.jensen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
From: Jonas Jensen <jonas.jensen@gmail.com> Date: Mon, 25 Aug 2014 16:22:22 +0200 > build_skb() is used to make skbs out of existing RX ring memory > which is bad because the RX ring is allocated only once, on probe. > Memory corruption occur because said memory is reclaimed, i.e. > __kfree_skb() (and eventually put_page()). > > Replace build_skb() with netdev_alloc_skb_ip_align() and use memcpy(). > > Remove SKB_DATA_ALIGN() from RX buffer size while we're at it. > > Addresses https://bugzilla.kernel.org/show_bug.cgi?id=69041 > > Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com> Applied.
On Monday 25 August 2014 16:22:22 Jonas Jensen wrote: > @@ -226,13 +226,15 @@ static int moxart_rx_poll(struct napi_struct *napi, int budget) > if (len > RX_BUF_SIZE) > len = RX_BUF_SIZE; > > - skb = build_skb(priv->rx_buf[rx_head], priv->rx_buf_size); > + skb = netdev_alloc_skb_ip_align(ndev, len); > + > if (unlikely(!skb)) { > - net_dbg_ratelimited("build_skb failed\n"); > + net_dbg_ratelimited("netdev_alloc_skb_ip_align failed\n"); > priv->stats.rx_dropped++; > priv->stats.rx_errors++; > } > > + memcpy(skb->data, priv->rx_buf[rx_head], len); > skb_put(skb, len); > skb->protocol = eth_type_trans(skb, ndev); > napi_gro_receive(&priv->napi, skb); While this seems correct, I wonder why you don't do the normal approach of dequeuing the skb from the chain and adding a newly allocated skb to it to save the memcpy. Arnd
From: Arnd Bergmann > On Monday 25 August 2014 16:22:22 Jonas Jensen wrote: > > @@ -226,13 +226,15 @@ static int moxart_rx_poll(struct napi_struct *napi, int budget) > > if (len > RX_BUF_SIZE) > > len = RX_BUF_SIZE; > > > > - skb = build_skb(priv->rx_buf[rx_head], priv->rx_buf_size); > > + skb = netdev_alloc_skb_ip_align(ndev, len); > > + > > if (unlikely(!skb)) { > > - net_dbg_ratelimited("build_skb failed\n"); > > + net_dbg_ratelimited("netdev_alloc_skb_ip_align failed\n"); > > priv->stats.rx_dropped++; > > priv->stats.rx_errors++; > > } > > > > + memcpy(skb->data, priv->rx_buf[rx_head], len); Is this memcpy() aligned? If the hardware can receive to a 4n+2 offset it is probably better to copy the two bytes before the frame data and to round the length of to a whole number of words. > > skb_put(skb, len); > > skb->protocol = eth_type_trans(skb, ndev); > > napi_gro_receive(&priv->napi, skb); > > While this seems correct, I wonder why you don't do the normal approach of > dequeuing the skb from the chain and adding a newly allocated skb to it to > save the memcpy. Because the receive buffer area isn't made of skbs. Post-allocating the skb also reduces the 'true size' of the skb. David
On Tue, 2014-08-26 at 09:10 +0000, David Laight wrote: > From: Arnd Bergmann > > While this seems correct, I wonder why you don't do the normal approach of > > dequeuing the skb from the chain and adding a newly allocated skb to it to > > save the memcpy. > > Because the receive buffer area isn't made of skbs. > Post-allocating the skb also reduces the 'true size' of the skb. This strategy assumes this is not a 10Gbe NIC. We try to avoid copies because they are generally not needed. Wifi devices are usually slow, and packet losses are more frequent, so the copybreak gives better chance to not doing the collapses [1] later in the TCP stack. [1] collapses : reducing skb overhead (skb->len / skb->truesize ratio)
diff --git a/drivers/net/ethernet/moxa/moxart_ether.c b/drivers/net/ethernet/moxa/moxart_ether.c index eed70d9..d66058d 100644 --- a/drivers/net/ethernet/moxa/moxart_ether.c +++ b/drivers/net/ethernet/moxa/moxart_ether.c @@ -226,13 +226,15 @@ static int moxart_rx_poll(struct napi_struct *napi, int budget) if (len > RX_BUF_SIZE) len = RX_BUF_SIZE; - skb = build_skb(priv->rx_buf[rx_head], priv->rx_buf_size); + skb = netdev_alloc_skb_ip_align(ndev, len); + if (unlikely(!skb)) { - net_dbg_ratelimited("build_skb failed\n"); + net_dbg_ratelimited("netdev_alloc_skb_ip_align failed\n"); priv->stats.rx_dropped++; priv->stats.rx_errors++; } + memcpy(skb->data, priv->rx_buf[rx_head], len); skb_put(skb, len); skb->protocol = eth_type_trans(skb, ndev); napi_gro_receive(&priv->napi, skb); @@ -464,8 +466,7 @@ static int moxart_mac_probe(struct platform_device *pdev) spin_lock_init(&priv->txlock); priv->tx_buf_size = TX_BUF_SIZE; - priv->rx_buf_size = RX_BUF_SIZE + - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); + priv->rx_buf_size = RX_BUF_SIZE; priv->tx_desc_base = dma_alloc_coherent(NULL, TX_REG_DESC_SIZE * TX_DESC_NUM, &priv->tx_base,
build_skb() is used to make skbs out of existing RX ring memory which is bad because the RX ring is allocated only once, on probe. Memory corruption occur because said memory is reclaimed, i.e. __kfree_skb() (and eventually put_page()). Replace build_skb() with netdev_alloc_skb_ip_align() and use memcpy(). Remove SKB_DATA_ALIGN() from RX buffer size while we're at it. Addresses https://bugzilla.kernel.org/show_bug.cgi?id=69041 Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com> --- Notes: Changes since v5: 1. broke out DMA synchronization to separate patch Applies to next-20140825 drivers/net/ethernet/moxa/moxart_ether.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)