| Message ID | 1408474371-19509-2-git-send-email-bpoirier@suse.de |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 2014/08/19 11:52, Benjamin Poirier wrote: > + trace_printk("stopping queue, %d <= %d\n", > + tg3_tx_avail(tnapi), skb_shinfo(skb)->gso_segs); > netif_tx_stop_queue(txq); > + trace_printk("stopped queue\n"); err, I'll resubmit without the trace_printk. Please review for other issues nevertheless. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: > @@ -7838,11 +7838,14 @@ 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; > > - /* Estimate the number of fragments in the worst case */ > - if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) { > + if (unlikely(tg3_tx_avail(tnapi) <= skb_shinfo(skb)->gso_segs)) { > + trace_printk("stopping queue, %d <= %d\n", > + tg3_tx_avail(tnapi), skb_shinfo(skb)->gso_segs); > netif_tx_stop_queue(txq); > + trace_printk("stopped queue\n"); > + tnapi->wakeup_thresh = skb_shinfo(skb)->gso_segs; > + BUG_ON(tnapi->wakeup_thresh >= tnapi->tx_pending); > > /* netif_tx_stop_queue() must be done before checking > * checking tx index in tg3_tx_avail() below, because in I don't quite understand this logic and I must be missing something. gso_segs is the number of TCP segments the large packet will be broken up into. If it exceeds dev->gso_max_segs, it means it exceeds hardware's capabilty and it will do GSO instead of TSO. But in this case in tg3_tso_bug(), we are doing GSO and we may not have enough DMA descriptors to do GSO. Each gso_seg typically requires 2 DMA descriptors. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/08/19 16:10, Michael Chan wrote: > On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: > > @@ -7838,11 +7838,14 @@ 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; > > > > - /* Estimate the number of fragments in the worst case */ > > - if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) { > > + if (unlikely(tg3_tx_avail(tnapi) <= skb_shinfo(skb)->gso_segs)) { > > + trace_printk("stopping queue, %d <= %d\n", > > + tg3_tx_avail(tnapi), skb_shinfo(skb)->gso_segs); > > netif_tx_stop_queue(txq); > > + trace_printk("stopped queue\n"); > > + tnapi->wakeup_thresh = skb_shinfo(skb)->gso_segs; > > + BUG_ON(tnapi->wakeup_thresh >= tnapi->tx_pending); > > > > /* netif_tx_stop_queue() must be done before checking > > * checking tx index in tg3_tx_avail() below, because in > > I don't quite understand this logic and I must be missing something. > gso_segs is the number of TCP segments the large packet will be broken > up into. If it exceeds dev->gso_max_segs, it means it exceeds > hardware's capabilty and it will do GSO instead of TSO. But in this > case in tg3_tso_bug(), we are doing GSO and we may not have enough DMA > descriptors to do GSO. Each gso_seg typically requires 2 DMA > descriptors. You're right, I had wrongly assumed that the skbs coming out of skb_gso_segment() were linear. I'll address that in v2 of the patch by masking out NETIF_F_SG in tg3_tso_bug(). I noticed another issue that had not occurred to me: when tg3_tso_bug is submitting a full gso segs sequence to tg3_start_xmit, the code at the end of that function stops the queue before the end of the sequence because tx_avail becomes smaller than (MAX_SKB_FRAGS + 1). The transmission actually proceeds because tg3_tso_bug() does not honour the queue state but it seems rather unsightly to me. I'm trying different solutions to this and will resubmit. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-08-20 at 18:23 -0700, Benjamin Poirier wrote: > On 2014/08/19 16:10, Michael Chan wrote: > > On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: > > > @@ -7838,11 +7838,14 @@ 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; > > > > > > - /* Estimate the number of fragments in the worst case */ > > > - if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) { > > > + if (unlikely(tg3_tx_avail(tnapi) <= skb_shinfo(skb)->gso_segs)) { > > > + trace_printk("stopping queue, %d <= %d\n", > > > + tg3_tx_avail(tnapi), skb_shinfo(skb)->gso_segs); > > > netif_tx_stop_queue(txq); > > > + trace_printk("stopped queue\n"); > > > + tnapi->wakeup_thresh = skb_shinfo(skb)->gso_segs; > > > + BUG_ON(tnapi->wakeup_thresh >= tnapi->tx_pending); > > > > > > /* netif_tx_stop_queue() must be done before checking > > > * checking tx index in tg3_tx_avail() below, because in > > > > I don't quite understand this logic and I must be missing something. > > gso_segs is the number of TCP segments the large packet will be broken > > up into. If it exceeds dev->gso_max_segs, it means it exceeds > > hardware's capabilty and it will do GSO instead of TSO. But in this > > case in tg3_tso_bug(), we are doing GSO and we may not have enough DMA > > descriptors to do GSO. Each gso_seg typically requires 2 DMA > > descriptors. > > You're right, I had wrongly assumed that the skbs coming out of > skb_gso_segment() were linear. I'll address that in v2 of the patch by masking > out NETIF_F_SG in tg3_tso_bug(). > While masking out NETF_F_SG will work, it will also disable checksum offload for the whole device momentarily. > I noticed another issue that had not occurred to me: when tg3_tso_bug is > submitting a full gso segs sequence to tg3_start_xmit, the code at the end of > that function stops the queue before the end of the sequence because tx_avail > becomes smaller than (MAX_SKB_FRAGS + 1). The transmission actually proceeds > because tg3_tso_bug() does not honour the queue state but it seems rather > unsightly to me. That's why the number of DMA descriptors that we estimate has to be accurate. It's unfortunate that the various tg3 chips require so many different workarounds. The objective is to keep TSO and checksum enabled and workaround the occasional packets using GSO. I believe that the boundary error conditions that you brought up can be addressed by enforcing some limits on the tx ring size and by reducing gso_max_size/gso_max_segs when necessary (for example when MTU and/or ring size is set very small). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/08/21 02:51, Michael Chan wrote: > On Wed, 2014-08-20 at 18:23 -0700, Benjamin Poirier wrote: > > On 2014/08/19 16:10, Michael Chan wrote: > > > On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote: > > > > @@ -7838,11 +7838,14 @@ 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; > > > > > > > > - /* Estimate the number of fragments in the worst case */ > > > > - if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) { > > > > + if (unlikely(tg3_tx_avail(tnapi) <= skb_shinfo(skb)->gso_segs)) { > > > > + trace_printk("stopping queue, %d <= %d\n", > > > > + tg3_tx_avail(tnapi), skb_shinfo(skb)->gso_segs); > > > > netif_tx_stop_queue(txq); > > > > + trace_printk("stopped queue\n"); > > > > + tnapi->wakeup_thresh = skb_shinfo(skb)->gso_segs; > > > > + BUG_ON(tnapi->wakeup_thresh >= tnapi->tx_pending); > > > > > > > > /* netif_tx_stop_queue() must be done before checking > > > > * checking tx index in tg3_tx_avail() below, because in > > > > > > I don't quite understand this logic and I must be missing something. > > > gso_segs is the number of TCP segments the large packet will be broken > > > up into. If it exceeds dev->gso_max_segs, it means it exceeds > > > hardware's capabilty and it will do GSO instead of TSO. But in this > > > case in tg3_tso_bug(), we are doing GSO and we may not have enough DMA > > > descriptors to do GSO. Each gso_seg typically requires 2 DMA > > > descriptors. > > > > You're right, I had wrongly assumed that the skbs coming out of > > skb_gso_segment() were linear. I'll address that in v2 of the patch by masking > > out NETIF_F_SG in tg3_tso_bug(). > > > > While masking out NETF_F_SG will work, it will also disable checksum > offload for the whole device momentarily. > > > I noticed another issue that had not occurred to me: when tg3_tso_bug is > > submitting a full gso segs sequence to tg3_start_xmit, the code at the end of > > that function stops the queue before the end of the sequence because tx_avail > > becomes smaller than (MAX_SKB_FRAGS + 1). The transmission actually proceeds > > because tg3_tso_bug() does not honour the queue state but it seems rather > > unsightly to me. > > That's why the number of DMA descriptors that we estimate has to be > accurate. It's unfortunate that the various tg3 chips require so many > different workarounds. The objective is to keep TSO and checksum > enabled and workaround the occasional packets using GSO. Ah, now I understand the reason for the * 3 in u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3; /* Estimate the number of fragments in the worst case */ but that is not really the "worst case". It's not forbidden to have more than two frags per skb output from skb_gso_segment(). I've kept this estimation approach but I've added code to validate the estimation or else linearize the skb. > > I believe that the boundary error conditions that you brought up can be > addressed by enforcing some limits on the tx ring size and by reducing > gso_max_size/gso_max_segs when necessary (for example when MTU and/or > ring size is set very small). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Benjamin Poirier <bpoirier@suse.de> Date: Thu, 21 Aug 2014 14:59:11 -0700 > Ah, now I understand the reason for the * 3 in > u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3; > > /* Estimate the number of fragments in the worst case */ > but that is not really the "worst case". It's not forbidden to have more than > two frags per skb output from skb_gso_segment(). I've kept this estimation > approach but I've added code to validate the estimation or else linearize the > skb. This is a common situation drivers run into, and there have been a few notable situations in virtualization drivers recently. Although in those cases the problems arise from the fact that if an SKB fragment is a compound page, this can evaluate to requiring multiple descriptors, one for each 4K segment within that fragment. Anyways, the point I wanted to make is that you shouldn't do anything too complicated to handle all of this. And I think your conclusion to linearize if the estimation fails is a good one. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index b11c0fd..7022f6d 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -6609,10 +6609,10 @@ static void tg3_tx(struct tg3_napi *tnapi) smp_mb(); if (unlikely(netif_tx_queue_stopped(txq) && - (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi)))) { + (tg3_tx_avail(tnapi) > tnapi->wakeup_thresh))) { __netif_tx_lock(txq, smp_processor_id()); if (netif_tx_queue_stopped(txq) && - (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi))) + (tg3_tx_avail(tnapi) > tnapi->wakeup_thresh)) netif_tx_wake_queue(txq); __netif_tx_unlock(txq); } @@ -7838,11 +7838,14 @@ 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; - /* Estimate the number of fragments in the worst case */ - if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) { + if (unlikely(tg3_tx_avail(tnapi) <= skb_shinfo(skb)->gso_segs)) { + trace_printk("stopping queue, %d <= %d\n", + tg3_tx_avail(tnapi), skb_shinfo(skb)->gso_segs); netif_tx_stop_queue(txq); + trace_printk("stopped queue\n"); + tnapi->wakeup_thresh = skb_shinfo(skb)->gso_segs; + BUG_ON(tnapi->wakeup_thresh >= tnapi->tx_pending); /* netif_tx_stop_queue() must be done before checking * checking tx index in tg3_tx_avail() below, because in @@ -7850,7 +7853,7 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi, * netif_tx_queue_stopped(). */ smp_mb(); - if (tg3_tx_avail(tnapi) <= frag_cnt_est) + if (tg3_tx_avail(tnapi) <= tnapi->wakeup_thresh) return NETDEV_TX_BUSY; netif_tx_wake_queue(txq); @@ -7905,12 +7908,17 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev) if (unlikely(budget <= (skb_shinfo(skb)->nr_frags + 1))) { if (!netif_tx_queue_stopped(txq)) { netif_tx_stop_queue(txq); + tnapi->wakeup_thresh = TG3_TX_WAKEUP_THRESH(tnapi); /* This is a hard error, log it. */ netdev_err(dev, "BUG! Tx Ring full when queue awake!\n"); } - return NETDEV_TX_BUSY; + smp_mb(); + if (tg3_tx_avail(tnapi) <= tnapi->wakeup_thresh) + return NETDEV_TX_BUSY; + + netif_tx_wake_queue(txq); } entry = tnapi->tx_prod; @@ -8089,6 +8097,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev) tnapi->tx_prod = entry; if (unlikely(tg3_tx_avail(tnapi) <= (MAX_SKB_FRAGS + 1))) { netif_tx_stop_queue(txq); + tnapi->wakeup_thresh = TG3_TX_WAKEUP_THRESH(tnapi); /* netif_tx_stop_queue() must be done before checking * checking tx index in tg3_tx_avail() below, because in @@ -8096,7 +8105,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev) * netif_tx_queue_stopped(). */ smp_mb(); - if (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi)) + if (tg3_tx_avail(tnapi) > tnapi->wakeup_thresh) netif_tx_wake_queue(txq); } @@ -12319,9 +12328,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) || - (tg3_flag(tp, TSO_BUG) && - (ering->tx_pending <= (MAX_SKB_FRAGS * 3)))) + (ering->tx_pending <= MAX_SKB_FRAGS)) return -EINVAL; if (netif_running(dev)) { @@ -12341,6 +12348,7 @@ static int tg3_set_ringparam(struct net_device *dev, struct ethtool_ringparam *e if (tg3_flag(tp, JUMBO_RING_ENABLE)) tp->rx_jumbo_pending = ering->rx_jumbo_pending; + dev->gso_max_segs = ering->tx_pending - 1; for (i = 0; i < tp->irq_max; i++) tp->napi[i].tx_pending = ering->tx_pending; @@ -17817,6 +17825,7 @@ static int tg3_init_one(struct pci_dev *pdev, else sndmbx += 0xc; } + dev->gso_max_segs = TG3_DEF_TX_RING_PENDING - 1; tg3_init_coal(tp); diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h index 461acca..6a7e13d 100644 --- a/drivers/net/ethernet/broadcom/tg3.h +++ b/drivers/net/ethernet/broadcom/tg3.h @@ -3006,6 +3006,7 @@ struct tg3_napi { u32 tx_pending; u32 last_tx_cons; u32 prodmbox; + u32 wakeup_thresh; struct tg3_tx_buffer_desc *tx_ring; struct tg3_tx_ring_info *tx_buffers;
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, we instead take the approach of adjusting dev->gso_max_segs according to the requested tx_pending size. This puts us in the exceptional situation that a single skb that triggers tg3_tso_bug() may require the entire tx ring. Usually the tx queue is woken up when at least a quarter of it is available (TG3_TX_WAKEUP_THRESH) but that would be insufficient now. To avoid useless wakeups, the tx queue wake up threshold is made dynamic. Signed-off-by: Benjamin Poirier <bpoirier@suse.de> --- 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 | 31 ++++++++++++++++++++----------- drivers/net/ethernet/broadcom/tg3.h | 1 + 2 files changed, 21 insertions(+), 11 deletions(-)