Re: [PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit
From: Vineet Gupta
Date: Fri, 6 Sep 2013 04:24:39 +
> On 09/05/2013 11:54 PM, David Miller wrote:
>> You should keep the check in the transmit queueing code as a BUG check,
>> almost every driver has code of the form (using NIU as an example):
...
>> Otherwise queue management bugs are incredibly hard to diagnose.
>>
>> I'm not applying this patch.
>
> The check is already there for current BD. What I removed was checking for
> next BD
> too (please see below). IMHO this is useless since it will be done in next
> iteration anyways. In my tests, the next check never got hit, so it was waste
> of
> cycles.
>
> static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
> {
> if (unlikely((le32_to_cpu(*info) & OWN_MASK) == FOR_EMAC)) {
> netif_stop_queue(ndev);
> return NETDEV_TX_BUSY;
> }
>
> ...
> *txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
>
> - /* Get "info" of the next BD */
> - info = >txbd[*txbd_curr].info;
> -
> - /* Check if if Tx BD ring is full - next BD is still owned by EMAC */
> - if (unlikely((le32_to_cpu(*info) & OWN_MASK) == FOR_EMAC))
> - netif_stop_queue(ndev);
>
> OTOH, I do see a slight stats update issue - if the queue is stopped (but pkt
> not
> dropped) we are failing to increment tx_errors. But that would be a separate
> patch.
It is exactly the correct thing to do. The driver should _NEVER_
return NETDEV_TX_BUSY under normal circumstances. The queue should
always be stopped by the ->ndo_start_xmit() method when it fills the
queue.
Again, when ->ndo_start_xmit() is invoked, it should never see the
queue full. When that happens it is a bug.
You are deleting exactly the correct part of this function, what it is
doing right now is precisely the correct way to manage netif queue
state.
The only valid change you can make here is to make the:
if (unlikely((le32_to_cpu(*info) & OWN_MASK) == FOR_EMAC)) {
netif_stop_queue(ndev);
return NETDEV_TX_BUSY;
}
print out an error message and increment tx_errors.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit
Hi David,
On 09/05/2013 11:54 PM, David Miller wrote:
> From: Vineet Gupta
> Date: Wed, 4 Sep 2013 18:33:11 +0530
>
>> This came out of staring at code due to recent performance fix.
>>
>> * TX BD reclaim can call netif_wake_queue() once, outside the loop if
>> one/more BDs were freed, NO need to do this each iteration.
>>
>> * TX need not look at next BD to stop the netif queue. It rather be done
>> in the next tx call, when it actually fails as the queue seldom gets
>> full but the check nevertheless needs to be done for each packet Tx.
>> Profiled this under heavy traffic (big tar file cp, LMBench betworking
>> tests) and saw not a single hit to that code.
>>
>> Signed-off-by: Vineet Gupta
> You should keep the check in the transmit queueing code as a BUG check,
> almost every driver has code of the form (using NIU as an example):
>
> if (niu_tx_avail(rp) <= (skb_shinfo(skb)->nr_frags + 1)) {
> netif_tx_stop_queue(txq);
> dev_err(np->device, "%s: BUG! Tx ring full when queue
> awake!\n", dev->name);
> rp->tx_errors++;
> return NETDEV_TX_BUSY;
> }
>
> and arc_emac should too.
>
> Otherwise queue management bugs are incredibly hard to diagnose.
>
> I'm not applying this patch.
The check is already there for current BD. What I removed was checking for next
BD
too (please see below). IMHO this is useless since it will be done in next
iteration anyways. In my tests, the next check never got hit, so it was waste of
cycles.
static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
{
if (unlikely((le32_to_cpu(*info) & OWN_MASK) == FOR_EMAC)) {
netif_stop_queue(ndev);
return NETDEV_TX_BUSY;
}
...
*txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
- /* Get "info" of the next BD */
- info = >txbd[*txbd_curr].info;
-
- /* Check if if Tx BD ring is full - next BD is still owned by EMAC */
- if (unlikely((le32_to_cpu(*info) & OWN_MASK) == FOR_EMAC))
- netif_stop_queue(ndev);
OTOH, I do see a slight stats update issue - if the queue is stopped (but pkt
not
dropped) we are failing to increment tx_errors. But that would be a separate
patch.
-Vineet
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit
From: Vineet Gupta
Date: Wed, 4 Sep 2013 18:33:11 +0530
> This came out of staring at code due to recent performance fix.
>
> * TX BD reclaim can call netif_wake_queue() once, outside the loop if
> one/more BDs were freed, NO need to do this each iteration.
>
> * TX need not look at next BD to stop the netif queue. It rather be done
> in the next tx call, when it actually fails as the queue seldom gets
> full but the check nevertheless needs to be done for each packet Tx.
> Profiled this under heavy traffic (big tar file cp, LMBench betworking
> tests) and saw not a single hit to that code.
>
> Signed-off-by: Vineet Gupta
You should keep the check in the transmit queueing code as a BUG check,
almost every driver has code of the form (using NIU as an example):
if (niu_tx_avail(rp) <= (skb_shinfo(skb)->nr_frags + 1)) {
netif_tx_stop_queue(txq);
dev_err(np->device, "%s: BUG! Tx ring full when queue
awake!\n", dev->name);
rp->tx_errors++;
return NETDEV_TX_BUSY;
}
and arc_emac should too.
Otherwise queue management bugs are incredibly hard to diagnose.
I'm not applying this patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit
From: Vineet Gupta vineet.gup...@synopsys.com
Date: Wed, 4 Sep 2013 18:33:11 +0530
This came out of staring at code due to recent performance fix.
* TX BD reclaim can call netif_wake_queue() once, outside the loop if
one/more BDs were freed, NO need to do this each iteration.
* TX need not look at next BD to stop the netif queue. It rather be done
in the next tx call, when it actually fails as the queue seldom gets
full but the check nevertheless needs to be done for each packet Tx.
Profiled this under heavy traffic (big tar file cp, LMBench betworking
tests) and saw not a single hit to that code.
Signed-off-by: Vineet Gupta vgu...@synopsys.com
You should keep the check in the transmit queueing code as a BUG check,
almost every driver has code of the form (using NIU as an example):
if (niu_tx_avail(rp) = (skb_shinfo(skb)-nr_frags + 1)) {
netif_tx_stop_queue(txq);
dev_err(np-device, %s: BUG! Tx ring full when queue
awake!\n, dev-name);
rp-tx_errors++;
return NETDEV_TX_BUSY;
}
and arc_emac should too.
Otherwise queue management bugs are incredibly hard to diagnose.
I'm not applying this patch.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit
Hi David,
On 09/05/2013 11:54 PM, David Miller wrote:
From: Vineet Gupta vineet.gup...@synopsys.com
Date: Wed, 4 Sep 2013 18:33:11 +0530
This came out of staring at code due to recent performance fix.
* TX BD reclaim can call netif_wake_queue() once, outside the loop if
one/more BDs were freed, NO need to do this each iteration.
* TX need not look at next BD to stop the netif queue. It rather be done
in the next tx call, when it actually fails as the queue seldom gets
full but the check nevertheless needs to be done for each packet Tx.
Profiled this under heavy traffic (big tar file cp, LMBench betworking
tests) and saw not a single hit to that code.
Signed-off-by: Vineet Gupta vgu...@synopsys.com
You should keep the check in the transmit queueing code as a BUG check,
almost every driver has code of the form (using NIU as an example):
if (niu_tx_avail(rp) = (skb_shinfo(skb)-nr_frags + 1)) {
netif_tx_stop_queue(txq);
dev_err(np-device, %s: BUG! Tx ring full when queue
awake!\n, dev-name);
rp-tx_errors++;
return NETDEV_TX_BUSY;
}
and arc_emac should too.
Otherwise queue management bugs are incredibly hard to diagnose.
I'm not applying this patch.
The check is already there for current BD. What I removed was checking for next
BD
too (please see below). IMHO this is useless since it will be done in next
iteration anyways. In my tests, the next check never got hit, so it was waste of
cycles.
static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
{
if (unlikely((le32_to_cpu(*info) OWN_MASK) == FOR_EMAC)) {
netif_stop_queue(ndev);
return NETDEV_TX_BUSY;
}
...
*txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
- /* Get info of the next BD */
- info = priv-txbd[*txbd_curr].info;
-
- /* Check if if Tx BD ring is full - next BD is still owned by EMAC */
- if (unlikely((le32_to_cpu(*info) OWN_MASK) == FOR_EMAC))
- netif_stop_queue(ndev);
OTOH, I do see a slight stats update issue - if the queue is stopped (but pkt
not
dropped) we are failing to increment tx_errors. But that would be a separate
patch.
-Vineet
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit
From: Vineet Gupta vineet.gup...@synopsys.com
Date: Fri, 6 Sep 2013 04:24:39 +
On 09/05/2013 11:54 PM, David Miller wrote:
You should keep the check in the transmit queueing code as a BUG check,
almost every driver has code of the form (using NIU as an example):
...
Otherwise queue management bugs are incredibly hard to diagnose.
I'm not applying this patch.
The check is already there for current BD. What I removed was checking for
next BD
too (please see below). IMHO this is useless since it will be done in next
iteration anyways. In my tests, the next check never got hit, so it was waste
of
cycles.
static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
{
if (unlikely((le32_to_cpu(*info) OWN_MASK) == FOR_EMAC)) {
netif_stop_queue(ndev);
return NETDEV_TX_BUSY;
}
...
*txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
- /* Get info of the next BD */
- info = priv-txbd[*txbd_curr].info;
-
- /* Check if if Tx BD ring is full - next BD is still owned by EMAC */
- if (unlikely((le32_to_cpu(*info) OWN_MASK) == FOR_EMAC))
- netif_stop_queue(ndev);
OTOH, I do see a slight stats update issue - if the queue is stopped (but pkt
not
dropped) we are failing to increment tx_errors. But that would be a separate
patch.
It is exactly the correct thing to do. The driver should _NEVER_
return NETDEV_TX_BUSY under normal circumstances. The queue should
always be stopped by the -ndo_start_xmit() method when it fills the
queue.
Again, when -ndo_start_xmit() is invoked, it should never see the
queue full. When that happens it is a bug.
You are deleting exactly the correct part of this function, what it is
doing right now is precisely the correct way to manage netif queue
state.
The only valid change you can make here is to make the:
if (unlikely((le32_to_cpu(*info) OWN_MASK) == FOR_EMAC)) {
netif_stop_queue(ndev);
return NETDEV_TX_BUSY;
}
print out an error message and increment tx_errors.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit
This came out of staring at code due to recent performance fix. * TX BD reclaim can call netif_wake_queue() once, outside the loop if one/more BDs were freed, NO need to do this each iteration. * TX need not look at next BD to stop the netif queue. It rather be done in the next tx call, when it actually fails as the queue seldom gets full but the check nevertheless needs to be done for each packet Tx. Profiled this under heavy traffic (big tar file cp, LMBench betworking tests) and saw not a single hit to that code. Signed-off-by: Vineet Gupta Cc: Alexey Brodkin Cc: "David S. Miller" Cc: Francois Romieu Cc: net...@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: arc-linux-...@synopsys.com --- drivers/net/ethernet/arc/emac_main.c | 13 +++-- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/arc/emac_main.c b/drivers/net/ethernet/arc/emac_main.c index 9e16014..a3dd048 100644 --- a/drivers/net/ethernet/arc/emac_main.c +++ b/drivers/net/ethernet/arc/emac_main.c @@ -179,10 +179,10 @@ static void arc_emac_tx_clean(struct net_device *ndev) txbd->info = 0; *txbd_dirty = (*txbd_dirty + 1) % TX_BD_NUM; - - if (netif_queue_stopped(ndev)) - netif_wake_queue(ndev); } + + if (i && netif_queue_stopped(ndev)) + netif_wake_queue(ndev); } /** @@ -570,13 +570,6 @@ static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev) /* Increment index to point to the next BD */ *txbd_curr = (*txbd_curr + 1) % TX_BD_NUM; - /* Get "info" of the next BD */ - info = >txbd[*txbd_curr].info; - - /* Check if if Tx BD ring is full - next BD is still owned by EMAC */ - if (unlikely((le32_to_cpu(*info) & OWN_MASK) == FOR_EMAC)) - netif_stop_queue(ndev); - arc_reg_set(priv, R_STATUS, TXPL_MASK); skb_tx_timestamp(skb); -- 1.8.1.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit
This came out of staring at code due to recent performance fix. * TX BD reclaim can call netif_wake_queue() once, outside the loop if one/more BDs were freed, NO need to do this each iteration. * TX need not look at next BD to stop the netif queue. It rather be done in the next tx call, when it actually fails as the queue seldom gets full but the check nevertheless needs to be done for each packet Tx. Profiled this under heavy traffic (big tar file cp, LMBench betworking tests) and saw not a single hit to that code. Signed-off-by: Vineet Gupta vgu...@synopsys.com Cc: Alexey Brodkin abrod...@synopsys.com Cc: David S. Miller da...@davemloft.net Cc: Francois Romieu rom...@fr.zoreil.com Cc: net...@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: arc-linux-...@synopsys.com --- drivers/net/ethernet/arc/emac_main.c | 13 +++-- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/arc/emac_main.c b/drivers/net/ethernet/arc/emac_main.c index 9e16014..a3dd048 100644 --- a/drivers/net/ethernet/arc/emac_main.c +++ b/drivers/net/ethernet/arc/emac_main.c @@ -179,10 +179,10 @@ static void arc_emac_tx_clean(struct net_device *ndev) txbd-info = 0; *txbd_dirty = (*txbd_dirty + 1) % TX_BD_NUM; - - if (netif_queue_stopped(ndev)) - netif_wake_queue(ndev); } + + if (i netif_queue_stopped(ndev)) + netif_wake_queue(ndev); } /** @@ -570,13 +570,6 @@ static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev) /* Increment index to point to the next BD */ *txbd_curr = (*txbd_curr + 1) % TX_BD_NUM; - /* Get info of the next BD */ - info = priv-txbd[*txbd_curr].info; - - /* Check if if Tx BD ring is full - next BD is still owned by EMAC */ - if (unlikely((le32_to_cpu(*info) OWN_MASK) == FOR_EMAC)) - netif_stop_queue(ndev); - arc_reg_set(priv, R_STATUS, TXPL_MASK); skb_tx_timestamp(skb); -- 1.8.1.2 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
