From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [net-next 04/14] net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state Date: Sat, 21 Apr 2012 09:27:31 -0700 Message-ID: <4F92DFF3.2080301@intel.com> References: <1331611432-30109-1-git-send-email-jeffrey.t.kirsher@intel.com> <1331611432-30109-5-git-send-email-jeffrey.t.kirsher@intel.com> <4F91D5A2.2060300@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Alexander Duyck , Alexander Duyck , Jeff Kirsher , davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com To: Tom Herbert Return-path: Received: from mga11.intel.com ([192.55.52.93]:40958 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750829Ab2DUQ1c (ORCPT ); Sat, 21 Apr 2012 12:27:32 -0400 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On 4/20/2012 11:01 PM, Tom Herbert wrote: >> I don't recall the exact reason now, as John said I think it had to do >> with the ethtool tests not using the same cleanup routine and leaving >> us in a bad state. I am pretty sure there was some path in which >> where the call was didn't work but I do not recall the exact details >> now. Most of the reason for moving it is due to the fact that the >> reset is now also clearing the bit, and from the driver perspective we >> didn't need it in two places. After looking it all over again, I >> suppose this causes a cosmetic issue for the bql "inflight" statistic >> in sysfs since the value will be retained until the interface is >> brought back up with this change. Is that an issue or something that >> can be lived with since the interface isn't active anyway in this >> case? >> > On the surface, it seems cleaner and probably a simpler convention to > clear the state when the buffers are being freed. If there's a good > reason not to do this for igb, it makes me wonder if this should be > done the same way in other drivers... > > Tom Just dug up the old thread. This was to fix a bug that byte queue limits was causing with some of the loopback tests evoked from ethtool. The loopback test uses a separate path to free the buffers which wasn't calling netdev_tx_reset_queue(). We should likely just explicitly clear the state in each routine rather than try to be clever IMO. .John