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 <redacted>
---
drivers/net/ethernet/freescale/ucc_geth.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
@@ -3181,8 +3181,6 @@ 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 */
@@ -3196,6 +3194,7 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)(ugeth->skb_curtx[txQ]+1)&TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);+spin_lock_irqsave(&ugeth->lock,flags);/* set up the buffer descriptor */out_be32(&((structqe_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))
UCC controller can shift received Ethernet frames 2 bytes into the buffer,
making IP data word aligned, this patch enables that feature.
Signed-off-by: Joakim Tjernlund <redacted>
---
drivers/net/ethernet/freescale/ucc_geth.c | 19 +++++++++++--------
drivers/net/ethernet/freescale/ucc_geth.h | 1 +
2 files changed, 12 insertions(+), 8 deletions(-)
We still see dropped pkgs in a busy network, this makes it better.
Signed-off-by: Joakim Tjernlund <redacted>
---
drivers/net/ethernet/freescale/ucc_geth.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
@@ -2390,7 +2402,7 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)bd=ugeth->rxBd[j]=ugeth->p_rx_bd_ring[j];for(i=0;i<ug_info->bdRingLenRx[j];i++){/* set bd status and length */-out_be32((u32__iomem*)bd,R_I);+out_be32((u32__iomem*)bd,0);/* clear bd buffer */out_be32(&((structqe_bd__iomem*)bd)->buf,0);bd+=sizeof(structqe_bd);
Francois Romieu [off-list ref] wrote on 2012/09/19 00:39:38:
Joakim Tjernlund [off-list ref] :
quoted
Currently ucc_geth_start_xmit wraps IRQ off for the
whole body just to be safe.
Reduce the IRQ off period to a minimum.
The driver does not do much work in its irq handler. You may as well
convert it to the usual tg3-ish locking style (i.e. almost no locking).
You mean broadcom/tg3.c? It is a bit much to look at ATM for me and
there almost no locking with my patch also. Could possibly
be improved further but I am happy for now.
Jocke
Currently ucc_geth_start_xmit wraps IRQ off for the
whole body just to be safe.
Reduce the IRQ off period to a minimum.
It opens a window in ucc_geth_start_xmit where the skb slot in
ugeth->tx_skbuff[txQ] is set and T_RA has not been written into
the descriptor status. Consider a racing poll : the !skb test in
ucc_geth_tx may not work as expected.
--
Ueimor
Francois Romieu [off-list ref] wrote on 2012/09/20 00:34:16:
Joakim Tjernlund [off-list ref] :
quoted
Currently ucc_geth_start_xmit wraps IRQ off for the
whole body just to be safe.
Reduce the IRQ off period to a minimum.
It opens a window in ucc_geth_start_xmit where the skb slot in
ugeth->tx_skbuff[txQ] is set and T_RA has not been written into
the descriptor status. Consider a racing poll : the !skb test in
ucc_geth_tx may not work as expected.
Right, good catch!
Surprisingly the driver never showed any malfunction even though I hit
it pretty hard.
I will send a V2 of this patch where I move the assignment inside the IRQ
off part:
@@ -3200,8 +3200,6 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)/* 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]=
@@ -3209,6 +3207,8 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)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(&((structqe_bd__iomem*)bd)->buf,dma_map_single(ugeth->dev,skb->data,Jocke
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 <redacted>
---
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 [off-list ref]
drivers/net/ethernet/freescale/ucc_geth.c | 11 +++++------
1 files changed, 5 insertions(+), 6 deletions(-)
@@ -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(&((structqe_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))
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 <redacted>
---
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 [off-list ref]
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.
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 <redacted>
---
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 [off-list ref]
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
Currently ucc_geth_start_xmit wraps IRQ off for the
whole body just to be safe. By rearranging the code a bit
one can avoid the lock completely.
Signed-off-by: Joakim Tjernlund <redacted>
---
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 [off-list ref]
v3: Lockless xmit
Here is my attemept to do lockless xmit. Thanks to
Francois Romieu [off-list ref] for the idea.
drivers/net/ethernet/freescale/ucc_geth.c | 17 ++++++++---------
1 files changed, 8 insertions(+), 9 deletions(-)
@@ -3177,19 +3177,20 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)u8__iomem*bd;/* BD pointer */u32bd_status;u8txQ=0;-unsignedlongflags;ugeth_vdbg("%s: IN",__func__);-spin_lock_irqsave(&ugeth->lock,flags);-dev->stats.tx_bytes+=skb->len;+/* We are running in BH disabled context with netif_tx_lock+*andTXreclaimrunsviatp->napi.pollinsideofasoftware+*interrupt.Furthermore,IRQprocessingrunslocklesssowehave+*noIRQcontextdeadlockstoworryabouteither.Rejoice!+*/+/* 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]=
@@ -3207,6 +3208,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);+/* Save the skb pointer so we can free it later */+ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]]=skb;/* Move to next BD in the ring */if(!(bd_status&T_W))
Currently ucc_geth_start_xmit wraps IRQ off for the
whole body just to be safe. By rearranging the code a bit
one can avoid the lock completely.
Afaics you went a bit too lockless with the queueing disable / enable
logic. The hard_start_xmit handler is run in a locally softirq disabled
section but it will happily race with the napi handler on a different
CPU. Grep netif_tx_lock in tg3.c for it.
The Tx skb free logic probably requires some smp memory barriers as
well since the current skb is used by the ucc_geth driver to sync the
Tx xmit with the napi completion handler.
--
Ueimor
Ping? Got no comments and I can see it in net or net-next trees either.
Joakim Tjernlund [off-list ref] wrote on 2012/09/18 18:56:22:
quoted hunk
From: Joakim Tjernlund <redacted>
To: netdev@vger.kernel.org,
Cc: Joakim Tjernlund <redacted>
Date: 2012/09/18 18:56
Subject: [PATCH 2/5] ucc_geth: Word align Ethernet RX data
UCC controller can shift received Ethernet frames 2 bytes into the buffer,
making IP data word aligned, this patch enables that feature.
Signed-off-by: Joakim Tjernlund <redacted>
---
drivers/net/ethernet/freescale/ucc_geth.c | 19 +++++++++++--------
drivers/net/ethernet/freescale/ucc_geth.h | 1 +
2 files changed, 12 insertions(+), 8 deletions(-)
Ping? Got no comments and I can see it in net or net-next trees either.
Joakim Tjernlund [off-list ref] wrote on 2012/09/18 18:56:24:
quoted hunk
From: Joakim Tjernlund <redacted>
To: netdev@vger.kernel.org,
Cc: Joakim Tjernlund <redacted>
Date: 2012/09/18 18:56
Subject: [PATCH 4/5] ucc_geth: Increase RX ring buffer from 32 to 64
We still see dropped pkgs in a busy network, this makes it better.
Signed-off-by: Joakim Tjernlund <redacted>
---
drivers/net/ethernet/freescale/ucc_geth.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
@@ -2390,7 +2402,7 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)bd=ugeth->rxBd[j]=ugeth->p_rx_bd_ring[j];for(i=0;i<ug_info->bdRingLenRx[j];i++){/* set bd status and length */-out_be32((u32__iomem*)bd,R_I);+out_be32((u32__iomem*)bd,0);/* clear bd buffer */out_be32(&((structqe_bd__iomem*)bd)->buf,0);bd+=sizeof(structqe_bd);
Ping? Got no comments and I can see it in net or net-next trees either.
No patch will be applied to the tree when one of the indiviual patches
get feedback and request changes from you.
When an individual patch must be redone, you must resend the entire
series not just the individual patch which changed.
You also never need to ask the kind of question you are asking here,
you simply need to look into patch work to see what the state of
your patches is:
I maintain this state exactly so people don't need to waste precious
developer time asking "what is the state of my patch" like you are
making me do right here.
See:
http://patchwork.ozlabs.org/project/netdev/list/?state=*&q=ucc_geth
And as you can see this entire series is marked as either "RFC"
or "Changes Requested"
Even worse, you did this for not one but several of the patches you
posted.
Ping? Got no comments and I can see it in net or net-next trees either.
No patch will be applied to the tree when one of the indiviual patches
get feedback and request changes from you.
When an individual patch must be redone, you must resend the entire
series not just the individual patch which changed.
oh, I see what I did wrong. I sent them as series which may have internal dependencies.
However, each patch can be applied on its own.
You also never need to ask the kind of question you are asking here,
you simply need to look into patch work to see what the state of
your patches is:
I maintain this state exactly so people don't need to waste precious
developer time asking "what is the state of my patch" like you are
making me do right here.
See:
http://patchwork.ozlabs.org/project/netdev/list/?state=*&q=ucc_geth
Ahh, better bookmark this page for future use.
And as you can see this entire series is marked as either "RFC"
or "Changes Requested"
Yes, I see that now.
I guess I should resend each patch separately then?
(Possibly wait until I have figured out the lockless TX stuff)
Even worse, you did this for not one but several of the patches you
posted.