svn commit: r275431 - in head/sys/dev: e1000 ixgbe
Alfred Perlstein
alfred at freebsd.org
Wed Dec 3 03:17:07 UTC 2014
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 at freebsd.org
> <mailto:jfv at 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:
>
>
-------------- next part --------------
An embedded message was scrubbed...
From: Alfred Perlstein <alfred at freebsd.org>
Subject: Fwd: [Differential] [Commented On] D1149: ixgbe and igb should check tunables at load time. Also support per-device queue count.
Date: Sat, 15 Nov 2014 12:16:42 -0800
Size: 1658
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20141202/c65dd493/attachment.mht>
More information about the svn-src-all
mailing list