| Message ID | 1429101008-9464-5-git-send-email-dingtianhong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote: > @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev) > > /* Ensure tx_head update visible to tx reclaim */ > smp_wmb(); > + count++; > + priv->tx_head = TX_NEXT(tx_head); > > Something is wrong here, the comment does not match the code any more, because the smp_wmb() won't actually make the tx_head update visible. Arnd
On 2015/4/15 22:19, Arnd Bergmann wrote: > On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote: >> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev) >> >> /* Ensure tx_head update visible to tx reclaim */ >> smp_wmb(); >> + count++; >> + priv->tx_head = TX_NEXT(tx_head); >> >> > > Something is wrong here, the comment does not match the code any more, because > the smp_wmb() won't actually make the tx_head update visible. > > Arnd > Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit. > . >
On Thursday 16 April 2015 14:26:21 Ding Tianhong wrote: > On 2015/4/15 22:19, Arnd Bergmann wrote: > > On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote: > >> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev) > >> > >> /* Ensure tx_head update visible to tx reclaim */ > >> smp_wmb(); > >> + count++; > >> + priv->tx_head = TX_NEXT(tx_head); > >> > >> > > > > Something is wrong here, the comment does not match the code any more, because > > the smp_wmb() won't actually make the tx_head update visible. > > > > Arnd > > > Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit. The problem you need to avoid is that the tx reclaim function thinks it's done with all outstanding packets and does not retrigger, while the start_xmit thinks it will still get to that. This means you need a barrier between the priv->tx_head update and the napi_schedule_prep() call. Arnd
On 2015/4/16 16:41, Arnd Bergmann wrote: > On Thursday 16 April 2015 14:26:21 Ding Tianhong wrote: >> On 2015/4/15 22:19, Arnd Bergmann wrote: >>> On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote: >>>> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev) >>>> >>>> /* Ensure tx_head update visible to tx reclaim */ >>>> smp_wmb(); >>>> + count++; >>>> + priv->tx_head = TX_NEXT(tx_head); >>>> >>>> >>> >>> Something is wrong here, the comment does not match the code any more, because >>> the smp_wmb() won't actually make the tx_head update visible. >>> >>> Arnd >>> >> Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit. > > The problem you need to avoid is that the tx reclaim function thinks it's > done with all outstanding packets and does not retrigger, while the > start_xmit thinks it will still get to that. This means you > need a barrier between the priv->tx_head update and the > napi_schedule_prep() call. > But I still doubt about that, when the buffer is ready to xmit, it will add to fifo queue, but it doesn't mean it is finished xmit yet, the tx reclaim function will free the skb only depend on whether tx_head is updated. Ding > Arnd > > . >
diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c index 7533858..ff9d19c 100644 --- a/drivers/net/ethernet/hisilicon/hip04_eth.c +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c @@ -480,8 +480,6 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev) skb_tx_timestamp(skb); hip04_set_xmit_desc(priv, phys); - priv->tx_head = TX_NEXT(tx_head); - count++; netdev_sent_queue(ndev, skb->len); stats->tx_bytes += skb->len; @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev) /* Ensure tx_head update visible to tx reclaim */ smp_wmb(); + count++; + priv->tx_head = TX_NEXT(tx_head); /* queue is getting full, better start cleaning up now */ if (count >= priv->tx_coalesce_frames) {
Eric pointed out the problem that the tx skb might already have be freed by tx completion before enter the xmit queue, so don't notice the tx completion until the skb is ready to be freed. Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Dumazet <eric.dumazet@gmail.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Zhangfei Gao <zhangfei.gao@linaro.org> Cc: Dan Carpenter <dan.carpenter@oracle.com> Cc: Joe Perches <joe@perches.com> --- drivers/net/ethernet/hisilicon/hip04_eth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)