[RFC] Re: Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

From: Pavel Machek
Date: Wed Dec 07 2016 - 07:31:48 EST


On Fri 2016-12-02 15:05:21, Lino Sanfilippo wrote:
> Hi,
>
>
> >
> > 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 <pavel@xxxxxx>
Date: Wed Dec 7 13:29:15 2016 +0100

Reported-by: Lino Sanfilippo <LinoSanfilippo@xxxxxx>

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 <pavel@xxxxxxx>

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.html

Attachment: signature.asc
Description: Digital signature