| Message ID | 1348129021-28333-1-git-send-email-Joakim.Tjernlund@transmode.se |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> Date: Thu, 20 Sep 2012 10:17:01 +0200 > Currently ucc_geth_start_xmit wraps IRQ off for the > whole body just to be safe. > Reduce the IRQ off period to a minimum. > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> > --- > > v2: Move assignment of ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] > inside IRQ off section to prevent racing against > ucc_geth_tx(). Spotted by Francois Romieu <romieu@fr.zoreil.com> I agree with Francois's initial analysis, and disagree with you're response to him, wrt. the suggest to remove all locking entirely. Unlike what you claim, there isn't much of a gain at all from merely make the window of lock holding smaller, especially on the scale in which you are doing it here. Whereas removing the lock and the atomic completely, as tg3 does, will give very significant performance gains. The locking cost of grabbing the spinlock, and the memory transactions associated with it, dominate. Furthermore, even if the gains of your change are non-trivial, you haven't documented it. So unless you should some noticable gains from this, it's just code masterbation as far as I'm concerned and I'm therefore inclined to not apply patches like this. TG3's core interrupt locking is not that difficult to understand and replicate in other drivers, so I dismiss your attempts to avoid that approach on difficulty grounds as well. -- 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 Miller <davem@davemloft.net> wrote on 2012/09/20 23:08:52: > > From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> > Date: Thu, 20 Sep 2012 10:17:01 +0200 > > > Currently ucc_geth_start_xmit wraps IRQ off for the > > whole body just to be safe. > > Reduce the IRQ off period to a minimum. > > > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> > > --- > > > > v2: Move assignment of ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] > > inside IRQ off section to prevent racing against > > ucc_geth_tx(). Spotted by Francois Romieu <romieu@fr.zoreil.com> > > I agree with Francois's initial analysis, and disagree with you're > response to him, wrt. the suggest to remove all locking entirely. > > Unlike what you claim, there isn't much of a gain at all from merely > make the window of lock holding smaller, especially on the scale > in which you are doing it here. > > Whereas removing the lock and the atomic completely, as tg3 does, > will give very significant performance gains. > > The locking cost of grabbing the spinlock, and the memory transactions > associated with it, dominate. > > Furthermore, even if the gains of your change are non-trivial, you > haven't documented it. So unless you should some noticable gains from > this, it's just code masterbation as far as I'm concerned and I'm > therefore inclined to not apply patches like this. > > TG3's core interrupt locking is not that difficult to understand and > replicate in other drivers, so I dismiss your attempts to avoid that > approach on difficulty grounds as well. OK, I will give it a go. Got something working that I will send in shortly. I hope the other patches were OK? Jocke -- 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/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c index 9ac14f8..0100bca 100644 --- a/drivers/net/ethernet/freescale/ucc_geth.c +++ b/drivers/net/ethernet/freescale/ucc_geth.c @@ -3181,21 +3181,20 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev) ugeth_vdbg("%s: IN", __func__); - spin_lock_irqsave(&ugeth->lock, flags); - dev->stats.tx_bytes += skb->len; /* Start from the next BD that should be filled */ bd = ugeth->txBd[txQ]; bd_status = in_be32((u32 __iomem *)bd); - /* Save the skb pointer so we can free it later */ - ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] = skb; /* Update the current skb pointer (wrapping if this was the last) */ ugeth->skb_curtx[txQ] = (ugeth->skb_curtx[txQ] + 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]); + spin_lock_irqsave(&ugeth->lock, flags); + /* Save the skb pointer so we can free it later */ + ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] = skb; /* set up the buffer descriptor */ out_be32(&((struct qe_bd __iomem *)bd)->buf, dma_map_single(ugeth->dev, skb->data, @@ -3207,6 +3206,8 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev) /* set bd status and length */ out_be32((u32 __iomem *)bd, bd_status); + spin_unlock_irqrestore(&ugeth->lock, flags); + /* Move to next BD in the ring */ if (!(bd_status & T_W)) @@ -3238,8 +3239,6 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev) uccf = ugeth->uccf; out_be16(uccf->p_utodr, UCC_FAST_TOD); #endif - spin_unlock_irqrestore(&ugeth->lock, flags); - return NETDEV_TX_OK; }
Currently ucc_geth_start_xmit wraps IRQ off for the whole body just to be safe. Reduce the IRQ off period to a minimum. Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> --- v2: Move assignment of ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] inside IRQ off section to prevent racing against ucc_geth_tx(). Spotted by Francois Romieu <romieu@fr.zoreil.com> drivers/net/ethernet/freescale/ucc_geth.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)