Date: Tue, 02 Dec 2014 19:17:04 -0800 From: Alfred Perlstein <alfred@freebsd.org> To: Jack Vogel <jfvogel@gmail.com>, Jack F Vogel <jfv@freebsd.org> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org> Subject: Re: svn commit: r275431 - in head/sys/dev: e1000 ixgbe Message-ID: <547E80B0.8020407@freebsd.org> In-Reply-To: <CAFOYbcktpW3OGUx3ex6_HvKUmPhn=L6VUrVRiOHDiWmzrYoCHg@mail.gmail.com> References: <201412022302.sB2N2w9V002617@svn.freebsd.org> <CAFOYbcktpW3OGUx3ex6_HvKUmPhn=L6VUrVRiOHDiWmzrYoCHg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --------------070409090102030909030801 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Thanks Jack. I do appreciate you reviewing the change and reconsidering the implementation. A few things to note please: 1) The work was submitted for approval, please check your inbox from ~3 weeks ago. I've attached the message for reference. 2) Regarding #2, in FreeBSD past silence, especially for a minor change with good intentions, was considered acceptance. I may have missed an entry somewhere saying where the timeout for acceptance was, and if so, my apologies, however that doesn't seem to be the case. I will in the future give much more space than ~2 weeks for issues in the driver here. 3) In future would appreciate not calling my work sloppy in the commit logs. Commit logs serve as a very public record. Thanks again for considering implementing this another way. It would do a service to us if we could sort out how to tune the drivers only when they become active as it allows us to tune them later by using arbitrary means. I hope we can get something in that works for both of us. -Alfred On 12/2/14 4:10 PM, Jack Vogel wrote: > 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 <jfv@freebsd.org > <mailto:jfv@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: > > --------------070409090102030909030801 Content-Type: message/rfc822; name="Attached Message" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="Attached Message" Message-ID: <5467B4AA.7060009@freebsd.org> Date: Sat, 15 Nov 2014 12:16:42 -0800 From: Alfred Perlstein <alfred@freebsd.org> Organization: FreeBSD User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: jfv <jfv@FreeBSD.org> Subject: Fwd: [Differential] [Commented On] D1149: ixgbe and igb should check tunables at load time. Also support per-device queue count. References: <f4a3b7b1b89caa0bc043b29e3f62397a@localhost.localdomain> In-Reply-To: <f4a3b7b1b89caa0bc043b29e3f62397a@localhost.localdomain> X-Forwarded-Message-Id: <f4a3b7b1b89caa0bc043b29e3f62397a@localhost.localdomain> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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-noreply@FreeBSD.org> To: alfred@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 --------------070409090102030909030801--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?547E80B0.8020407>