Just to make it clear, I am not opposed to what this code was trying
to do, in
fact I think its a pretty cool idea, but I think it can be implemented
more cleanly.
Eric and myself will be discussing the details.
Jack
On Tue, Dec 2, 2014 at 3:02 PM, Jack F Vogel <j...@freebsd.org
<mailto:j...@freebsd.org>> wrote:
Author: jfv
Date: Tue Dec 2 23:02:57 2014
New Revision: 275431
URL: https://svnweb.freebsd.org/changeset/base/275431
Log:
Revert r275136, it was not approved, it was sloppy, if a feature
like this is needed please resubmit for Intel's approval.
Modified:
head/sys/dev/e1000/if_igb.c
head/sys/dev/ixgbe/ixgbe.c
Modified: head/sys/dev/e1000/if_igb.c
==============================================================================
--- head/sys/dev/e1000/if_igb.c Tue Dec 2 22:35:43 2014 (r275430)
+++ head/sys/dev/e1000/if_igb.c Tue Dec 2 23:02:57 2014 (r275431)
@@ -188,7 +188,6 @@ static char *igb_strings[] = {
/*********************************************************************
* Function prototypes
*********************************************************************/
-static int igb_per_unit_num_queues(SYSCTL_HANDLER_ARGS);
static int igb_probe(device_t);
static int igb_attach(device_t);
static int igb_detach(device_t);
@@ -494,11 +493,6 @@ igb_attach(device_t dev)
OID_AUTO, "nvm", CTLTYPE_INT|CTLFLAG_RW, adapter, 0,
igb_sysctl_nvm_info, "I", "NVM Information");
- SYSCTL_ADD_PROC(device_get_sysctl_ctx(dev),
- SYSCTL_CHILDREN(device_get_sysctl_tree(dev)),
- OID_AUTO, "num_queues", CTLTYPE_INT |
CTLFLAG_RD,
- adapter, 0, igb_per_unit_num_queues, "I",
"Number of Queues");
-
igb_set_sysctl_value(adapter, "enable_aim",
"Interrupt Moderation", &adapter->enable_aim,
igb_enable_aim);
@@ -2837,7 +2831,6 @@ igb_setup_msix(struct adapter *adapter)
{
device_t dev = adapter->dev;
int bar, want, queues, msgs, maxqueues;
- int n_queues;
/* tuneable override */
if (igb_enable_msix == 0)
@@ -2865,18 +2858,11 @@ igb_setup_msix(struct adapter *adapter)
goto msi;
}
- n_queues = 0;
- /* try more specific tunable, then global, then finally
default to boot time tunable if set. */
- if (device_getenv_int(dev, "num_queues", &n_queues) != 0) {
- device_printf(dev, "using specific tunable
num_queues=%d", n_queues);
- } else if (TUNABLE_INT_FETCH("hw.igb.num_queues",
&n_queues) != 0) {
- if (igb_num_queues != n_queues) {
- device_printf(dev, "using global tunable
hw.igb.num_queues=%d", n_queues);
- igb_num_queues = n_queues;
- }
- } else {
- n_queues = igb_num_queues;
- }
+ queues = (mp_ncpus > (msgs-1)) ? (msgs-1) : mp_ncpus;
+
+ /* Override via tuneable */
+ if (igb_num_queues != 0)
+ queues = igb_num_queues;
#ifdef RSS
/* If we're doing RSS, clamp at the number of RSS buckets */
@@ -2884,12 +2870,6 @@ igb_setup_msix(struct adapter *adapter)
queues = rss_getnumbuckets();
#endif
- if (n_queues != 0) {
- queues = n_queues;
- } else {
- /* Figure out a reasonable auto config value */
- queues = (mp_ncpus > (msgs-1)) ? (msgs-1) : mp_ncpus;
- }
/* Sanity check based on HW */
switch (adapter->hw.mac.type) {
@@ -2912,17 +2892,10 @@ igb_setup_msix(struct adapter *adapter)
maxqueues = 1;
break;
}
- if (queues > maxqueues) {
- device_printf(adapter->dev, "requested %d queues,
but max for this adapter is %d\n",
- queues, maxqueues);
+
+ /* Final clamp on the actual hardware capability */
+ if (queues > maxqueues)
queues = maxqueues;
- } else if (queues == 0) {
- queues = 1;
- } else if (queues < 0) {
- device_printf(adapter->dev, "requested %d queues,
but min for this adapter is %d\n",
- queues, 1);
- queues = 1;
- }
/*
** One vector (RX/TX pair) per queue
@@ -6407,14 +6380,3 @@ igb_sysctl_eee(SYSCTL_HANDLER_ARGS)
IGB_CORE_UNLOCK(adapter);
return (0);
}
-
-static int
-igb_per_unit_num_queues(SYSCTL_HANDLER_ARGS)
-{
- struct adapter *adapter;
-
- adapter = (struct adapter *) arg1;
-
- return sysctl_handle_int(oidp, &adapter->num_queues, 0, req);
-}
-
Modified: head/sys/dev/ixgbe/ixgbe.c
==============================================================================
--- head/sys/dev/ixgbe/ixgbe.c Tue Dec 2 22:35:43 2014 (r275430)
+++ head/sys/dev/ixgbe/ixgbe.c Tue Dec 2 23:02:57 2014 (r275431)
@@ -103,7 +103,6 @@ static char *ixgbe_strings[] = {
/*********************************************************************
* Function prototypes
*********************************************************************/
-static int ixgbe_per_unit_num_queues(SYSCTL_HANDLER_ARGS);
static int ixgbe_probe(device_t);
static int ixgbe_attach(device_t);
static int ixgbe_detach(device_t);
@@ -476,11 +475,6 @@ ixgbe_attach(device_t dev)
SYSCTL_ADD_PROC(device_get_sysctl_ctx(dev),
SYSCTL_CHILDREN(device_get_sysctl_tree(dev)),
- OID_AUTO, "num_queues", CTLTYPE_INT |
CTLFLAG_RD,
- adapter, 0, ixgbe_per_unit_num_queues,
"I", "Number of Queues");
-
- SYSCTL_ADD_PROC(device_get_sysctl_ctx(dev),
- SYSCTL_CHILDREN(device_get_sysctl_tree(dev)),
OID_AUTO, "ts", CTLTYPE_INT | CTLFLAG_RW,
adapter,
0, ixgbe_set_thermal_test, "I", "Thermal
Test");
@@ -2522,7 +2516,6 @@ ixgbe_setup_msix(struct adapter *adapter
{
device_t dev = adapter->dev;
int rid, want, queues, msgs;
- int n_queues;
/* Override by tuneable */
if (ixgbe_enable_msix == 0)
@@ -2549,34 +2542,19 @@ ixgbe_setup_msix(struct adapter *adapter
/* Figure out a reasonable auto config value */
queues = (mp_ncpus > (msgs-1)) ? (msgs-1) : mp_ncpus;
+
+ /* Override based on tuneable */
+ if (ixgbe_num_queues != 0)
+ queues = ixgbe_num_queues;
+
#ifdef RSS
/* If we're doing RSS, clamp at the number of RSS buckets */
if (queues > rss_getnumbuckets())
queues = rss_getnumbuckets();
#endif
- /* try more specific tunable, then global, then finally
default to boot time tunable if set. */
- if (device_getenv_int(dev, "num_queues", &n_queues) != 0) {
- device_printf(dev, "using specific tunable
numqueues=%d", n_queues);
- } else if (TUNABLE_INT_FETCH("hw.ix.num_queues",
&n_queues) != 0) {
- if (ixgbe_num_queues != n_queues) {
- device_printf(dev, "using global tunable
num_queues=%d", n_queues);
- ixgbe_num_queues = n_queues;
- }
- } else {
- n_queues = ixgbe_num_queues;
- }
-
- if (n_queues < 0) {
- device_printf(dev, "tunable < 0, resetting to
default");
- n_queues = 0;
- }
-
- if (n_queues != 0)
- queues = n_queues;
- /* Set max queues to 8 when autoconfiguring */
- else if ((ixgbe_num_queues == 0) && (queues > 8))
- queues = 8;
+ /* reflect correct sysctl value */
+ ixgbe_num_queues = queues;
/*
** Want one vector (RX/TX pair) per queue
@@ -5964,15 +5942,6 @@ ixgbe_set_flowcntl(SYSCTL_HANDLER_ARGS)
return error;
}
-static int
-ixgbe_per_unit_num_queues(SYSCTL_HANDLER_ARGS)
-{
- struct adapter *adapter;
-
- adapter = (struct adapter *) arg1;
-
- return sysctl_handle_int(oidp, &adapter->num_queues, 0, req);
-}
/*
** Control link advertise speed:
--- Begin Message ---
JFYI,
We are discussing some very minor driver changes here:
https://reviews.freebsd.org/D1149
The gist of this change is just to allow later tuning of the num_queues
parameter for Intel NICS.
Can you please sign up for a phabricator account?
https://wiki.freebsd.org/CodeReview
-Alfred
-------- Original Message --------
Subject: [Differential] [Commented On] D1149: ixgbe and igb should
check tunables at load time. Also support per-device queue count.
Date: Sat, 15 Nov 2014 20:13:51 +0000
From: alfredperlstein (Alfred Perlstein) <phabric-nore...@freebsd.org>
To: alf...@freebsd.org
alfredperlstein added a comment.
I don't know how to do that. Please make 'jfv' an account.
REVISION DETAIL
https://reviews.freebsd.org/D1149
To: alfredperlstein, glebius, adrian, melifaro, hrs, wollman, bryanv, rpaulo,
bz, gnn, hiren, rwatson, smh
Cc: ngie, bryanv
--- End Message ---
