GNOME Bugzilla – Bug 739084
[review] dcbw/wifi-powersave: enable Wi-Fi powersave if connection requests it
Last modified: 2015-02-25 14:09:02 UTC
Created attachment 289218 [details] [review] enable powersave via nl80211 This is also in the TODO list for NetworkManager, for tablet/mobile behavior. NM should, among other things, enable power saving for the devices that support it. Since I'm using 0.9.8.8 at the moment, I've attached a patch that does this on that version. I'll make a new patch for git master.
Created attachment 289223 [details] [review] Patch against git master
Looks right to me.
It was pointed out to me that we should do a bit more performance testing on this patch, to make sure it doesn't adversely affect throughput. We know on the intel wifi drivers it's most likely just fine, because as soon as you try to pass lots of traffic through, they will revert to non-powersave by themselves. I'm not in a position to complete that testing this week though, I don't have all my hardware with me; I'll get started on it next week.
Ok, let us know how the testing goes. I recall that powersave can sometimes interact badly with some APs though, so should we also have an '802-11-wireless' setting option for powersave? It should default to TRUE but then could be turned off if it caused problems for certain users.
I reworked the patches a bit for git master and added the powersave property, and pushed to dcbw/wifi-powersave. One thing I'm not sure about is what kind of values we want to support here. There's "old" WiFi powersaving (PSPoll) and there's "new" (U-APSD). Both can theoretically cause problems such that you'd want to turn them off, but I'm not sure whether you'd want to enable PSPoll and disable U-APSD or what. So my concern is that I don't know what the option matrix is here, thus I created the 'powersave' property as guint32 so that we could make it a flags-based variable later. Mathieu, any results from your testing?
Oh, two other changes from Mathieu's patches: 1) powersave is only relevant for STA mode, so I've wrapped the powersave setting bits in nm-device-wifi.c in a check for that. Yes, U-APSD could be enabled in AP-mode, but we'll leave that up to the driver, plus I'm not even sure the supplicant's lightweight AP supports that. 2) I've moved the enable bits to stage2 along with other wifi-type setup, since enabling powersave after connection could reconfigure the device and require a reconnection, and I"ve moved the disable to the 'deactivate' function which is called whenever an activation or activation attempt is cleaned up.
+ * Wi-Fi power saving behavior is enabled. All other values are reserved. since verify() checks that the value is not larger then 1, I don't see how we could later modify the valid range without breaking backward compatibility. Either don't check the values in verify(), or just make it a boolean. would also need nmcli support. +nm_platform_wifi_set_powersave (int ifindex, guint32 powersave) +{ + reset_error (); + + g_return_if_fail (ifindex > 0); + + klass->wifi_set_powersave (platform, ifindex, powersave); +} + either - check whether virtual method is implemented (if(klass->wifi_set_powersave)) - implement a stub for fake-platform - implement a NOP implementation in NMPlatform (I prefer this). + nm_log_err (LOGD_HW | LOGD_WIFI, "(%s): error setting powersave %d", + wext->parent.iface, powersave); G_GUINT32_FORMAT ? rest LGTM
I've done some testing here to try to see if there was a difference in bandwidth as reported by iperf between systems running with or without powersave enabled. I've tested one ath9k system, which has powersave enabled by default; I don't see major differences (though it's hard to test this properly outside a clean lab). There was also an intel system (iwldvm); also with powersave enabled by default. Again, no major differences. I also tested a broadcom system (can't enable powersave), a zd1211 dongle I had (also can't enable powersave); and an Edimax AC-1200 dongle (same as intel and ath9k, uses rtl8812). It satisfied me for the performance tests -- things match with the documentation for dynamic power save, so as long as devices that can't do powersave don't cause the device activation to fail, I think we're fine.
Fixed up for comments and repushed. Please re-review!
The fact that Mathieu's testing did not turn up any performance improvements makes me wonder whether these patches really make sense... > libnm/libnm-util: add Wi-Fi 'powersave' property This is already belated, but we need to properly deal with the new APIs we're adding. ("Since" tags, NM_AVAILABLE_IN_ macros, a new section in libnm.ver, etc.) >+ * If set to 0, Wi-Fi power saving behavior is disabled. If set to 1, >+ * Wi-Fi power saving behavior is enabled. All other values are reserved. You could call it TRUE/FALSE for now (but noting that it's a uint-valued property, not gboolean-valued), and if we add more values later, we can then claim that it's a flags type (with "BLAH_BLAH_DISABLED" being 0 and "BLAH_BLAH_ENABLE_ALL" being 1). >+ g_param_spec_uint (NM_SETTING_WIRELESS_POWERSAVE, "", "", >+ 0, G_MAXUINT32, 0, You had suggested having it be enabled by default before... (Remember to add G_PARAM_CONSTRUCT too to cause the default value to actually be set if you change that.) > cli: add support for wifi.powersave setting >+ return nm_setting_wireless_get_powersave (s_wireless) ? >+ g_strdup (_("yes")) : g_strdup (_("no")); possibly distinguish == 1 ("yes") from > 1 ("yes (0x%x)") for some forward-compatibility ? > wifi: set wireless power-save for nl80211 devices when activated s/power-save/powersave/ for consistency (here and in the next commit message) > (fixups and WEXT implementation by dcbw) Did you test the WEXT code? If not, I'd say drop it.
Actually, the fact that my testing didn't turn up performance differences can be one of three things: it's beyond what I used to measure performance (and/or hidden by noise), there is no difference, or I didn't measure properly. That's added to the fact that we're not looking for wifi performance differences, but rather power usage changes -- those I have already verified, we see a marked difference, for example, on a Nexus 4 running our Ubuntu Touch images, between with and without powersave enabled. From there I did a very unscientific check with iperf to try to see whether there was a noticeable difference, nothing more. Since it's wifi, it's already hard to map this as even though I have control over my wireless AP, etc, there is still interference, and a load of other things that can impact on the performance and speed of data transfers. I wasn't testing any of this in a clean room. I haven't personally testing the WEXT code, my devices are all nl80211.
(In reply to comment #10) > > libnm/libnm-util: add Wi-Fi 'powersave' property > > This is already belated, but we need to properly deal with the new APIs we're > adding. ("Since" tags, NM_AVAILABLE_IN_ macros, a new section in libnm.ver, > etc.) bug 742993
(In reply to comment #10) > The fact that Mathieu's testing did not turn up any performance improvements > makes me wonder whether these patches really make sense... It's actually not about performance, it's about battery savings. But performance enters into the picture because we need to ensure that powersave doesn't adversely affect throughput. If enabling powersave killed performance but saved battery, that would be a big issue. > > libnm/libnm-util: add Wi-Fi 'powersave' property > > This is already belated, but we need to properly deal with the new APIs we're > adding. ("Since" tags, NM_AVAILABLE_IN_ macros, a new section in libnm.ver, > etc.) Fixed, though I've now rebased on top of danw/versioning-bgo742993 > >+ * If set to 0, Wi-Fi power saving behavior is disabled. If set to 1, > >+ * Wi-Fi power saving behavior is enabled. All other values are reserved. > > You could call it TRUE/FALSE for now (but noting that it's a uint-valued > property, not gboolean-valued), and if we add more values later, we can then > claim that it's a flags type (with "BLAH_BLAH_DISABLED" being 0 and > "BLAH_BLAH_ENABLE_ALL" being 1). Done. > >+ g_param_spec_uint (NM_SETTING_WIRELESS_POWERSAVE, "", "", > >+ 0, G_MAXUINT32, 0, > > You had suggested having it be enabled by default before... (Remember to add > G_PARAM_CONSTRUCT too to cause the default value to actually be set if you > change that.) Yeah, I did. However, there can still be problems with powersave and specific APs which could cause disconnections, so we might get more bugs in the future if we go enabled by default. Also, there's the question about changing current connections to powersave by default, which I'm not sure we should do, but would be more consistent. (if we didn't change existing configs to powersave, then newly created ones would default to on but existing ones would default to off, which could be confusing, so maybe we should just bit the bullet and turn it on for everyone) > > cli: add support for wifi.powersave setting > > >+ return nm_setting_wireless_get_powersave (s_wireless) ? > >+ g_strdup (_("yes")) : g_strdup (_("no")); > > possibly distinguish == 1 ("yes") from > 1 ("yes (0x%x)") for some > forward-compatibility ? Done. > > wifi: set wireless power-save for nl80211 devices when activated > > s/power-save/powersave/ for consistency (here and in the next commit message) > > > (fixups and WEXT implementation by dcbw) > > Did you test the WEXT code? If not, I'd say drop it. Nope, I didn't. But now I'm totally going to :P Repushed!
(In reply to comment #13) > (In reply to comment #10) > > Did you test the WEXT code? If not, I'd say drop it. > > Nope, I didn't. But now I'm totally going to :P Yeah, it works.
(In reply to comment #13) > (In reply to comment #10) > > The fact that Mathieu's testing did not turn up any performance improvements > > makes me wonder whether these patches really make sense... > > It's actually not about performance, it's about battery savings. But > performance enters into the picture because we need to ensure that powersave > doesn't adversely affect throughput. If enabling powersave killed performance > but saved battery, that would be a big issue. Oh, oops, I didn't read carefully enough and thought he meant that battery performance was unchanged, not that networking performance was unchanged >+ * reserved. Note that even though boolean values are allowed, the property >+ * type is an unsigned integer to allow for future expansion. I think you want the word "only" before "boolean" there. Everything else looks good. Assuming you tested the WEXT code :)
Fixed up for wording, a small nmcli fix, trivial ifcfg-rh patch to save the value, and pushed. 5293683e4a01df1dc47e884ead3a6c6c9493b1f9 6a3531d02d72b51b6070362df218f5d4c84edc62 c3e319266c64a418f9edba2c95187fefc04c203f a428de82158fc9006820a2b4888e7260dfbfdbda