* [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
@ 2014-03-17 12:31 Venkat Duvvuru
2014-03-19 19:59 ` David Miller
0 siblings, 1 reply; 15+ messages in thread
From: Venkat Duvvuru @ 2014-03-17 12:31 UTC (permalink / raw)
To: netdev; +Cc: Venkat Duvvuru
NIC drivers that support RSS use either a hard-coded value or a random value for
the RSS hash key. Irrespective of the type of the key used, the user would want
to change the hash key if he/she is not satisfied with the effectiveness of the
default hash-key in spreading the incoming flows evenly across the RSS queues.
This patch set adds support for configuring the RSS hash-key via the ethtool
interface using -X option.
v5:
1. Removed "inline" keyword for ethtool_copy_validate_indir function.
2. Added description about what "indir_size == 0xDEADBEEF" means.
Venkat Duvvuru (2):
ethtool: Support for configurable RSS hash key.
be2net: Support for configurable RSS hash key.
drivers/net/ethernet/emulex/benet/be.h | 12 ++-
drivers/net/ethernet/emulex/benet/be_cmds.c | 7 +-
drivers/net/ethernet/emulex/benet/be_cmds.h | 2 +-
drivers/net/ethernet/emulex/benet/be_ethtool.c | 103 ++++++++++--
drivers/net/ethernet/emulex/benet/be_main.c | 31 ++--
include/linux/ethtool.h | 13 ++
include/uapi/linux/ethtool.h | 29 +++
net/core/ethtool.c | 219 ++++++++++++++++++++++--
8 files changed, 371 insertions(+), 45 deletions(-)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-03-17 12:31 [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key Venkat Duvvuru
@ 2014-03-19 19:59 ` David Miller
2014-03-20 12:02 ` Venkata Duvvuru
2014-03-21 14:33 ` Ben Hutchings
0 siblings, 2 replies; 15+ messages in thread
From: David Miller @ 2014-03-19 19:59 UTC (permalink / raw)
To: VenkatKumar.Duvvuru; +Cc: netdev
From: Venkat Duvvuru <VenkatKumar.Duvvuru@Emulex.com>
Date: Mon, 17 Mar 2014 18:01:33 +0530
> NIC drivers that support RSS use either a hard-coded value or a random value for
> the RSS hash key. Irrespective of the type of the key used, the user would want
> to change the hash key if he/she is not satisfied with the effectiveness of the
> default hash-key in spreading the incoming flows evenly across the RSS queues.
>
> This patch set adds support for configuring the RSS hash-key via the ethtool
> interface using -X option.
I apologize, but I really dislike this. For several reasons.
First, why aren't we adding _just_ a RSS hash changing interface?
We already have an interface for changing the indirection table,
there is absolutely not need to add a second interface that supports
both indirection table _plus_ hash modifications.
And combining these two is what leads to this hard to audit, ugly,
data structure layout.
There's the indirection table at some offset, then the key at some
other offset. This makes it impossible to impose type checking
of any kind on both objects.
And the data structure is furthermore named in a way that suggests
it's just for doing things with the RSS hash.
Just add:
struct ethtool_rxfh {
__u32 cmd;
__u32 key_size;
__u32 key[0];
};
And that's it.
To change the indirection table, call "ETHTOOL_SRXFH". To set the
RSS flow hash, call "ETHTOOL_SRSSH".
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-03-19 19:59 ` David Miller
@ 2014-03-20 12:02 ` Venkata Duvvuru
2014-03-20 19:43 ` David Miller
2014-03-21 14:33 ` Ben Hutchings
1 sibling, 1 reply; 15+ messages in thread
From: Venkata Duvvuru @ 2014-03-20 12:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev@vger.kernel.org, Ben Hutchings (ben@decadent.org.uk)
> > NIC drivers that support RSS use either a hard-coded value or a random
> > value for the RSS hash key. Irrespective of the type of the key used,
> > the user would want to change the hash key if he/she is not satisfied
> > with the effectiveness of the default hash-key in spreading the incoming flows
> evenly across the RSS queues.
> >
> > This patch set adds support for configuring the RSS hash-key via the
> > ethtool interface using -X option.
>
> I apologize, but I really dislike this. For several reasons.
>
> First, why aren't we adding _just_ a RSS hash changing interface?
>
> We already have an interface for changing the indirection table, there is
> absolutely not need to add a second interface that supports both indirection
> table _plus_ hash modifications.
>
> And combining these two is what leads to this hard to audit, ugly, data structure
> layout.
>
> There's the indirection table at some offset, then the key at some other offset.
> This makes it impossible to impose type checking of any kind on both objects.
>
> And the data structure is furthermore named in a way that suggests it's just for
> doing things with the RSS hash.
>
> Just add:
>
> struct ethtool_rxfh {
> __u32 cmd;
> __u32 key_size;
> __u32 key[0];
> };
>
> And that's it.
>
> To change the indirection table, call "ETHTOOL_SRXFH". To set the RSS flow
> hash, call "ETHTOOL_SRSSH".
My original patch was exactly the way you suggested.
It went through many changes later with the intention of combining both indirection and hash key setting into one option as both are related to rss configuration.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-03-20 12:02 ` Venkata Duvvuru
@ 2014-03-20 19:43 ` David Miller
0 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2014-03-20 19:43 UTC (permalink / raw)
To: VenkatKumar.Duvvuru; +Cc: netdev, ben
From: Venkata Duvvuru <VenkatKumar.Duvvuru@Emulex.Com>
Date: Thu, 20 Mar 2014 12:02:11 +0000
> It went through many changes later with the intention of combining
> both indirection and hash key setting into one option as both are
> related to rss configuration.
Combining these two things makes the buffer management at the end of
the defined data type more complex.
Furthermore, we generally steer far away from having multiple ways
to do the same exact thing.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-03-19 19:59 ` David Miller
2014-03-20 12:02 ` Venkata Duvvuru
@ 2014-03-21 14:33 ` Ben Hutchings
2014-03-21 19:28 ` David Miller
1 sibling, 1 reply; 15+ messages in thread
From: Ben Hutchings @ 2014-03-21 14:33 UTC (permalink / raw)
To: David Miller; +Cc: VenkatKumar.Duvvuru, netdev
[-- Attachment #1: Type: text/plain, Size: 1680 bytes --]
On Wed, 2014-03-19 at 15:59 -0400, David Miller wrote:
> From: Venkat Duvvuru <VenkatKumar.Duvvuru@Emulex.com>
> Date: Mon, 17 Mar 2014 18:01:33 +0530
>
> > NIC drivers that support RSS use either a hard-coded value or a random value for
> > the RSS hash key. Irrespective of the type of the key used, the user would want
> > to change the hash key if he/she is not satisfied with the effectiveness of the
> > default hash-key in spreading the incoming flows evenly across the RSS queues.
> >
> > This patch set adds support for configuring the RSS hash-key via the ethtool
> > interface using -X option.
>
> I apologize, but I really dislike this. For several reasons.
>
> First, why aren't we adding _just_ a RSS hash changing interface?
>
> We already have an interface for changing the indirection table,
> there is absolutely not need to add a second interface that supports
> both indirection table _plus_ hash modifications.
That's what I asked for, because I see the hash key and indirection
table as being a single logical object (RSS context).
> And combining these two is what leads to this hard to audit, ugly,
> data structure layout.
>
> There's the indirection table at some offset, then the key at some
> other offset. This makes it impossible to impose type checking
> of any kind on both objects.
[...]
Which is why I previously suggested making the ethtool core find the two
arrays and pass those into the driver operations. We do that for other
ethtool operations that use even a single variable-length array.
Ben.
--
Ben Hutchings
One of the nice things about standards is that there are so many of them.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-03-21 14:33 ` Ben Hutchings
@ 2014-03-21 19:28 ` David Miller
2014-03-26 9:21 ` Venkata Duvvuru
0 siblings, 1 reply; 15+ messages in thread
From: David Miller @ 2014-03-21 19:28 UTC (permalink / raw)
To: ben; +Cc: VenkatKumar.Duvvuru, netdev
From: Ben Hutchings <ben@decadent.org.uk>
Date: Fri, 21 Mar 2014 14:33:13 +0000
> On Wed, 2014-03-19 at 15:59 -0400, David Miller wrote:
>> From: Venkat Duvvuru <VenkatKumar.Duvvuru@Emulex.com>
>> Date: Mon, 17 Mar 2014 18:01:33 +0530
>>
>> > NIC drivers that support RSS use either a hard-coded value or a random value for
>> > the RSS hash key. Irrespective of the type of the key used, the user would want
>> > to change the hash key if he/she is not satisfied with the effectiveness of the
>> > default hash-key in spreading the incoming flows evenly across the RSS queues.
>> >
>> > This patch set adds support for configuring the RSS hash-key via the ethtool
>> > interface using -X option.
>>
>> I apologize, but I really dislike this. For several reasons.
>>
>> First, why aren't we adding _just_ a RSS hash changing interface?
>>
>> We already have an interface for changing the indirection table,
>> there is absolutely not need to add a second interface that supports
>> both indirection table _plus_ hash modifications.
>
> That's what I asked for, because I see the hash key and indirection
> table as being a single logical object (RSS context).
>
>> And combining these two is what leads to this hard to audit, ugly,
>> data structure layout.
>>
>> There's the indirection table at some offset, then the key at some
>> other offset. This makes it impossible to impose type checking
>> of any kind on both objects.
> [...]
>
> Which is why I previously suggested making the ethtool core find the two
> arrays and pass those into the driver operations. We do that for other
> ethtool operations that use even a single variable-length array.
Sure, we could hide the dirt in the generic ethtool code and pass
separated out pointers to the drivers.
But you have to recognize that the dirt will still exist in userspace
when it calls this stuff.
Nothing stops ethtool from providing a combined operation on the
command line, but that doesn't mean we necessarily have to have
a single kernel interface doing both thing.
I really still think that adding just a new key operation is the way
to go.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-03-21 19:28 ` David Miller
@ 2014-03-26 9:21 ` Venkata Duvvuru
2014-03-27 18:53 ` David Miller
0 siblings, 1 reply; 15+ messages in thread
From: Venkata Duvvuru @ 2014-03-26 9:21 UTC (permalink / raw)
To: David Miller, ben@decadent.org.uk; +Cc: netdev@vger.kernel.org
> Nothing stops ethtool from providing a combined operation on the command
> line, but that doesn't mean we necessarily have to have a single kernel
> interface doing both thing.
Sounds good. We can have a combined operation on the command line and can still have different data structure, command and kernel interface for each of the sub operations.
> I really still think that adding just a new key operation is the way to go.
Ben and David,
I don't mind going back to the original approach of adding a new hash key operation. Please let me know what you guys think, combined operation or a new operation.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-03-26 9:21 ` Venkata Duvvuru
@ 2014-03-27 18:53 ` David Miller
2014-03-28 1:39 ` Ben Hutchings
0 siblings, 1 reply; 15+ messages in thread
From: David Miller @ 2014-03-27 18:53 UTC (permalink / raw)
To: VenkatKumar.Duvvuru; +Cc: ben, netdev
From: Venkata Duvvuru <VenkatKumar.Duvvuru@Emulex.Com>
Date: Wed, 26 Mar 2014 09:21:01 +0000
> I don't mind going back to the original approach of adding a new
> hash key operation. Please let me know what you guys think, combined
> operation or a new operation.
Unless Ben has a major objection, I would prefer that we just add a
new hash key interface.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-03-27 18:53 ` David Miller
@ 2014-03-28 1:39 ` Ben Hutchings
2014-03-28 21:12 ` David Miller
0 siblings, 1 reply; 15+ messages in thread
From: Ben Hutchings @ 2014-03-28 1:39 UTC (permalink / raw)
To: David Miller; +Cc: VenkatKumar.Duvvuru, netdev
[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]
On Thu, 2014-03-27 at 14:53 -0400, David Miller wrote:
> From: Venkata Duvvuru <VenkatKumar.Duvvuru@Emulex.Com>
> Date: Wed, 26 Mar 2014 09:21:01 +0000
>
> > I don't mind going back to the original approach of adding a new
> > hash key operation. Please let me know what you guys think, combined
> > operation or a new operation.
>
> Unless Ben has a major objection, I would prefer that we just add a
> new hash key interface.
My objection is that there is also the prospect of having multiple RSS
contexts on a single device. I think we discussed the usefulness of
that several years back at a netconf. And there is now hardware that
supports it: at least the Solarflare SFC9100 controllers do, and I doubt
they are unique in this.
By grouping the key and hash together, with some reserved fields (which
the setter should initially require to be equal to zero), it should be
possible to fill in support for numbered RSS contexts later without yet
another new structure and operations.
Ben.
--
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-03-28 1:39 ` Ben Hutchings
@ 2014-03-28 21:12 ` David Miller
2014-04-01 15:19 ` Venkata Duvvuru
0 siblings, 1 reply; 15+ messages in thread
From: David Miller @ 2014-03-28 21:12 UTC (permalink / raw)
To: ben; +Cc: VenkatKumar.Duvvuru, netdev
From: Ben Hutchings <ben@decadent.org.uk>
Date: Fri, 28 Mar 2014 01:39:30 +0000
> By grouping the key and hash together, with some reserved fields (which
> the setter should initially require to be equal to zero), it should be
> possible to fill in support for numbered RSS contexts later without yet
> another new structure and operations.
If that is the case, why don't we add from the start a "u32 rss_context;"
field that has to be set to zero for now?
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-03-28 21:12 ` David Miller
@ 2014-04-01 15:19 ` Venkata Duvvuru
2014-04-01 16:20 ` David Miller
0 siblings, 1 reply; 15+ messages in thread
From: Venkata Duvvuru @ 2014-04-01 15:19 UTC (permalink / raw)
To: David Miller, ben@decadent.org.uk; +Cc: netdev@vger.kernel.org
> > By grouping the key and hash together, with some reserved fields
> > (which the setter should initially require to be equal to zero), it
> > should be possible to fill in support for numbered RSS contexts later
> > without yet another new structure and operations.
> If that is the case, why don't we add from the start a "u32 rss_context;"
> field that has to be set to zero for now?
The current structure has got three u32 reserved fields already. We could use one of them when the support is added.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-04-01 15:19 ` Venkata Duvvuru
@ 2014-04-01 16:20 ` David Miller
2014-04-01 16:55 ` Ben Hutchings
2014-04-02 11:14 ` Venkata Duvvuru
0 siblings, 2 replies; 15+ messages in thread
From: David Miller @ 2014-04-01 16:20 UTC (permalink / raw)
To: VenkatKumar.Duvvuru; +Cc: ben, netdev
From: Venkata Duvvuru <VenkatKumar.Duvvuru@Emulex.Com>
Date: Tue, 1 Apr 2014 15:19:48 +0000
>> > By grouping the key and hash together, with some reserved fields
>> > (which the setter should initially require to be equal to zero), it
>> > should be possible to fill in support for numbered RSS contexts later
>> > without yet another new structure and operations.
>
>
>> If that is the case, why don't we add from the start a "u32 rss_context;"
>> field that has to be set to zero for now?
>
> The current structure has got three u32 reserved fields already. We could use one of them when the support is added.
Just do as I said, naming one of those to "rss_context" now, and require
that it be zero.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-04-01 16:20 ` David Miller
@ 2014-04-01 16:55 ` Ben Hutchings
2014-04-02 11:14 ` Venkata Duvvuru
1 sibling, 0 replies; 15+ messages in thread
From: Ben Hutchings @ 2014-04-01 16:55 UTC (permalink / raw)
To: David Miller, VenkatKumar.Duvvuru; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]
On Tue, 2014-04-01 at 12:20 -0400, David Miller wrote:
> From: Venkata Duvvuru <VenkatKumar.Duvvuru@Emulex.Com>
> Date: Tue, 1 Apr 2014 15:19:48 +0000
>
> >> > By grouping the key and hash together, with some reserved fields
> >> > (which the setter should initially require to be equal to zero), it
> >> > should be possible to fill in support for numbered RSS contexts later
> >> > without yet another new structure and operations.
> >
> >
> >> If that is the case, why don't we add from the start a "u32 rss_context;"
> >> field that has to be set to zero for now?
> >
> > The current structure has got three u32 reserved fields already. We could use one of them when the support is added.
>
> Just do as I said, naming one of those to "rss_context" now, and require
> that it be zero.
I agree, I think that makes sense for the initial definition.
Sorry about all the back-and-forth discussion on this, Venkata.
Ben.
--
Ben Hutchings
Computers are not intelligent. They only think they are.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-04-01 16:20 ` David Miller
2014-04-01 16:55 ` Ben Hutchings
@ 2014-04-02 11:14 ` Venkata Duvvuru
2014-04-03 18:37 ` David Miller
1 sibling, 1 reply; 15+ messages in thread
From: Venkata Duvvuru @ 2014-04-02 11:14 UTC (permalink / raw)
To: David Miller; +Cc: ben@decadent.org.uk, netdev@vger.kernel.org
> >> > By grouping the key and hash together, with some reserved fields
> >> > (which the setter should initially require to be equal to zero), it
> >> > should be possible to fill in support for numbered RSS contexts
> >> > later without yet another new structure and operations.
> >
> >
> >> If that is the case, why don't we add from the start a "u32 rss_context;"
> >> field that has to be set to zero for now?
> >
> > The current structure has got three u32 reserved fields already. We could use
> one of them when the support is added.
>
> Just do as I said, naming one of those to "rss_context" now, and require that it
> be zero.
David, I see that net-next will not be opened for new feature patches for some time. Please let me know if I can submit "v6" version of this patch now or wait till net-next opens back up. Thanks.
Venkat.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key
2014-04-02 11:14 ` Venkata Duvvuru
@ 2014-04-03 18:37 ` David Miller
0 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2014-04-03 18:37 UTC (permalink / raw)
To: VenkatKumar.Duvvuru; +Cc: ben, netdev
From: Venkata Duvvuru <VenkatKumar.Duvvuru@Emulex.Com>
Date: Wed, 2 Apr 2014 11:14:20 +0000
> David, I see that net-next will not be opened for new feature
> patches for some time. Please let me know if I can submit "v6"
> version of this patch now or wait till net-next opens back
> up. Thanks.
Unfortunately, I think we need to defer.
Thank you.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-04-03 18:36 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-17 12:31 [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key Venkat Duvvuru
2014-03-19 19:59 ` David Miller
2014-03-20 12:02 ` Venkata Duvvuru
2014-03-20 19:43 ` David Miller
2014-03-21 14:33 ` Ben Hutchings
2014-03-21 19:28 ` David Miller
2014-03-26 9:21 ` Venkata Duvvuru
2014-03-27 18:53 ` David Miller
2014-03-28 1:39 ` Ben Hutchings
2014-03-28 21:12 ` David Miller
2014-04-01 15:19 ` Venkata Duvvuru
2014-04-01 16:20 ` David Miller
2014-04-01 16:55 ` Ben Hutchings
2014-04-02 11:14 ` Venkata Duvvuru
2014-04-03 18:37 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).