From patchwork Thu Nov 17 01:46:03 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: david decotigny X-Patchwork-Id: 126102 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 18A33B71E5 for ; Thu, 17 Nov 2011 12:46:33 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755193Ab1KQBqN (ORCPT ); Wed, 16 Nov 2011 20:46:13 -0500 Received: from mail-ww0-f74.google.com ([74.125.82.74]:56252 "EHLO mail-ww0-f74.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752620Ab1KQBqM (ORCPT ); Wed, 16 Nov 2011 20:46:12 -0500 Received: by wwe5 with SMTP id 5so27472wwe.1 for ; Wed, 16 Nov 2011 17:46:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=beta; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references :organization:x-system-of-record; bh=jNn+Tcch5GK9HtUBC3+r01aCAjbFGoU7a7lz44J0i8Y=; b=KuZkbJuJsj1EhdXnjF0Yd9mO9zTZbpRHqM/48n2tHEqmMEUWwz9Hpk6+9B2nja4v+8 cd0C4dDHcTABktd/JxXA== Received: by 10.14.13.2 with SMTP id a2mr2024389eea.14.1321494370447; Wed, 16 Nov 2011 17:46:10 -0800 (PST) Received: by 10.14.13.2 with SMTP id a2mr2024339eea.14.1321494369906; Wed, 16 Nov 2011 17:46:09 -0800 (PST) Received: from hpza10.eem.corp.google.com ([74.125.121.33]) by gmr-mx.google.com with ESMTPS id 4si18576830eew.1.2011.11.16.17.46.09 (version=TLSv1/SSLv3 cipher=AES128-SHA); Wed, 16 Nov 2011 17:46:09 -0800 (PST) Received: from wpaz5.hot.corp.google.com (wpaz5.hot.corp.google.com [172.24.198.69]) by hpza10.eem.corp.google.com (Postfix) with ESMTPS id 9B99C20004E; Wed, 16 Nov 2011 17:46:09 -0800 (PST) Received: from decotigny.mtv.corp.google.com (decotigny.mtv.corp.google.com [172.18.64.159]) by wpaz5.hot.corp.google.com with ESMTP id pAH1k5ub021105; Wed, 16 Nov 2011 17:46:06 -0800 Received: by decotigny.mtv.corp.google.com (Postfix, from userid 128857) id B90CD26B4D; Wed, 16 Nov 2011 17:46:05 -0800 (PST) From: David Decotigny To: Jeff Kirsher , Jesse Brandeburg , Bruce Allan , Carolyn Wyborny , Don Skidmore , Greg Rose , Peter P Waskiewicz Jr , Alex Duyck , John Ronciak , e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Paul Gortmaker , David Decotigny , Ying Cai Subject: [PATCH FIX net v2] net-e1000(e): Fix default interrupt throttle rate not set in NIC HW Date: Wed, 16 Nov 2011 17:46:03 -0800 Message-Id: <75e944ced8ad7c58a0b838c0fe2a9e315f9e0c37.1321494268.git.david.decotigny@google.com> X-Mailer: git-send-email 1.7.3.1 In-Reply-To: References: Organization: Google, Inc. X-System-Of-Record: true Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Ying Cai This change ensures that the itr/itr_setting adjustment logic is used, even for the default/compiled-in value. Context: When we changed the default InterruptThrottleRate value from default (3 = dynamic mode) to 8000 for example, only adapter->itr_setting (which controls interrupt coalescing mode) was set to 8000, but adapter->itr (which controls the value set in NIC register) was not updated accordingly. So from ethtool, it seemed the interrupt throttling is enabled at 8000 intr/s, but the NIC actually was running in dynamic mode which has lower CPU efficiency especially when throughput is not high. Signed-off-by: David Decotigny --- drivers/net/ethernet/intel/e1000/e1000_param.c | 81 +++++++++++-------- drivers/net/ethernet/intel/e1000e/param.c | 98 +++++++++++++----------- 2 files changed, 99 insertions(+), 80 deletions(-) diff --git a/drivers/net/ethernet/intel/e1000/e1000_param.c b/drivers/net/ethernet/intel/e1000/e1000_param.c index 1301eba..595e462 100644 --- a/drivers/net/ethernet/intel/e1000/e1000_param.c +++ b/drivers/net/ethernet/intel/e1000/e1000_param.c @@ -173,7 +173,7 @@ E1000_PARAM(RxAbsIntDelay, "Receive Absolute Interrupt Delay"); /* Interrupt Throttle Rate (interrupts/sec) * - * Valid Range: 100-100000 (0=off, 1=dynamic, 3=dynamic conservative) + * Valid Range: 100-100000 or one of: 0=off, 1=dynamic, 3=dynamic conservative */ E1000_PARAM(InterruptThrottleRate, "Interrupt Throttling Rate"); #define DEFAULT_ITR 3 @@ -460,41 +460,54 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) }; if (num_InterruptThrottleRate > bd) { - adapter->itr = InterruptThrottleRate[bd]; - switch (adapter->itr) { - case 0: - e_dev_info("%s turned off\n", opt.name); - break; - case 1: - e_dev_info("%s set to dynamic mode\n", - opt.name); - adapter->itr_setting = adapter->itr; - adapter->itr = 20000; - break; - case 3: - e_dev_info("%s set to dynamic conservative " - "mode\n", opt.name); - adapter->itr_setting = adapter->itr; - adapter->itr = 20000; - break; - case 4: - e_dev_info("%s set to simplified " - "(2000-8000) ints mode\n", opt.name); - adapter->itr_setting = adapter->itr; - break; - default: - e1000_validate_option(&adapter->itr, &opt, - adapter); - /* save the setting, because the dynamic bits - * change itr. - * clear the lower two bits because they are - * used as control */ - adapter->itr_setting = adapter->itr & ~3; - break; - } + /* Make sure a message is printed for + * non-special values. And in case of an + * invalid option, display warning, use + * default and go through itr/itr_setting + * adjustment logic below */ + if ((adapter->itr < 0 || adapter->itr > 4) + && e1000_validate_option(&adapter->itr, &opt, + adapter)) + adapter->itr = opt.def; } else { - adapter->itr_setting = opt.def; + /* if no option specified, use default value + and go through the logic below to adjust + itr/itr_setting */ + adapter->itr = opt.def; + + /* Make sure a message is printed for + * non-special default values */ + if (adapter->itr < 0 || adapter->itr > 4) + e_dev_info("%s set to default %d\n", + opt.name, adapter->itr); + } + + adapter->itr_setting = adapter->itr; + switch (adapter->itr) { + case 0: + e_dev_info("%s turned off\n", opt.name); + break; + case 1: + e_dev_info("%s set to dynamic mode\n", + opt.name); + adapter->itr = 20000; + break; + case 3: + e_dev_info("%s set to dynamic conservative " + "mode\n", opt.name); adapter->itr = 20000; + break; + case 4: + e_dev_info("%s set to simplified " + "(2000-8000) ints mode\n", opt.name); + break; + default: + /* save the setting, because the dynamic bits + * change itr. + * clear the lower two bits because they are + * used as control */ + adapter->itr_setting &= ~3; + break; } } { /* Smart Power Down */ diff --git a/drivers/net/ethernet/intel/e1000e/param.c b/drivers/net/ethernet/intel/e1000e/param.c index 20e93b0..41937e5 100644 --- a/drivers/net/ethernet/intel/e1000e/param.c +++ b/drivers/net/ethernet/intel/e1000e/param.c @@ -106,7 +106,7 @@ E1000_PARAM(RxAbsIntDelay, "Receive Absolute Interrupt Delay"); /* * Interrupt Throttle Rate (interrupts/sec) * - * Valid Range: 100-100000 (0=off, 1=dynamic, 3=dynamic conservative) + * Valid Range: 100-100000 or one of: 0=off, 1=dynamic, 3=dynamic conservative */ E1000_PARAM(InterruptThrottleRate, "Interrupt Throttling Rate"); #define DEFAULT_ITR 3 @@ -335,53 +335,59 @@ void __devinit e1000e_check_options(struct e1000_adapter *adapter) if (num_InterruptThrottleRate > bd) { adapter->itr = InterruptThrottleRate[bd]; - switch (adapter->itr) { - case 0: - e_info("%s turned off\n", opt.name); - break; - case 1: - e_info("%s set to dynamic mode\n", opt.name); - adapter->itr_setting = adapter->itr; - adapter->itr = 20000; - break; - case 3: - e_info("%s set to dynamic conservative mode\n", - opt.name); - adapter->itr_setting = adapter->itr; - adapter->itr = 20000; - break; - case 4: - e_info("%s set to simplified (2000-8000 ints) " - "mode\n", opt.name); - adapter->itr_setting = 4; - break; - default: - /* - * Save the setting, because the dynamic bits - * change itr. - */ - if (e1000_validate_option(&adapter->itr, &opt, - adapter) && - (adapter->itr == 3)) { - /* - * In case of invalid user value, - * default to conservative mode. - */ - adapter->itr_setting = adapter->itr; - adapter->itr = 20000; - } else { - /* - * Clear the lower two bits because - * they are used as control. - */ - adapter->itr_setting = - adapter->itr & ~3; - } - break; - } + + /* Make sure a message is printed for + * non-special values. And in case of an + * invalid option, display warning, use + * default and go through itr/itr_setting + * adjustment logic below */ + if ((adapter->itr < 0 || adapter->itr > 4) + && e1000_validate_option(&adapter->itr, &opt, + adapter)) + adapter->itr = opt.def; } else { - adapter->itr_setting = opt.def; + /* if no option specified, use default value + and go through the logic below to adjust + itr/itr_setting */ + adapter->itr = opt.def; + + /* Make sure a message is printed for + * non-special default values */ + if (adapter->itr < 0 || adapter->itr > 4) + e_info("%s set to default %d\n", + opt.name, adapter->itr); + } + + + + adapter->itr_setting = adapter->itr; + switch (adapter->itr) { + case 0: + e_info("%s turned off\n", opt.name); + break; + case 1: + e_info("%s set to dynamic mode\n", opt.name); + adapter->itr = 20000; + break; + case 3: + e_info("%s set to dynamic conservative mode\n", + opt.name); adapter->itr = 20000; + break; + case 4: + e_info("%s set to simplified (2000-8000 ints) " + "mode\n", opt.name); + break; + default: + /* + * Save the setting, because the dynamic bits + * change itr. + * + * Clear the lower two bits because + * they are used as control. + */ + adapter->itr_setting &= ~3; + break; } } { /* Interrupt Mode */