netdev - Re: [PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 6 Sep 2013 04:24:39 +0000
From:	Vineet Gupta <Vineet.Gupta1@...opsys.com>
To:	David Miller <davem@...emloft.net>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Alexey.Brodkin@...opsys.com" <Alexey.Brodkin@...opsys.com>,
	"romieu@...zoreil.com" <romieu@...zoreil.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"arc-linux-dev@...opsys.com" <arc-linux-dev@...opsys.com>
Subject: 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.Gupta1@...opsys.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 <vgupta@...opsys.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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html