From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750800Ab3IFFDK (ORCPT ); Fri, 6 Sep 2013 01:03:10 -0400 Received: from shards.monkeyblade.net ([149.20.54.216]:59484 "EHLO shards.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750720Ab3IFFDI (ORCPT ); Fri, 6 Sep 2013 01:03:08 -0400 Date: Fri, 06 Sep 2013 01:03:03 -0400 (EDT) Message-Id: <20130906.010303.1964966449906114324.davem@davemloft.net> To: Vineet.Gupta1@synopsys.com Cc: netdev@vger.kernel.org, Alexey.Brodkin@synopsys.com, romieu@fr.zoreil.com, linux-kernel@vger.kernel.org, arc-linux-dev@synopsys.com Subject: Re: [PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit From: David Miller In-Reply-To: References: <1378299791-24598-1-git-send-email-vgupta@synopsys.com> <20130905.142436.685399325201245026.davem@davemloft.net> X-Mailer: Mew version 6.5 on Emacs 24.1 / Mule 6.0 (HANACHIRUSATO) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.1 (shards.monkeyblade.net [0.0.0.0]); Thu, 05 Sep 2013 22:03:06 -0700 (PDT) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Vineet Gupta Date: Fri, 6 Sep 2013 04:24:39 +0000 > 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.