Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
On 9/30/2014 9:24 PM, Eric Dumazet wrote:
On Tue, 2014-09-30 at 20:14 -0700, Prashant wrote:
Sorry about the late reply, out of all the HW bug conditions checked in
tg3_tx_frag_set() the most frequently hit condition is the short 8 byte
dma bug, where the chip cannot handle TX descriptors whose data buffer
is 8 bytes or less. Most of the LSO skb's given to the driver has their
fragments filled upto PAGE_SIZE (expect the last fragment depending on
skb->len). And if such a LSO skb's last fragment meets the 8 bytes HW
bug condition the above routine will not help workaround this particular
case.
Thats pretty easy to work around.
Say rebuilt skb has N frags (N > 1 given your description)
They are numbered 0, ... N-2, N-1
Instead of filling N-2 completely, fill it to PAGE_SIZE-8, so that last
frag has at least 8 bytes in it.
definitely it can be tweaked to match what is needed with additional
workarounds.
Also take a look at commit 2e4e44107176d552f8bb1bb76053e850e3809841
("net: add alloc_skb_with_frags() helper")
This helper is much modular/flexible than the initial proposed one. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
On 9/30/2014 9:24 PM, Eric Dumazet wrote: On Tue, 2014-09-30 at 20:14 -0700, Prashant wrote: Sorry about the late reply, out of all the HW bug conditions checked in tg3_tx_frag_set() the most frequently hit condition is the short 8 byte dma bug, where the chip cannot handle TX descriptors whose data buffer is 8 bytes or less. Most of the LSO skb's given to the driver has their fragments filled upto PAGE_SIZE (expect the last fragment depending on skb-len). And if such a LSO skb's last fragment meets the 8 bytes HW bug condition the above routine will not help workaround this particular case. Thats pretty easy to work around. Say rebuilt skb has N frags (N 1 given your description) They are numbered 0, ... N-2, N-1 Instead of filling N-2 completely, fill it to PAGE_SIZE-8, so that last frag has at least 8 bytes in it. definitely it can be tweaked to match what is needed with additional workarounds. Also take a look at commit 2e4e44107176d552f8bb1bb76053e850e3809841 (net: add alloc_skb_with_frags() helper) This helper is much modular/flexible than the initial proposed one. Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
On Tue, 2014-09-30 at 20:14 -0700, Prashant wrote:
> Sorry about the late reply, out of all the HW bug conditions checked in
> tg3_tx_frag_set() the most frequently hit condition is the short 8 byte
> dma bug, where the chip cannot handle TX descriptors whose data buffer
> is 8 bytes or less. Most of the LSO skb's given to the driver has their
> fragments filled upto PAGE_SIZE (expect the last fragment depending on
> skb->len). And if such a LSO skb's last fragment meets the 8 bytes HW
> bug condition the above routine will not help workaround this particular
> case.
Thats pretty easy to work around.
Say rebuilt skb has N frags (N > 1 given your description)
They are numbered 0, ... N-2, N-1
Instead of filling N-2 completely, fill it to PAGE_SIZE-8, so that last
frag has at least 8 bytes in it.
Also take a look at commit 2e4e44107176d552f8bb1bb76053e850e3809841
("net: add alloc_skb_with_frags() helper")
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
On 9/5/2014 9:39 PM, David Miller wrote:
From: David Miller
Date: Fri, 05 Sep 2014 17:13:06 -0700 (PDT)
From: Eric Dumazet
Date: Fri, 05 Sep 2014 17:03:30 -0700
Instead of this private helper (and pretty limited one btw), we could
add a core function, that would build skbs with order-0 fragments.
Instead of skb_linearize(), I guess many call sites could instead use
this new helper.
Because as you said, skb_linearize() of one 64KB GSO packet can ask
order-5 allocations, and this generally does not work reliably.
xen-netback could make use of this helper too.
I was curious what it might look like so I cobbled the following
completely untested patch together :-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index da1378a..eba0ad6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -955,6 +955,67 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t
gfp_mask)
EXPORT_SYMBOL(skb_copy);
/**
+ * skb_copy_pskb - copy sk_buff into a paged skb
+ * @oskb: buffer to copy
+ * @gfp_mask: allocation priority
+ *
+ * Normalize a paged skb into one that maximally uses order
+ * zero pages in it's fragment array. This is used to canonicalize
+ * spaghetti SKBs that use the page array inefficiently (f.e. only
+ * one byte per page frag).
+ */
+
+struct sk_buff *skb_copy_pskb(const struct sk_buff *oskb, gfp_t gfp_mask)
+{
+ unsigned int data_len = oskb->data_len;
+ int offset, npages, i;
+ struct sk_buff *skb;
+
+ npages = (data_len + (PAGE_SIZE - 1)) >> PAGE_SHIFT;
+ if (npages > MAX_SKB_FRAGS)
+ return NULL;
+
+ skb = __alloc_skb(skb_end_offset(oskb), gfp_mask,
+ skb_alloc_rx_flag(oskb), NUMA_NO_NODE);
+ if (!skb)
+ return NULL;
+
+ skb_reserve(skb, skb_headroom(oskb));
+ skb_put(skb, skb_headlen(oskb));
+ skb_copy_from_linear_data(oskb, skb->data, skb->len);
+
+ copy_skb_header(skb, oskb);
+
+ skb->truesize += data_len;
+ offset = skb_headlen(oskb);
+ for (i = 0; i < npages; i++) {
+ struct page *page = alloc_page(gfp_mask);
+ unsigned int chunk;
+ u8 *vaddr;
+
+ if (!page) {
+ kfree(skb);
+ skb = NULL;
+ break;
+ }
+
+ chunk = min_t(unsigned int, data_len, PAGE_SIZE);
+ skb_fill_page_desc(skb, i, page, 0, chunk);
+
+ vaddr = kmap_atomic(page);
+ skb_copy_bits(oskb, offset, vaddr, chunk);
+ kunmap_atomic(vaddr);
+
+ offset += chunk;
+ data_len -= chunk;
+ skb->data_len += chunk;
+ }
+
+ return skb;
+}
+EXPORT_SYMBOL(skb_copy_pskb);
+
+/**
*__pskb_copy_fclone - create copy of an sk_buff with private head.
*@skb: buffer to copy
*@headroom: headroom of new skb
Sorry about the late reply, out of all the HW bug conditions checked in
tg3_tx_frag_set() the most frequently hit condition is the short 8 byte
dma bug, where the chip cannot handle TX descriptors whose data buffer
is 8 bytes or less. Most of the LSO skb's given to the driver has their
fragments filled upto PAGE_SIZE (expect the last fragment depending on
skb->len). And if such a LSO skb's last fragment meets the 8 bytes HW
bug condition the above routine will not help workaround this particular
case.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
On 9/5/2014 9:39 PM, David Miller wrote:
From: David Miller da...@davemloft.net
Date: Fri, 05 Sep 2014 17:13:06 -0700 (PDT)
From: Eric Dumazet eric.duma...@gmail.com
Date: Fri, 05 Sep 2014 17:03:30 -0700
Instead of this private helper (and pretty limited one btw), we could
add a core function, that would build skbs with order-0 fragments.
Instead of skb_linearize(), I guess many call sites could instead use
this new helper.
Because as you said, skb_linearize() of one 64KB GSO packet can ask
order-5 allocations, and this generally does not work reliably.
xen-netback could make use of this helper too.
I was curious what it might look like so I cobbled the following
completely untested patch together :-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index da1378a..eba0ad6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -955,6 +955,67 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t
gfp_mask)
EXPORT_SYMBOL(skb_copy);
/**
+ * skb_copy_pskb - copy sk_buff into a paged skb
+ * @oskb: buffer to copy
+ * @gfp_mask: allocation priority
+ *
+ * Normalize a paged skb into one that maximally uses order
+ * zero pages in it's fragment array. This is used to canonicalize
+ * spaghetti SKBs that use the page array inefficiently (f.e. only
+ * one byte per page frag).
+ */
+
+struct sk_buff *skb_copy_pskb(const struct sk_buff *oskb, gfp_t gfp_mask)
+{
+ unsigned int data_len = oskb-data_len;
+ int offset, npages, i;
+ struct sk_buff *skb;
+
+ npages = (data_len + (PAGE_SIZE - 1)) PAGE_SHIFT;
+ if (npages MAX_SKB_FRAGS)
+ return NULL;
+
+ skb = __alloc_skb(skb_end_offset(oskb), gfp_mask,
+ skb_alloc_rx_flag(oskb), NUMA_NO_NODE);
+ if (!skb)
+ return NULL;
+
+ skb_reserve(skb, skb_headroom(oskb));
+ skb_put(skb, skb_headlen(oskb));
+ skb_copy_from_linear_data(oskb, skb-data, skb-len);
+
+ copy_skb_header(skb, oskb);
+
+ skb-truesize += data_len;
+ offset = skb_headlen(oskb);
+ for (i = 0; i npages; i++) {
+ struct page *page = alloc_page(gfp_mask);
+ unsigned int chunk;
+ u8 *vaddr;
+
+ if (!page) {
+ kfree(skb);
+ skb = NULL;
+ break;
+ }
+
+ chunk = min_t(unsigned int, data_len, PAGE_SIZE);
+ skb_fill_page_desc(skb, i, page, 0, chunk);
+
+ vaddr = kmap_atomic(page);
+ skb_copy_bits(oskb, offset, vaddr, chunk);
+ kunmap_atomic(vaddr);
+
+ offset += chunk;
+ data_len -= chunk;
+ skb-data_len += chunk;
+ }
+
+ return skb;
+}
+EXPORT_SYMBOL(skb_copy_pskb);
+
+/**
*__pskb_copy_fclone - create copy of an sk_buff with private head.
*@skb: buffer to copy
*@headroom: headroom of new skb
Sorry about the late reply, out of all the HW bug conditions checked in
tg3_tx_frag_set() the most frequently hit condition is the short 8 byte
dma bug, where the chip cannot handle TX descriptors whose data buffer
is 8 bytes or less. Most of the LSO skb's given to the driver has their
fragments filled upto PAGE_SIZE (expect the last fragment depending on
skb-len). And if such a LSO skb's last fragment meets the 8 bytes HW
bug condition the above routine will not help workaround this particular
case.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
On Tue, 2014-09-30 at 20:14 -0700, Prashant wrote: Sorry about the late reply, out of all the HW bug conditions checked in tg3_tx_frag_set() the most frequently hit condition is the short 8 byte dma bug, where the chip cannot handle TX descriptors whose data buffer is 8 bytes or less. Most of the LSO skb's given to the driver has their fragments filled upto PAGE_SIZE (expect the last fragment depending on skb-len). And if such a LSO skb's last fragment meets the 8 bytes HW bug condition the above routine will not help workaround this particular case. Thats pretty easy to work around. Say rebuilt skb has N frags (N 1 given your description) They are numbered 0, ... N-2, N-1 Instead of filling N-2 completely, fill it to PAGE_SIZE-8, so that last frag has at least 8 bytes in it. Also take a look at commit 2e4e44107176d552f8bb1bb76053e850e3809841 (net: add alloc_skb_with_frags() helper) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
From: David Miller
Date: Fri, 05 Sep 2014 17:13:06 -0700 (PDT)
> From: Eric Dumazet
> Date: Fri, 05 Sep 2014 17:03:30 -0700
>
>> Instead of this private helper (and pretty limited one btw), we could
>> add a core function, that would build skbs with order-0 fragments.
>>
>> Instead of skb_linearize(), I guess many call sites could instead use
>> this new helper.
>>
>> Because as you said, skb_linearize() of one 64KB GSO packet can ask
>> order-5 allocations, and this generally does not work reliably.
>
> xen-netback could make use of this helper too.
I was curious what it might look like so I cobbled the following
completely untested patch together :-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index da1378a..eba0ad6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -955,6 +955,67 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t
gfp_mask)
EXPORT_SYMBOL(skb_copy);
/**
+ * skb_copy_pskb - copy sk_buff into a paged skb
+ * @oskb: buffer to copy
+ * @gfp_mask: allocation priority
+ *
+ * Normalize a paged skb into one that maximally uses order
+ * zero pages in it's fragment array. This is used to canonicalize
+ * spaghetti SKBs that use the page array inefficiently (f.e. only
+ * one byte per page frag).
+ */
+
+struct sk_buff *skb_copy_pskb(const struct sk_buff *oskb, gfp_t gfp_mask)
+{
+ unsigned int data_len = oskb->data_len;
+ int offset, npages, i;
+ struct sk_buff *skb;
+
+ npages = (data_len + (PAGE_SIZE - 1)) >> PAGE_SHIFT;
+ if (npages > MAX_SKB_FRAGS)
+ return NULL;
+
+ skb = __alloc_skb(skb_end_offset(oskb), gfp_mask,
+ skb_alloc_rx_flag(oskb), NUMA_NO_NODE);
+ if (!skb)
+ return NULL;
+
+ skb_reserve(skb, skb_headroom(oskb));
+ skb_put(skb, skb_headlen(oskb));
+ skb_copy_from_linear_data(oskb, skb->data, skb->len);
+
+ copy_skb_header(skb, oskb);
+
+ skb->truesize += data_len;
+ offset = skb_headlen(oskb);
+ for (i = 0; i < npages; i++) {
+ struct page *page = alloc_page(gfp_mask);
+ unsigned int chunk;
+ u8 *vaddr;
+
+ if (!page) {
+ kfree(skb);
+ skb = NULL;
+ break;
+ }
+
+ chunk = min_t(unsigned int, data_len, PAGE_SIZE);
+ skb_fill_page_desc(skb, i, page, 0, chunk);
+
+ vaddr = kmap_atomic(page);
+ skb_copy_bits(oskb, offset, vaddr, chunk);
+ kunmap_atomic(vaddr);
+
+ offset += chunk;
+ data_len -= chunk;
+ skb->data_len += chunk;
+ }
+
+ return skb;
+}
+EXPORT_SYMBOL(skb_copy_pskb);
+
+/**
* __pskb_copy_fclone - create copy of an sk_buff with private head.
* @skb: buffer to copy
* @headroom: headroom of new skb
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
From: Eric Dumazet Date: Fri, 05 Sep 2014 17:03:30 -0700 > Instead of this private helper (and pretty limited one btw), we could > add a core function, that would build skbs with order-0 fragments. > > Instead of skb_linearize(), I guess many call sites could instead use > this new helper. > > Because as you said, skb_linearize() of one 64KB GSO packet can ask > order-5 allocations, and this generally does not work reliably. xen-netback could make use of this helper too. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
On Fri, 2014-09-05 at 16:35 -0700, Prashant Sreedharan wrote: > fyi.. Initially the driver was doing a skb_copy() > (tigon3_dma_hwbug_workaround()) for LSO skb that met HW bug conditions > but users started reporting page allocation failures due to copying of > large LSO skbs. To avoid this Commit 4caab52eb102f1 (tg3: Prevent page > allocation failure during TSO workaround) changed the driver logic to do > skb_gso_segment() for LSO skbs that met the HW bug conditions. With > skb_linearize() we might end up again with memory allocation failures > for large LSO skbs though at a much less frequent level (ie when TX > queue is almost full). Note that TCP stack has one skb collapse feature, currently limited in allocation of linear skbs fitting a whole page. Instead of this private helper (and pretty limited one btw), we could add a core function, that would build skbs with order-0 fragments. Instead of skb_linearize(), I guess many call sites could instead use this new helper. Because as you said, skb_linearize() of one 64KB GSO packet can ask order-5 allocations, and this generally does not work reliably. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
> static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
> struct netdev_queue *txq, struct sk_buff *skb)
> {
> - struct sk_buff *segs, *nskb;
> - u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
> + unsigned int segs_remaining = skb_shinfo(skb)->gso_segs;
>
> - /* Estimate the number of fragments in the worst case */
> - tg3_maybe_stop_txq(tnapi, txq, frag_cnt_est, frag_cnt_est);
> - if (netif_tx_queue_stopped(txq))
> - return NETDEV_TX_BUSY;
> + if (unlikely(tg3_tx_avail(tnapi) <= segs_remaining)) {
> + if (!skb_is_nonlinear(skb) || skb_linearize(skb))
> + goto tg3_tso_bug_drop;
> + tg3_start_xmit(skb, tp->dev);
fyi.. Initially the driver was doing a skb_copy()
(tigon3_dma_hwbug_workaround()) for LSO skb that met HW bug conditions
but users started reporting page allocation failures due to copying of
large LSO skbs. To avoid this Commit 4caab52eb102f1 (tg3: Prevent page
allocation failure during TSO workaround) changed the driver logic to do
skb_gso_segment() for LSO skbs that met the HW bug conditions. With
skb_linearize() we might end up again with memory allocation failures
for large LSO skbs though at a much less frequent level (ie when TX
queue is almost full).
Also some of the tg3 supported chips like 5719, 57766 have dma_limits of
4k, 2k respectively so if the LSO skb that gets linearized has size more
than dma_limit then tg3_tx_frag_set() will consume more descriptors and
if budget becomes 0 in tg3_tx_frag_set() we end up calling tg3_tso_bug()
again and eventually dropping the skb, if descriptors do not get freed
still. Instead the skb can be dropped when we know we do not have enough
descriptors to handle skb for these chip versions.
> + } else {
> + struct sk_buff *segs, *nskb;
>
> - segs = skb_gso_segment(skb, tp->dev->features &
> - ~(NETIF_F_TSO | NETIF_F_TSO6));
> - if (IS_ERR(segs) || !segs)
> - goto tg3_tso_bug_end;
> + segs = skb_gso_segment(skb, tp->dev->features &
> +~(NETIF_F_TSO | NETIF_F_TSO6 |
> + NETIF_F_SG));
> + if (IS_ERR(segs) || !segs)
> + goto tg3_tso_bug_drop;
>
> - do {
> - nskb = segs;
> - segs = segs->next;
> - nskb->next = NULL;
> - tg3_start_xmit(nskb, tp->dev);
> - } while (segs);
> + do {
> + nskb = segs;
> + segs = segs->next;
> + nskb->next = NULL;
> + if (--segs_remaining)
> + __tg3_start_xmit(nskb, tp->dev, segs_remaining);
> + else
> + tg3_start_xmit(nskb, tp->dev);
> + } while (segs);
>
> -tg3_tso_bug_end:
> + dev_kfree_skb_any(skb);
> + }
> +
> + return NETDEV_TX_OK;
> +
> +tg3_tso_bug_drop:
> + tp->tx_dropped++;
> dev_kfree_skb_any(skb);
>
> return NETDEV_TX_OK;
> @@ -7895,6 +7908,12 @@ tg3_tso_bug_end:
> /* hard_start_xmit for all devices */
> static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device
> *dev)
> {
> + return __tg3_start_xmit(skb, dev, MAX_SKB_FRAGS + 1);
> +}
> +
> +static netdev_tx_t __tg3_start_xmit(struct sk_buff *skb,
> + struct net_device *dev, u32 stop_thresh)
> +{
> struct tg3 *tp = netdev_priv(dev);
> u32 len, entry, base_flags, mss, vlan = 0;
> u32 budget;
> @@ -8102,7 +8121,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> tw32_tx_mbox(tnapi->prodmbox, entry);
>
> tnapi->tx_prod = entry;
> - tg3_maybe_stop_txq(tnapi, txq, MAX_SKB_FRAGS + 1,
> + tg3_maybe_stop_txq(tnapi, txq, stop_thresh,
> TG3_TX_WAKEUP_THRESH(tnapi));
>
> mmiowb();
> @@ -12336,9 +12355,7 @@ static int tg3_set_ringparam(struct net_device *dev,
> struct ethtool_ringparam *e
> if ((ering->rx_pending > tp->rx_std_ring_mask) ||
> (ering->rx_jumbo_pending > tp->rx_jmb_ring_mask) ||
> (ering->tx_pending > TG3_TX_RING_SIZE - 1) ||
> - (ering->tx_pending <= MAX_SKB_FRAGS + 1) ||
> - (tg3_flag(tp, TSO_BUG) &&
> - (ering->tx_pending <= (MAX_SKB_FRAGS * 3
> + (ering->tx_pending <= MAX_SKB_FRAGS + 1))
> return -EINVAL;
>
> if (netif_running(dev)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
struct netdev_queue *txq, struct sk_buff *skb)
{
- struct sk_buff *segs, *nskb;
- u32 frag_cnt_est = skb_shinfo(skb)-gso_segs * 3;
+ unsigned int segs_remaining = skb_shinfo(skb)-gso_segs;
- /* Estimate the number of fragments in the worst case */
- tg3_maybe_stop_txq(tnapi, txq, frag_cnt_est, frag_cnt_est);
- if (netif_tx_queue_stopped(txq))
- return NETDEV_TX_BUSY;
+ if (unlikely(tg3_tx_avail(tnapi) = segs_remaining)) {
+ if (!skb_is_nonlinear(skb) || skb_linearize(skb))
+ goto tg3_tso_bug_drop;
+ tg3_start_xmit(skb, tp-dev);
fyi.. Initially the driver was doing a skb_copy()
(tigon3_dma_hwbug_workaround()) for LSO skb that met HW bug conditions
but users started reporting page allocation failures due to copying of
large LSO skbs. To avoid this Commit 4caab52eb102f1 (tg3: Prevent page
allocation failure during TSO workaround) changed the driver logic to do
skb_gso_segment() for LSO skbs that met the HW bug conditions. With
skb_linearize() we might end up again with memory allocation failures
for large LSO skbs though at a much less frequent level (ie when TX
queue is almost full).
Also some of the tg3 supported chips like 5719, 57766 have dma_limits of
4k, 2k respectively so if the LSO skb that gets linearized has size more
than dma_limit then tg3_tx_frag_set() will consume more descriptors and
if budget becomes 0 in tg3_tx_frag_set() we end up calling tg3_tso_bug()
again and eventually dropping the skb, if descriptors do not get freed
still. Instead the skb can be dropped when we know we do not have enough
descriptors to handle skb for these chip versions.
+ } else {
+ struct sk_buff *segs, *nskb;
- segs = skb_gso_segment(skb, tp-dev-features
- ~(NETIF_F_TSO | NETIF_F_TSO6));
- if (IS_ERR(segs) || !segs)
- goto tg3_tso_bug_end;
+ segs = skb_gso_segment(skb, tp-dev-features
+~(NETIF_F_TSO | NETIF_F_TSO6 |
+ NETIF_F_SG));
+ if (IS_ERR(segs) || !segs)
+ goto tg3_tso_bug_drop;
- do {
- nskb = segs;
- segs = segs-next;
- nskb-next = NULL;
- tg3_start_xmit(nskb, tp-dev);
- } while (segs);
+ do {
+ nskb = segs;
+ segs = segs-next;
+ nskb-next = NULL;
+ if (--segs_remaining)
+ __tg3_start_xmit(nskb, tp-dev, segs_remaining);
+ else
+ tg3_start_xmit(nskb, tp-dev);
+ } while (segs);
-tg3_tso_bug_end:
+ dev_kfree_skb_any(skb);
+ }
+
+ return NETDEV_TX_OK;
+
+tg3_tso_bug_drop:
+ tp-tx_dropped++;
dev_kfree_skb_any(skb);
return NETDEV_TX_OK;
@@ -7895,6 +7908,12 @@ tg3_tso_bug_end:
/* hard_start_xmit for all devices */
static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device
*dev)
{
+ return __tg3_start_xmit(skb, dev, MAX_SKB_FRAGS + 1);
+}
+
+static netdev_tx_t __tg3_start_xmit(struct sk_buff *skb,
+ struct net_device *dev, u32 stop_thresh)
+{
struct tg3 *tp = netdev_priv(dev);
u32 len, entry, base_flags, mss, vlan = 0;
u32 budget;
@@ -8102,7 +8121,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
struct net_device *dev)
tw32_tx_mbox(tnapi-prodmbox, entry);
tnapi-tx_prod = entry;
- tg3_maybe_stop_txq(tnapi, txq, MAX_SKB_FRAGS + 1,
+ tg3_maybe_stop_txq(tnapi, txq, stop_thresh,
TG3_TX_WAKEUP_THRESH(tnapi));
mmiowb();
@@ -12336,9 +12355,7 @@ static int tg3_set_ringparam(struct net_device *dev,
struct ethtool_ringparam *e
if ((ering-rx_pending tp-rx_std_ring_mask) ||
(ering-rx_jumbo_pending tp-rx_jmb_ring_mask) ||
(ering-tx_pending TG3_TX_RING_SIZE - 1) ||
- (ering-tx_pending = MAX_SKB_FRAGS + 1) ||
- (tg3_flag(tp, TSO_BUG)
- (ering-tx_pending = (MAX_SKB_FRAGS * 3
+ (ering-tx_pending = MAX_SKB_FRAGS + 1))
return -EINVAL;
if (netif_running(dev)) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
On Fri, 2014-09-05 at 16:35 -0700, Prashant Sreedharan wrote: fyi.. Initially the driver was doing a skb_copy() (tigon3_dma_hwbug_workaround()) for LSO skb that met HW bug conditions but users started reporting page allocation failures due to copying of large LSO skbs. To avoid this Commit 4caab52eb102f1 (tg3: Prevent page allocation failure during TSO workaround) changed the driver logic to do skb_gso_segment() for LSO skbs that met the HW bug conditions. With skb_linearize() we might end up again with memory allocation failures for large LSO skbs though at a much less frequent level (ie when TX queue is almost full). Note that TCP stack has one skb collapse feature, currently limited in allocation of linear skbs fitting a whole page. Instead of this private helper (and pretty limited one btw), we could add a core function, that would build skbs with order-0 fragments. Instead of skb_linearize(), I guess many call sites could instead use this new helper. Because as you said, skb_linearize() of one 64KB GSO packet can ask order-5 allocations, and this generally does not work reliably. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
From: Eric Dumazet eric.duma...@gmail.com Date: Fri, 05 Sep 2014 17:03:30 -0700 Instead of this private helper (and pretty limited one btw), we could add a core function, that would build skbs with order-0 fragments. Instead of skb_linearize(), I guess many call sites could instead use this new helper. Because as you said, skb_linearize() of one 64KB GSO packet can ask order-5 allocations, and this generally does not work reliably. xen-netback could make use of this helper too. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
From: David Miller da...@davemloft.net
Date: Fri, 05 Sep 2014 17:13:06 -0700 (PDT)
From: Eric Dumazet eric.duma...@gmail.com
Date: Fri, 05 Sep 2014 17:03:30 -0700
Instead of this private helper (and pretty limited one btw), we could
add a core function, that would build skbs with order-0 fragments.
Instead of skb_linearize(), I guess many call sites could instead use
this new helper.
Because as you said, skb_linearize() of one 64KB GSO packet can ask
order-5 allocations, and this generally does not work reliably.
xen-netback could make use of this helper too.
I was curious what it might look like so I cobbled the following
completely untested patch together :-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index da1378a..eba0ad6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -955,6 +955,67 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t
gfp_mask)
EXPORT_SYMBOL(skb_copy);
/**
+ * skb_copy_pskb - copy sk_buff into a paged skb
+ * @oskb: buffer to copy
+ * @gfp_mask: allocation priority
+ *
+ * Normalize a paged skb into one that maximally uses order
+ * zero pages in it's fragment array. This is used to canonicalize
+ * spaghetti SKBs that use the page array inefficiently (f.e. only
+ * one byte per page frag).
+ */
+
+struct sk_buff *skb_copy_pskb(const struct sk_buff *oskb, gfp_t gfp_mask)
+{
+ unsigned int data_len = oskb-data_len;
+ int offset, npages, i;
+ struct sk_buff *skb;
+
+ npages = (data_len + (PAGE_SIZE - 1)) PAGE_SHIFT;
+ if (npages MAX_SKB_FRAGS)
+ return NULL;
+
+ skb = __alloc_skb(skb_end_offset(oskb), gfp_mask,
+ skb_alloc_rx_flag(oskb), NUMA_NO_NODE);
+ if (!skb)
+ return NULL;
+
+ skb_reserve(skb, skb_headroom(oskb));
+ skb_put(skb, skb_headlen(oskb));
+ skb_copy_from_linear_data(oskb, skb-data, skb-len);
+
+ copy_skb_header(skb, oskb);
+
+ skb-truesize += data_len;
+ offset = skb_headlen(oskb);
+ for (i = 0; i npages; i++) {
+ struct page *page = alloc_page(gfp_mask);
+ unsigned int chunk;
+ u8 *vaddr;
+
+ if (!page) {
+ kfree(skb);
+ skb = NULL;
+ break;
+ }
+
+ chunk = min_t(unsigned int, data_len, PAGE_SIZE);
+ skb_fill_page_desc(skb, i, page, 0, chunk);
+
+ vaddr = kmap_atomic(page);
+ skb_copy_bits(oskb, offset, vaddr, chunk);
+ kunmap_atomic(vaddr);
+
+ offset += chunk;
+ data_len -= chunk;
+ skb-data_len += chunk;
+ }
+
+ return skb;
+}
+EXPORT_SYMBOL(skb_copy_pskb);
+
+/**
* __pskb_copy_fclone - create copy of an sk_buff with private head.
* @skb: buffer to copy
* @headroom: headroom of new skb
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
In tg3_set_ringparam(), the tx_pending test to cover the cases where
tg3_tso_bug() is entered has two problems
1) the check is only done for certain hardware whereas the workaround
is now used more broadly. IOW, the check may not be performed when it
is needed.
2) the check is too optimistic.
For example, with a 5761 (SHORT_DMA_BUG), tg3_set_ringparam() skips over the
"tx_pending <= (MAX_SKB_FRAGS * 3)" check because TSO_BUG is false. Even if it
did do the check, with a full sized skb, frag_cnt_est = 135 but the check is
for <= MAX_SKB_FRAGS * 3 (= 17 * 3 = 51). So the check is insufficient. This
leads to the following situation: by setting, ex. tx_pending = 100, there can
be an skb that triggers tg3_tso_bug() and that is large enough to cause
tg3_tso_bug() to stop the queue even when it is empty. We then end up with a
netdev watchdog transmit timeout.
Given that 1) some of the conditions tested for in tg3_tx_frag_set() apply
regardless of the chipset flags and that 2) it is difficult to estimate ahead
of time the max possible number of frames that a large skb may be split into
by gso, this patch changes tg3_set_ringparam() to ignore the requirements of
tg3_tso_bug(). Those requirements are instead checked in tg3_tso_bug() itself
and if there is not a sufficient number of descriptors available in the tx
queue, the skb is linearized.
This patch also removes the current scheme in tg3_tso_bug() where the number
of descriptors required to transmit an skb is estimated. Instead,
gso_segment() is called without _SG which yields predictable, linear skbs.
Signed-off-by: Benjamin Poirier
---
Changes v1->v2
* in tg3_set_ringparam(), reduce gso_max_segs further to budget 3 descriptors
per gso seg instead of only 1 as in v1
* in tg3_tso_bug(), check that this estimation (3 desc/seg) holds, otherwise
linearize some skbs as needed
* in tg3_start_xmit(), make the queue stop threshold a parameter, for the
reason explained in the commit description
Changes v2->v3
* use tg3_maybe_stop_txq() instead of repeatedly open coding it
* add the requested tp->tx_dropped++ stat increase in tg3_tso_bug() if
skb_linearize() fails and we must abort
* in the same code block, add an additional check to stop the queue with the
default threshold. Otherwise, the netdev_err message at the start of
__tg3_start_xmit() could be triggered when the next frame is transmitted.
That is because the previous calls to __tg3_start_xmit() in tg3_tso_bug()
may have been using a stop_thresh=segs_remaining that is < MAX_SKB_FRAGS +
1.
Changes v3->v4
* in tg3_set_ringparam(), make sure that wakeup_thresh does not end up being
>= tx_pending. Identified by Prashant.
Changes v4->v5
* in tg3_set_ringparam(), use TG3_TX_WAKEUP_THRESH() and tp->txq_cnt instead
of tp->irq_max. Identified by Prashant.
Changes v5->v6
* avoid changing gso_max_segs and making the tx queue wakeup threshold
dynamic. Instead of stopping the queue when there are not enough descriptors
available, the skb is linearized.
I reproduced this bug using the same approach explained in patch 1.
The bug reproduces with tx_pending <= 135
---
drivers/net/ethernet/broadcom/tg3.c | 59 -
1 file changed, 38 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c
b/drivers/net/ethernet/broadcom/tg3.c
index 6e6b07c..a9787a1 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7830,6 +7830,8 @@ static int tigon3_dma_hwbug_workaround(struct tg3_napi
*tnapi,
}
static netdev_tx_t tg3_start_xmit(struct sk_buff *, struct net_device *);
+static netdev_tx_t __tg3_start_xmit(struct sk_buff *, struct net_device *,
+ u32);
/* Returns true if the queue has been stopped. Note that it may have been
* restarted since.
@@ -7866,27 +7868,38 @@ static inline bool tg3_maybe_stop_txq(struct tg3_napi
*tnapi,
static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
struct netdev_queue *txq, struct sk_buff *skb)
{
- struct sk_buff *segs, *nskb;
- u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
+ unsigned int segs_remaining = skb_shinfo(skb)->gso_segs;
- /* Estimate the number of fragments in the worst case */
- tg3_maybe_stop_txq(tnapi, txq, frag_cnt_est, frag_cnt_est);
- if (netif_tx_queue_stopped(txq))
- return NETDEV_TX_BUSY;
+ if (unlikely(tg3_tx_avail(tnapi) <= segs_remaining)) {
+ if (!skb_is_nonlinear(skb) || skb_linearize(skb))
+ goto tg3_tso_bug_drop;
+ tg3_start_xmit(skb, tp->dev);
+ } else {
+ struct sk_buff *segs, *nskb;
- segs = skb_gso_segment(skb, tp->dev->features &
- ~(NETIF_F_TSO | NETIF_F_TSO6));
- if (IS_ERR(segs) || !segs)
- goto tg3_tso_bug_end;
+ segs = skb_gso_segment(skb, tp->dev->features
[PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
In tg3_set_ringparam(), the tx_pending test to cover the cases where
tg3_tso_bug() is entered has two problems
1) the check is only done for certain hardware whereas the workaround
is now used more broadly. IOW, the check may not be performed when it
is needed.
2) the check is too optimistic.
For example, with a 5761 (SHORT_DMA_BUG), tg3_set_ringparam() skips over the
tx_pending = (MAX_SKB_FRAGS * 3) check because TSO_BUG is false. Even if it
did do the check, with a full sized skb, frag_cnt_est = 135 but the check is
for = MAX_SKB_FRAGS * 3 (= 17 * 3 = 51). So the check is insufficient. This
leads to the following situation: by setting, ex. tx_pending = 100, there can
be an skb that triggers tg3_tso_bug() and that is large enough to cause
tg3_tso_bug() to stop the queue even when it is empty. We then end up with a
netdev watchdog transmit timeout.
Given that 1) some of the conditions tested for in tg3_tx_frag_set() apply
regardless of the chipset flags and that 2) it is difficult to estimate ahead
of time the max possible number of frames that a large skb may be split into
by gso, this patch changes tg3_set_ringparam() to ignore the requirements of
tg3_tso_bug(). Those requirements are instead checked in tg3_tso_bug() itself
and if there is not a sufficient number of descriptors available in the tx
queue, the skb is linearized.
This patch also removes the current scheme in tg3_tso_bug() where the number
of descriptors required to transmit an skb is estimated. Instead,
gso_segment() is called without _SG which yields predictable, linear skbs.
Signed-off-by: Benjamin Poirier bpoir...@suse.de
---
Changes v1-v2
* in tg3_set_ringparam(), reduce gso_max_segs further to budget 3 descriptors
per gso seg instead of only 1 as in v1
* in tg3_tso_bug(), check that this estimation (3 desc/seg) holds, otherwise
linearize some skbs as needed
* in tg3_start_xmit(), make the queue stop threshold a parameter, for the
reason explained in the commit description
Changes v2-v3
* use tg3_maybe_stop_txq() instead of repeatedly open coding it
* add the requested tp-tx_dropped++ stat increase in tg3_tso_bug() if
skb_linearize() fails and we must abort
* in the same code block, add an additional check to stop the queue with the
default threshold. Otherwise, the netdev_err message at the start of
__tg3_start_xmit() could be triggered when the next frame is transmitted.
That is because the previous calls to __tg3_start_xmit() in tg3_tso_bug()
may have been using a stop_thresh=segs_remaining that is MAX_SKB_FRAGS +
1.
Changes v3-v4
* in tg3_set_ringparam(), make sure that wakeup_thresh does not end up being
= tx_pending. Identified by Prashant.
Changes v4-v5
* in tg3_set_ringparam(), use TG3_TX_WAKEUP_THRESH() and tp-txq_cnt instead
of tp-irq_max. Identified by Prashant.
Changes v5-v6
* avoid changing gso_max_segs and making the tx queue wakeup threshold
dynamic. Instead of stopping the queue when there are not enough descriptors
available, the skb is linearized.
I reproduced this bug using the same approach explained in patch 1.
The bug reproduces with tx_pending = 135
---
drivers/net/ethernet/broadcom/tg3.c | 59 -
1 file changed, 38 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c
b/drivers/net/ethernet/broadcom/tg3.c
index 6e6b07c..a9787a1 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7830,6 +7830,8 @@ static int tigon3_dma_hwbug_workaround(struct tg3_napi
*tnapi,
}
static netdev_tx_t tg3_start_xmit(struct sk_buff *, struct net_device *);
+static netdev_tx_t __tg3_start_xmit(struct sk_buff *, struct net_device *,
+ u32);
/* Returns true if the queue has been stopped. Note that it may have been
* restarted since.
@@ -7866,27 +7868,38 @@ static inline bool tg3_maybe_stop_txq(struct tg3_napi
*tnapi,
static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
struct netdev_queue *txq, struct sk_buff *skb)
{
- struct sk_buff *segs, *nskb;
- u32 frag_cnt_est = skb_shinfo(skb)-gso_segs * 3;
+ unsigned int segs_remaining = skb_shinfo(skb)-gso_segs;
- /* Estimate the number of fragments in the worst case */
- tg3_maybe_stop_txq(tnapi, txq, frag_cnt_est, frag_cnt_est);
- if (netif_tx_queue_stopped(txq))
- return NETDEV_TX_BUSY;
+ if (unlikely(tg3_tx_avail(tnapi) = segs_remaining)) {
+ if (!skb_is_nonlinear(skb) || skb_linearize(skb))
+ goto tg3_tso_bug_drop;
+ tg3_start_xmit(skb, tp-dev);
+ } else {
+ struct sk_buff *segs, *nskb;
- segs = skb_gso_segment(skb, tp-dev-features
- ~(NETIF_F_TSO | NETIF_F_TSO6));
- if (IS_ERR(segs) || !segs)
- goto tg3_tso_bug_end;
+ segs = skb_gso_segment(skb, tp-dev-features
+
