[RFC] Re: Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.
From: Pavel Machek <hidden>
Date: 2016-12-07 12:31:45
Also in:
lkml
Subsystem:
networking drivers, stmmac ethernet driver, the rest · Maintainers:
Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Chevallier, Linus Torvalds
On Fri 2016-12-02 15:05:21, Lino Sanfilippo wrote:
Hi,quoted
There's nothing that protect stmmac_poll() from running concurently with stmmac_dma_interrupt(), right?could it be that there is also another issue concerned locking?: The tx completion handler takes the xmit_lock in case that the netif_queue is stopped. This is AFAICS unnecessary, since both xmit and completion handler are already synchronized by the private tx lock. But it is IMHO also dangerous: In the xmit handler we have the locking order 1. xmit_lock 2. private tx lock while in the completion handler its the reverse: 1. private tx lock 2. xmit lock. Do I miss something?
No, it seems you are right. Something like this?
Hmm. And can priv->tx_lock be removed, as we already rely on
netif_tx_lock?
(I copied the "lock already held" annotations from forcedeth. I hope
they are right....)
Best regards,
Pavel
commit a6f21255dfc11fcadc5062dfd0c5f3d77ca4f634
Author: Pavel [off-list ref]
Date: Wed Dec 7 13:29:15 2016 +0100
Reported-by: Lino Sanfilippo [off-list ref]
The tx completion handler takes the xmit_lock in case that the
netif_queue is stopped. This is AFAICS unnecessary, since both
xmit and completion handler are already synchronized by the private
tx lock. But it is IMHO also dangerous:
In the xmit handler we have the locking order
1. xmit_lock
2. private tx lock
while in the completion handler its the reverse:
1. private tx lock
2. xmit lock.
Signed-off-by: Pavel Machek [off-list ref]
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 982c952..5df9bb3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c@@ -1380,14 +1380,9 @@ static void stmmac_tx_clean(struct stmmac_priv *priv) if (unlikely(netif_queue_stopped(priv->dev) && stmmac_tx_avail(priv) > STMMAC_TX_THRESH)) { - netif_tx_lock(priv->dev); - if (netif_queue_stopped(priv->dev) && - stmmac_tx_avail(priv) > STMMAC_TX_THRESH) { - netif_dbg(priv, tx_done, priv->dev, - "%s: restart transmit\n", __func__); - netif_wake_queue(priv->dev); - } - netif_tx_unlock(priv->dev); + netif_dbg(priv, tx_done, priv->dev, + "%s: restart transmit\n", __func__); + netif_wake_queue(priv->dev); } if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
@@ -1630,7 +1625,9 @@ static void stmmac_tx_timer(unsigned long data) { struct stmmac_priv *priv = (struct stmmac_priv *)data; + netif_tx_lock_bh(priv->dev); stmmac_tx_clean(priv); + netif_tx_unlock_bh(priv->dev); } /**
@@ -1994,7 +1991,8 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des, * -------- * * mss is fixed when enable tso, so w/o programming the TDES3 ctx field. - */ + * + * Called with netif_tx_lock held. */ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev) { u32 pay_len, mss;
@@ -2174,6 +2172,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev) * Description : this is the tx entry point of the driver. * It programs the chain or the ring and supports oversized frames * and SG feature. + * Called with netif_tx_lock held. */ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) {
@@ -2684,7 +2683,9 @@ static int stmmac_poll(struct napi_struct *napi, int budget) int work_done = 0; priv->xstats.napi_poll++; + netif_tx_lock_bh(priv->dev); stmmac_tx_clean(priv); + netif_tx_unlock_bh(priv->dev); work_done = stmmac_rx(priv, budget); if (work_done < budget) {
@@ -2701,6 +2702,7 @@ static int stmmac_poll(struct napi_struct *napi, int budget) * complete within a reasonable time. The driver will mark the error in the * netdev structure and arrange for the device to be reset to a sane state * in order to transmit a new packet. + * Called with netif_tx_lock held. */ static void stmmac_tx_timeout(struct net_device *dev) {
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.htmlAttachments
- signature.asc [application/pgp-signature] 181 bytes