Message-ID: <1454627396.3086.14.camel@intel.com>
Date:	Thu, 4 Feb 2016 23:09:56 +0000
From:	"Keller, Jacob E" <jacob.e.keller@...el.com>
To:	"davem@...emloft.net" <davem@...emloft.net>
CC:	"andrew@...n.ch" <andrew@...n.ch>,
	"Nelson, Shannon" <shannon.nelson@...el.com>,
	"aduyck@...antis.com" <aduyck@...antis.com>,
	"thomas.petazzoni@...e-electrons.com" 
	<thomas.petazzoni@...e-electrons.com>,
	"mcarlson@...adcom.com" <mcarlson@...adcom.com>,
	"yuvalmin@...adcom.com" <yuvalmin@...adcom.com>,
	"amirv@...lanox.com" <amirv@...lanox.com>,
	"Williams, Mitch A" <mitch.a.williams@...el.com>,
	"_govind@....com" <_govind@....com>,
	"Lendacky@...r.kernel.org" <Lendacky@...r.kernel.org>,
	"Tantilov, Emil S" <emil.s.tantilov@...el.com>,
	"bhutchings@...arflare.com" <bhutchings@...arflare.com>,
	"mirq-linux@...e.qmqm.pl" <mirq-linux@...e.qmqm.pl>,
	"hariprasad@...lsio.com" <hariprasad@...lsio.com>,
	"kalesh.purayil@...lex.com" <kalesh.purayil@...lex.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"mchan@...adcom.com" <mchan@...adcom.com>,
	"Wyborny, Carolyn" <carolyn.wyborny@...el.com>,
	"sgoutham@...ium.com" <sgoutham@...ium.com>,
	"Thomas.Lendacky@....com" <Thomas.Lendacky@....com>,
	"achiad@...lanox.com" <achiad@...lanox.com>
Subject: Re: [PATCH 1/2] ethtool: add dynamic flag to ETHTOOL_{GS}RXFH
 commands

On Thu, 2016-02-04 at 17:53 -0500, David Miller wrote:
> From: Jacob Keller <jacob.e.keller@...el.com>
> Date: Tue,  2 Feb 2016 15:22:06 -0800
> 
> > Ethtool supports a few operations for modifying and controlling
> > a device's RSS table. Sometimes, changes in other features of the
> > device
> > may require (or desire) changes to the RSS table. Currently there
> > is no
> > method to indicate to the driver whether the current RSS table
> > settings
> > should be maintained or overridden.
> 
> Yes, there certainly is a way to indicate this.
> 
> If the user asks for the change in the number of queues, and you
> cannot retain the user's requested RSS settings, then you must fail
> the queue setting change.
> 
> And vice versa.
> 
> You can't say to the user "I can adhere to your requested
> configuration
> change, but I might undo it for some unspecified reason"
> 

The trouble here is the case where the indirection table configurations
are valid now, but a change in the number of queues happening at a
later time currently causes these settings to be lost.

> That's unacceptable behavior, and that's exactly what this dynamic
> flag means.
> 
> If you cannot give the user what he asks for, precisely and reliably,
> you fail the operation with an error.
> 
> There is no way I am adding code which allows these "maybe" kind of
> configuration operations.  Either you can or you can't, and you tell
> the user when you can't by erroring out on the operation that
> invalidates the requirements.
> 
> 

So you're suggesting instead, to error when the second operation
(change number of queues) would fail the current settings?

Current driver behaviors for all the drivers I checked work in one of
two ways.

1) changing queues will destroy the RSS table as it will be
reinitialized regardless of current settings

2) changing queues will maintain the RSS table if possible, unless the
previous RSS table can't function.

No driver currently fails this operation if the RSS table settings
can't be preserved. In addition it results in weird behavior when a
driver sets the RSS table at load, then increases the number of queues
via an ethtool op, the result is that RSS does not use the new queues
added by the ethtool operation.

I can instead drop the ethtool changes and just have my driver record
when the user has changed the tables, and attempt to error on queue
setting operation, which may work.

Essentially the idea was to have a flag indicating "use the driver
defaults" which the driver can change as necessary when the number of
queues changes, or other factors that may require RSS table changes.

I can do this all hidden in the driver but then there is nothing
exposing how the driver will behave under this circumstance.

I'm all for a better suggestion, because I think what we're doing now
is wrong, and the proposed solutions so far don't seem right either.

If we preserve the RSS table when queues increase, then the user may be
confused because RSS settings won't spread to the new queues. If we
destroy the RSS settings the user will be possibly confused because
their selected RSS settings do not work. If we fail the setting of
queues when RSS table is not the default value, then a user might be
confused as to why they can't change the number of queues. Also, I am
unsure whether or not we can tell from the ethtool op function that we
actually are being "reset" to the default, vs just being set. This is
because the netlink message of "0 length" which indicates default
simply has the ethtool core fill in a standard equal weight default,
which I am not sure our driver can tell that it should now be ok to
enable queue changes.

So, something is missing in the current flow to allow this. I think the
best solution is simply prevent changing number of queues while we have
a non-default RSS setting, and require RSS to be reset before queues
can be changed.

Thoughts?

Regards,
Jake

Powered by blists - more mailing lists