| Message ID | 1424535746.5565.42.camel@edumazet-glaptop2.roam.corp.google.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Eric Dumazet <eric.dumazet@gmail.com> wrote: > Hold on. > > I believe there is one race in the way you access skb->xmit_more _after_ > > txd->opts1 = cpu_to_le32(status); > > After this point, TX might have completed and TX completion already have > freed skb Hmm, I _thought_ HW would not start xmit of this descriptor/skb until after RTL_W8(TxPoll, NPQ); call a bit later... your patch surely looks safer though, thanks for looking into this. > Could Tomas try following fix ? In addition -- Tomas, on your affected board -- did you try turning off gso/tso? Holger Hoffstätte mentioned on lkml that his affected r8169 nic is stable with xmit_more+bql after disabling gso/tso on the nic (ethtool -k $dev to display settings, "-K $dev tso off gso off" to disable offloads). -- 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 Sat, 2015-02-21 at 18:46 +0100, Florian Westphal wrote: > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Hold on. > > > > I believe there is one race in the way you access skb->xmit_more _after_ > > > > txd->opts1 = cpu_to_le32(status); > > > > After this point, TX might have completed and TX completion already have > > freed skb > > Hmm, I _thought_ HW would not start xmit of this descriptor/skb until after > > RTL_W8(TxPoll, NPQ); Note this 'kick' does not provide tail ptr. NIC basically looks at TX descriptors to find ones with the DescOwn bit set. It stops when if find one TX descriptor _without_ DescOwn. So what can happen here is the NIC was kicked earlier (prior transmit), but found your TX descriptor, before you got a chance to read skb->xmit_more This is why we have these wmb() everywhere. We want to do txd->opts1 = XXX Only when we are really ready. -- 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
Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Sat, 2015-02-21 at 18:46 +0100, Florian Westphal wrote: > > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > Hold on. > > > > > > I believe there is one race in the way you access skb->xmit_more _after_ > > > > > > txd->opts1 = cpu_to_le32(status); > > > > > > After this point, TX might have completed and TX completion already have > > > freed skb > > > > Hmm, I _thought_ HW would not start xmit of this descriptor/skb until after > > > > RTL_W8(TxPoll, NPQ); > > Note this 'kick' does not provide tail ptr. > > NIC basically looks at TX descriptors to find ones with the DescOwn bit > set. It stops when if find one TX descriptor _without_ DescOwn. Makes sense, thanks for explaining this. -- 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
> > David, please consider reverting > > > > 1e918876853aa85435e0f17fd8b4a92dcfff53d6 > > (r8169: add support for Byte Queue Limits) > > > > and > > > > 0bec3b700d106a8b0a34227b2976d1a582f1aab7 > > (r8169: add support for xmit_more) > > > > I cannot reproduce any hangs (tried for 2days with 40 parallel > > netperfs using both 100mbit and 1gbit receiver). > > > > And I don't see anything wrong with the change either. > > Seems like some revisions of the HW are just dodgy? > > > > I hate giving up, but I have no means to diagnose this any further. > > Even reporter says it doesn't affect all of his r8169 nics. > > > > So I think the change is correct per se, but might be revealing some > > HW/firmware bug. > > Hold on. > > I believe there is one race in the way you access skb->xmit_more _after_ > > txd->opts1 = cpu_to_le32(status); > > After this point, TX might have completed and TX completion already have > freed skb > > Could Tomas try following fix ? > > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > index ad0020af2193..f2764366a36c 100644 > ... Sure, just did. Unfortunately, 3.19.0 + 0bec3b70 + this patch results in a driver that retains the problem. Sorry,
> > Could Tomas try following fix ? > > In addition -- Tomas, on your affected board -- did you try > turning off gso/tso? > > Holger Hoffstätte mentioned on lkml that his affected r8169 nic is > stable with xmit_more+bql after disabling gso/tso on the nic > > (ethtool -k $dev to display settings, "-K $dev tso off gso off" to > disable offloads). They're always off on this NIC (cannot be turned on). # ethtool -k eth1| grep segmentation-offload tcp-segmentation-offload: off generic-segmentation-offload: off [requested on] # ethtool -K eth1 gso on tso on Could not change any device features # ethtool -k eth1| grep segmentation-offload tcp-segmentation-offload: off generic-segmentation-offload: off [requested on]
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index ad0020af2193..f2764366a36c 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -7050,6 +7050,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb, u32 opts[2]; int frags; bool stop_queue; + bool xmit_more; if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) { netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n"); @@ -7091,7 +7092,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb, txd->opts2 = cpu_to_le32(opts[1]); netdev_sent_queue(dev, skb->len); - + xmit_more = skb->xmit_more; skb_tx_timestamp(skb); /* Force memory writes to complete before releasing descriptor */ @@ -7108,7 +7109,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb, stop_queue = !TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS); - if (!skb->xmit_more || stop_queue || + if (!xmit_more || stop_queue || netif_xmit_stopped(netdev_get_tx_queue(dev, 0))) { RTL_W8(TxPoll, NPQ);