RSS configuration in iwlwifi

RSS configuration in iwlwifi

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]

I'm not sure if you were aware, but there is a standard API for
configuring RSS in network drivers, part of ethtool_ops.  I think
iwlwifi should implement that rather than a driver-specific debugfs
interface.

Ben.

-- 
Ben Hutchings
Never attribute to conspiracy what can adequately be explained by stupidity.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: RSS configuration in iwlwifi

[Grumbach, Emmanuel]

SGkgQmVuLA0KDQoNClRoYW5rcyBmb3IgbG9va2luZyBhdCBvdXIgY29kZS4NCg0KDQpPbiBXZWQs
IDIwMTYtMDQtMjAgYXQgMTY6MDggKzAxMDAsIEJlbiBIdXRjaGluZ3Mgd3JvdGU6DQo+IEknbSBu
b3Qgc3VyZSBpZiB5b3Ugd2VyZSBhd2FyZSwgYnV0IHRoZXJlIGlzIGEgc3RhbmRhcmQgQVBJIGZv
cg0KPiBjb25maWd1cmluZyBSU1MgaW4gbmV0d29yayBkcml2ZXJzLCBwYXJ0IG9mIGV0aHRvb2xf
b3BzLiAgSSB0aGluaw0KPiBpd2x3aWZpIHNob3VsZCBpbXBsZW1lbnQgdGhhdCByYXRoZXIgdGhh
biBhIGRyaXZlci1zcGVjaWZpYyBkZWJ1Z2ZzDQo+IGludGVyZmFjZS4NCj4gDQoNCllvdSBhcmUg
cmlnaHQsIHRoaXMgaXMgd2h5IFNhcmEgbWFkZSB0aGlzIGNvbW1pdDoNCg0KY29tbWl0IDg1NGQ3
NzNlNGFiNTg2OTIwMDAwNGFmNGNhNWQ4NTE3MzA4NDk5MDMNCkF1dGhvcjogU2FyYSBTaGFyb24g
PHNhcmEuc2hhcm9uQGludGVsLmNvbT4NCkRhdGU6ICAgVHVlIE1hciAyMiAxNTo1NTo1OCAyMDE2
ICswMjAwDQoNCiAgICBpd2x3aWZpOiBtdm06IGltcHJvdmUgUlNTIGNvbmZpZ3VyYXRpb24NCiAg
ICANCiAgICBJbXByb3ZlIGN1cnJlbnQgUlNTIGNvbmZpZ3VyYXRpb246DQogICAgICogVXNlIG5l
dGRldl9yc3Nfa2V5IGluc3RlYWQgb2Yga2VlcGluZyBhIGxvY2FsIGNvcHkuDQogICAgICogQ29u
ZmlndXJlIGFsc28gVURQIGhhc2hpbmcgdG8gaGF2ZSBVRFAgdHJhZmZpYyBzcHJlYWQgYWNyb3Nz
IHF1ZXVlcy4NCiAgICAgKiBEbyBub3QgZGlyZWN0IFJTUyB0cmFmZmljIHRvIG91ciBmYWxsYmFj
ayBxdWV1ZS4NCiAgICANCiAgICBTaWduZWQtb2ZmLWJ5OiBTYXJhIFNoYXJvbiA8c2FyYS5zaGFy
b25AaW50ZWwuY29tPg0KICAgIFNpZ25lZC1vZmYtYnk6IEVtbWFudWVsIEdydW1iYWNoIDxlbW1h
bnVlbC5ncnVtYmFjaEBpbnRlbC5jb20+DQoNCg0KPiBCZW4u

Re: RSS configuration in iwlwifi

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]

On Wed, 2016-04-20 at 15:30 +0000, Grumbach, Emmanuel wrote:
> Hi Ben,
> 
> 
> Thanks for looking at our code.
> 
> 
> On Wed, 2016-04-20 at 16:08 +0100, Ben Hutchings wrote:
> > 
> > I'm not sure if you were aware, but there is a standard API for
> > configuring RSS in network drivers, part of ethtool_ops.  I think
> > iwlwifi should implement that rather than a driver-specific debugfs
> > interface.
> > 
> You are right, this is why Sara made this commit:
> 
> commit 854d773e4ab5869200004af4ca5d851730849903
> Author: Sara Sharon <sara.sharon@intel.com>
> Date:   Tue Mar 22 15:55:58 2016 +0200
> 
>     iwlwifi: mvm: improve RSS configuration
>     
>     Improve current RSS configuration:
>      * Use netdev_rss_key instead of keeping a local copy.
>      * Configure also UDP hashing to have UDP traffic spread across queues.
>      * Do not direct RSS traffic to our fallback queue.
>     
>     Signed-off-by: Sara Sharon <sara.sharon@intel.com>
>     Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

That doesn't really address what I said.  Yes, it's using the common
RSS key, but it's not implementing the ethtool operations to get and
set the indirection table and the types of flow hashing that are
enabled.

Ben.

-- 
Ben Hutchings
Never attribute to conspiracy what can adequately be explained by stupidity.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: RSS configuration in iwlwifi

[Grumbach, Emmanuel]

T24gV2VkLCAyMDE2LTA0LTIwIGF0IDE2OjQ3ICswMTAwLCBCZW4gSHV0Y2hpbmdzIHdyb3RlOg0K
PiBPbiBXZWQsIDIwMTYtMDQtMjAgYXQgMTU6MzAgKzAwMDAsIEdydW1iYWNoLCBFbW1hbnVlbCB3
cm90ZToNCj4gPiBIaSBCZW4sDQo+ID4gDQo+ID4gDQo+ID4gVGhhbmtzIGZvciBsb29raW5nIGF0
IG91ciBjb2RlLg0KPiA+IA0KPiA+IA0KPiA+IE9uIFdlZCwgMjAxNi0wNC0yMCBhdCAxNjowOCAr
MDEwMCwgQmVuIEh1dGNoaW5ncyB3cm90ZToNCj4gPiA+IA0KPiA+ID4gSSdtIG5vdCBzdXJlIGlm
IHlvdSB3ZXJlIGF3YXJlLCBidXQgdGhlcmUgaXMgYSBzdGFuZGFyZCBBUEkgZm9yDQo+ID4gPiBj
b25maWd1cmluZyBSU1MgaW4gbmV0d29yayBkcml2ZXJzLCBwYXJ0IG9mIGV0aHRvb2xfb3BzLiAg
SSB0aGluaw0KPiA+ID4gaXdsd2lmaSBzaG91bGQgaW1wbGVtZW50IHRoYXQgcmF0aGVyIHRoYW4g
YSBkcml2ZXItc3BlY2lmaWMNCj4gPiA+IGRlYnVnZnMNCj4gPiA+IGludGVyZmFjZS4NCj4gPiA+
IA0KPiA+IFlvdSBhcmUgcmlnaHQsIHRoaXMgaXMgd2h5IFNhcmEgbWFkZSB0aGlzIGNvbW1pdDoN
Cj4gPiANCj4gPiBjb21taXQgODU0ZDc3M2U0YWI1ODY5MjAwMDA0YWY0Y2E1ZDg1MTczMDg0OTkw
Mw0KPiA+IEF1dGhvcjogU2FyYSBTaGFyb24gPHNhcmEuc2hhcm9uQGludGVsLmNvbT4NCj4gPiBE
YXRlOiAgIFR1ZSBNYXIgMjIgMTU6NTU6NTggMjAxNiArMDIwMA0KPiA+IA0KPiA+ICAgICBpd2x3
aWZpOiBtdm06IGltcHJvdmUgUlNTIGNvbmZpZ3VyYXRpb24NCj4gPiAgICAgDQo+ID4gICAgIElt
cHJvdmUgY3VycmVudCBSU1MgY29uZmlndXJhdGlvbjoNCj4gPiAgICAgICogVXNlIG5ldGRldl9y
c3Nfa2V5IGluc3RlYWQgb2Yga2VlcGluZyBhIGxvY2FsIGNvcHkuDQo+ID4gICAgICAqIENvbmZp
Z3VyZSBhbHNvIFVEUCBoYXNoaW5nIHRvIGhhdmUgVURQIHRyYWZmaWMgc3ByZWFkIGFjcm9zcw0K
PiA+IHF1ZXVlcy4NCj4gPiAgICAgIERvIG5vdCBkaXJlY3QgUlNTIHRyYWZmaWMgdG8gb3VyIGZh
bGxiYWNrIHF1ZXVlLg0KPiA+DQo+ID4gICAgIFNpZ25lZC1vZmYtYnk6IFNhcmEgU2hhcm9uIDxz
YXJhLnNoYXJvbkBpbnRlbC5jb20+DQo+ID4gICAgIFNpZ25lZC1vZmYtYnk6IEVtbWFudWVsIEdy
dW1iYWNoIDxlbW1hbnVlbC5ncnVtYmFjaEBpbnRlbC5jb20+DQo+IA0KPiBUaGF0IGRvZXNuJ3Qg
cmVhbGx5IGFkZHJlc3Mgd2hhdCBJIHNhaWQuICBZZXMsIGl0J3MgdXNpbmcgdGhlIGNvbW1vbg0K
PiBSU1Mga2V5LCBidXQgaXQncyBub3QgaW1wbGVtZW50aW5nIHRoZSBldGh0b29sIG9wZXJhdGlv
bnMgdG8gZ2V0IGFuZA0KPiBzZXQgdGhlIGluZGlyZWN0aW9uIHRhYmxlIGFuZCB0aGUgdHlwZXMg
b2YgZmxvdyBoYXNoaW5nIHRoYXQgYXJlDQo+IGVuYWJsZWQuDQo+IA0KDQpIbS4uIEkgdGhpbmsg
dGhhdCBzZXR0aW5nIHRoZSBpbmRpcmVjdGlvbiB0YWJsZSBpcyBhIHByb2JsZW0gaW4gb3VyDQpj
YXNlIGJlY2F1c2UgdGhlIFBOIGNoZWNrIGlzIGRvbmUgaW4gdGhlIGRyaXZlci4gVGhlIFBOIGNo
ZWNrIGlzIHRoZQ0Kd2F5IFdpRmkgYWRkcmVzc2VzIHRoZSByZXBsYXkgYXR0YWNrLCBhbmQgc2lu
Y2UgdGhlIFBOIGNoZWNrIHJlbGllcyBvbg0KcGVyLWNwdSB2YXJpYWJsZXMsIHdlIGNhbm5vdCAq
c2FmZWx5KiBhbGxvdyB1c2VycyB0byBtb2RpZnkgdGhlDQppbmRpcmVjdGlvbiB0YWJsZSB3aGls
ZSB0cmFmZmljIGlzIGZsb3dpbmcuDQoNCj4gQmVuLg0KPiA=

Re: RSS configuration in iwlwifi

[Grumbach, Emmanuel]

T24gV2VkLCAyMDE2LTA0LTIwIGF0IDE5OjIyICswMzAwLCBFbW1hbnVlbCBHcnVtYmFjaCB3cm90
ZToNCj4gT24gV2VkLCAyMDE2LTA0LTIwIGF0IDE2OjQ3ICswMTAwLCBCZW4gSHV0Y2hpbmdzIHdy
b3RlOg0KPiA+IE9uIFdlZCwgMjAxNi0wNC0yMCBhdCAxNTozMCArMDAwMCwgR3J1bWJhY2gsIEVt
bWFudWVsIHdyb3RlOg0KPiA+ID4gSGkgQmVuLA0KPiA+ID4gDQo+ID4gPiANCj4gPiA+IFRoYW5r
cyBmb3IgbG9va2luZyBhdCBvdXIgY29kZS4NCj4gPiA+IA0KPiA+ID4gDQo+ID4gPiBPbiBXZWQs
IDIwMTYtMDQtMjAgYXQgMTY6MDggKzAxMDAsIEJlbiBIdXRjaGluZ3Mgd3JvdGU6DQo+ID4gPiA+
IA0KPiA+ID4gPiBJJ20gbm90IHN1cmUgaWYgeW91IHdlcmUgYXdhcmUsIGJ1dCB0aGVyZSBpcyBh
IHN0YW5kYXJkIEFQSSBmb3INCj4gPiA+ID4gY29uZmlndXJpbmcgUlNTIGluIG5ldHdvcmsgZHJp
dmVycywgcGFydCBvZiBldGh0b29sX29wcy4gIEkNCj4gPiA+ID4gdGhpbmsNCj4gPiA+ID4gaXds
d2lmaSBzaG91bGQgaW1wbGVtZW50IHRoYXQgcmF0aGVyIHRoYW4gYSBkcml2ZXItc3BlY2lmaWMN
Cj4gPiA+ID4gZGVidWdmcw0KPiA+ID4gPiBpbnRlcmZhY2UuDQo+ID4gPiA+IA0KPiA+ID4gWW91
IGFyZSByaWdodCwgdGhpcyBpcyB3aHkgU2FyYSBtYWRlIHRoaXMgY29tbWl0Og0KPiA+ID4gDQo+
ID4gPiBjb21taXQgODU0ZDc3M2U0YWI1ODY5MjAwMDA0YWY0Y2E1ZDg1MTczMDg0OTkwMw0KPiA+
ID4gQXV0aG9yOiBTYXJhIFNoYXJvbiA8c2FyYS5zaGFyb25AaW50ZWwuY29tPg0KPiA+ID4gRGF0
ZTogICBUdWUgTWFyIDIyIDE1OjU1OjU4IDIwMTYgKzAyMDANCj4gPiA+IA0KPiA+ID4gICAgIGl3
bHdpZmk6IG12bTogaW1wcm92ZSBSU1MgY29uZmlndXJhdGlvbg0KPiA+ID4gICAgIA0KPiA+ID4g
ICAgIEltcHJvdmUgY3VycmVudCBSU1MgY29uZmlndXJhdGlvbjoNCj4gPiA+ICAgICAgKiBVc2Ug
bmV0ZGV2X3Jzc19rZXkgaW5zdGVhZCBvZiBrZWVwaW5nIGEgbG9jYWwgY29weS4NCj4gPiA+ICAg
ICAgKiBDb25maWd1cmUgYWxzbyBVRFAgaGFzaGluZyB0byBoYXZlIFVEUCB0cmFmZmljIHNwcmVh
ZA0KPiA+ID4gYWNyb3NzDQo+ID4gPiBxdWV1ZXMuDQo+ID4gPiAgICAgIERvIG5vdCBkaXJlY3Qg
UlNTIHRyYWZmaWMgdG8gb3VyIGZhbGxiYWNrIHF1ZXVlLg0KPiA+ID4gDQo+ID4gPiAgICAgU2ln
bmVkLW9mZi1ieTogU2FyYSBTaGFyb24gPHNhcmEuc2hhcm9uQGludGVsLmNvbT4NCj4gPiA+ICAg
ICBTaWduZWQtb2ZmLWJ5OiBFbW1hbnVlbCBHcnVtYmFjaCA8ZW1tYW51ZWwuZ3J1bWJhY2hAaW50
ZWwuY29tDQo+ID4gPiA+DQo+ID4gDQo+ID4gVGhhdCBkb2Vzbid0IHJlYWxseSBhZGRyZXNzIHdo
YXQgSSBzYWlkLiAgWWVzLCBpdCdzIHVzaW5nIHRoZQ0KPiA+IGNvbW1vbg0KPiA+IFJTUyBrZXks
IGJ1dCBpdCdzIG5vdCBpbXBsZW1lbnRpbmcgdGhlIGV0aHRvb2wgb3BlcmF0aW9ucyB0byBnZXQN
Cj4gPiBhbmQNCj4gPiBzZXQgdGhlIGluZGlyZWN0aW9uIHRhYmxlIGFuZCB0aGUgdHlwZXMgb2Yg
ZmxvdyBoYXNoaW5nIHRoYXQgYXJlDQo+ID4gZW5hYmxlZC4NCj4gPiANCj4gDQo+IEhtLi4gSSB0
aGluayB0aGF0IHNldHRpbmcgdGhlIGluZGlyZWN0aW9uIHRhYmxlIGlzIGEgcHJvYmxlbSBpbiBv
dXINCj4gY2FzZSBiZWNhdXNlIHRoZSBQTiBjaGVjayBpcyBkb25lIGluIHRoZSBkcml2ZXIuIFRo
ZSBQTiBjaGVjayBpcyB0aGUNCj4gd2F5IFdpRmkgYWRkcmVzc2VzIHRoZSByZXBsYXkgYXR0YWNr
LCBhbmQgc2luY2UgdGhlIFBOIGNoZWNrIHJlbGllcw0KPiBvbg0KPiBwZXItY3B1IHZhcmlhYmxl
cywgd2UgY2Fubm90ICpzYWZlbHkqIGFsbG93IHVzZXJzIHRvIG1vZGlmeSB0aGUNCj4gaW5kaXJl
Y3Rpb24gdGFibGUgd2hpbGUgdHJhZmZpYyBpcyBmbG93aW5nLg0KPiANCg0KT2ggLSBhbmQgeWVz
LCB3ZSBkbyBoYXZlIGEgZGVidWdmcyBob29rIGZvciB0aGF0LiBJdCB3YXMgbWVhbnQgZm9yIHRo
ZQ0KdmVyeSBlYXJseSB0ZXN0aW5nIHBoYXNlcy4gSSBndWVzcyB0aGF0IHdlIHNob3VsZCByZW1v
dmUgaXQgYXQgc29tZQ0KcG9pbnQsIHVubGVzcyB3ZSBkZWNpZGUgdGhhdCB3aG9ldmVyIHdhbnRz
IHRvIHBsYXkgd2l0aCB0aGUgaW5kaXJlY3Rpb24NCnRhYmxlIHdoaWxlIGluYm91bmQgdHJhZmZp
YyBpcyBmbG93aW5nIHB1dHMgaGltc2VsZiBhdCByaXNrLg0KDQo+ID4gQmVuLg0KPiA+IA==

Re: RSS configuration in iwlwifi

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]

On Wed, 2016-04-20 at 16:22 +0000, Grumbach, Emmanuel wrote:
> On Wed, 2016-04-20 at 16:47 +0100, Ben Hutchings wrote:
> > 
> > On Wed, 2016-04-20 at 15:30 +0000, Grumbach, Emmanuel wrote:
> > > 
> > > Hi Ben,
> > > 
> > > 
> > > Thanks for looking at our code.
> > > 
> > > 
> > > On Wed, 2016-04-20 at 16:08 +0100, Ben Hutchings wrote:
> > > > 
> > > > 
> > > > I'm not sure if you were aware, but there is a standard API for
> > > > configuring RSS in network drivers, part of ethtool_ops.  I think
> > > > iwlwifi should implement that rather than a driver-specific
> > > > debugfs
> > > > interface.
> > > > 
> > > You are right, this is why Sara made this commit:
> > > 
> > > commit 854d773e4ab5869200004af4ca5d851730849903
> > > Author: Sara Sharon <sara.sharon@intel.com>
> > > Date:   Tue Mar 22 15:55:58 2016 +0200
> > > 
> > >     iwlwifi: mvm: improve RSS configuration
> > >     
> > >     Improve current RSS configuration:
> > >      * Use netdev_rss_key instead of keeping a local copy.
> > >      * Configure also UDP hashing to have UDP traffic spread across
> > > queues.
> > >      Do not direct RSS traffic to our fallback queue.
> > > 
> > >     Signed-off-by: Sara Sharon <sara.sharon@intel.com>
> > >     Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> > That doesn't really address what I said.  Yes, it's using the common
> > RSS key, but it's not implementing the ethtool operations to get and
> > set the indirection table and the types of flow hashing that are
> > enabled.
> > 
> Hm.. I think that setting the indirection table is a problem in our
> case because the PN check is done in the driver. The PN check is the
> way WiFi addresses the replay attack, and since the PN check relies on
> per-cpu variables, we cannot *safely* allow users to modify the
> indirection table while traffic is flowing.

I see.  You could make this work when the interface is down and return
-EBUSY if the interface is up.

Ben.

-- 
Ben Hutchings
Never attribute to conspiracy what can adequately be explained by stupidity.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: RSS configuration in iwlwifi

[Johannes Berg]

On Wed, 2016-04-20 at 18:05 +0100, Ben Hutchings wrote:
> 
> I see.  You could make this work when the interface is down and
> return -EBUSY if the interface is up.
> 

It's slightly more complicated, since it doesn't just affect a single
netdev but possibly more than one (since the table is shared by all
virtual interfaces) - but yes, it could be done.

johannes