Bug 27532
| Summary: | ath9k prevents CPU from entering lower C-states | ||
|---|---|---|---|
| Product: | Drivers | Reporter: | Thomas Bächler (thomas) |
| Component: | network-wireless | Assignee: | drivers_network-wireless (drivers_network-wireless) |
| Status: | CLOSED CODE_FIX | ||
| Severity: | high | CC: | ath9k-devel, florian, jirislaby, linville, maciej.rutecki, mcgrof, r.schtz, rjw, senthilkumar, shafi.wireless |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 2.6.37 | Subsystem: | |
| Regression: | Yes | Bisected commit-id: | |
| Bug Depends on: | |||
| Bug Blocks: | 7216, 21782 | ||
| Attachments: |
throughput with the patch applied
throughput without the patch applied /proc/cpuinfo lspci diff patch which addresses ath9k CPU C3 state regression final fix for ath9k-prevents CPU entering C3 states |
||
|
Description
Thomas Bächler
2011-01-24 22:43:03 UTC
(In reply to comment #0) > In 2.6.37, when ath9k is active (interface up and connected to wireless), my > CPU (Intel Core i5-520M) is not entering the lowest C-state anymore > (according > to powertop). > > This is caused by commit [1] and reported on the linux-wireless list at [2]. > Reverting the commit fixes the problem and returns it to 2.6.36 behaviour. > > > As a response to the mailing list post following [2]: I don't care about > alleged throughput problems on some Pinetrail platform I do not have. Nor do > I > care that this may be "fixed" by making the QoS values configurable in > 2.6.38. > > What I do care about is that this commit is keeping my CPU in a higher C > state > than necessary constantly, probably diminishing my battery life > significantly. > This is a serious regression for me. > > [1] > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=10598c124ecabbbfd7522f74de19b8f7d52a1bee > [2] http://permalink.gmane.org/gmane.linux.kernel.wireless.general/63430 We will look into it that, but reverting the commit may not be a correct solution. I would think it is (for now). However, I did that locally anyway, so I am happy for the moment. (In reply to comment #2) > I would think it is (for now). However, I did that locally anyway, so I am > happy for the moment. Thanks! If you need any help please free to ask. Nevertheless, that only buys a little time. When can we expect to see patches to address this? Reverting the patch is still on the table -- we generally don't trade one bug for another. "Throughput was severely affected in Intel Pinetrail platforms because of a DMA problem in C3 state. This patch fixes this issue." Was there a discussion/bugreport about this so-called several performance issue? My netbook is based on Pinetrail and I haven't noticed it so far. So I reverted the patch locally to save power, too. I am inclined to revert it as well -- I was trying to give the ath9k guys a chance to fix both issues together... Um I think this is an issue not with ath9k but with certain BIOSes which do not have a fix for C3 state. Did you upgrade your BIOS? It is an Samsung N145 Plus. I am still using the original BIOS version. It's timestamp is 20100818. (In reply to comment #7) > Um I think this is an issue not with ath9k but with certain BIOSes which do > not > have a fix for C3 state. Did you upgrade your BIOS? Luis, I am not sure I can follow (and Richard and I are in disagreement about what you actually meant by that statement). Can you elaborate a bit more? (In reply to comment #7) > Um I think this is an issue not with ath9k but with certain BIOSes which do > not > have a fix for C3 state. Did you upgrade your BIOS? I hope you're asking because that information would help you fix the problem and you're not going to say that a BIOS upgrade is necessary to avoid the problem. (In reply to comment #5) > "Throughput was severely affected in Intel Pinetrail platforms > because of a DMA problem in C3 state. This patch fixes this > issue." > > Was there a discussion/bugreport about this so-called several performance > issue? My netbook is based on Pinetrail and I haven't noticed it so far. So I > reverted the patch locally to save power, too. Please look at the performance/throughput when the battery is discharging(ie the power adapter is not connected to power supply). Created attachment 45442 [details]
throughput with the patch applied
Created attachment 45452 [details]
throughput without the patch applied
The troughput is somewhat lower, but that is nothing I would call severe. I think that is quite normal, because the CPU is changing between the C-States all the time and resuming from C4 (lowest C-state of my Atom) takes quite long in comparison to other C-state transitions.
10% is a significant difference -- not sure if it is "severe" or not. Can you quantify the difference in power consumption? (In reply to comment #14) > 10% is a significant difference -- not sure if it is "severe" or not. > > Can you quantify the difference in power consumption? powertop measures the following: without the patch: idle: 6.4W wireless traffic: 9.1W with the patch: idle: 7.5W wireless traffic: 9.2W (In reply to comment #15) > (In reply to comment #14) > > 10% is a significant difference -- not sure if it is "severe" or not. > > > > Can you quantify the difference in power consumption? > > > powertop measures the following: > > without the patch: > idle: 6.4W > wireless traffic: 9.1W > > with the patch: > idle: 7.5W > wireless traffic: 9.2W Richard I do accept the results are going to be inconsistent, but it was reported up-to 50% drop in throughput and thats why PM-QOS patch was sent. (In reply to comment #16) > Richard I do accept the results are going to be inconsistent, but it was > reported up-to 50% drop in throughput and thats why PM-QOS patch was sent. Could this be tracked down to a certain hardware combination? I believe we should address this cleanly as obviously, we don't want to restrict the lowest C-state for all the laptops. The patch was supposed to address only the problematic laptops.. not all. We shall analyze this and shall keep you posted. The C3 issue on Pinetrail is fixed by a BIOS upgrade, if no BIOS upgrade is available then the patch addresses the issue through a workaround by preventing the CPU from going into C3. If such a hack is desired upstream then a module parameter should likely be the thing to do but I do wonder -- ath9k should not be the only device affected.. so instead is it too much wishful thinking for this to be done properly through some C3-workaround hack for Pinetrail specifically designed to detect the poo somehow? If you are seeing this issue please post the output of lspci and cat /proc/cpuinfo Created attachment 46262 [details]
/proc/cpuinfo
As you can see, not a Pinetrail platform at all, still your "fix" affects me.
Created attachment 46272 [details]
lspci
IMHO this "fix" and other on other drivers should be reverted and instead userspace / distributions should figure out a way to let users enable a polling mechanism for cases where this is necessary, hopefully from userspace. http://johannes.sipsolutions.net/files/netlatency.c.txt like that but for CPU dma latency. Or foo /dev/cpu_dma_latency (In reply to comment #20) > Created an attachment (id=46262) [details] > /proc/cpuinfo > > As you can see, not a Pinetrail platform at all, still your "fix" affects me. Hi, can you please try this when you load the ath9k driver sudo modprobe -r ath9k sudo modprobe ath9k pmqos=0 (or) sudo modprobe ath9k pmqos=-1 *with this please see whether the CPU is entering lower C-states. *If it works we can have the pmqos value by default 0/-1 Interesting suggestion: $ modinfo ath9k|grep ^parm: parm: debug:Debugging mask (uint) parm: nohwcrypt:Disable hardware encryption (int) parm: blink:Enable LED blink on activity (int) (In reply to comment #24) > Interesting suggestion: > > $ modinfo ath9k|grep ^parm: > parm: debug:Debugging mask (uint) > parm: nohwcrypt:Disable hardware encryption (int) > parm: blink:Enable LED blink on activity (int) Looks like the patch to make pmqos value as modparam is not there. Can you please try with the latest compat wireless/wireless testing. Any way I will soon send a RFC patch to address this issue Created attachment 46842 [details]
diff patch which addresses ath9k CPU C3 state regression
Is there any progress here? This problem is seen on AMD Brazos with latest BIOS too. (In reply to comment #26) > Created an attachment (id=46842) [details] > diff patch which addresses ath9k CPU C3 state regression BTW this fixes it... A version of that patch was posted, there were some comments, and I don't recall seeing the follow-up yet...? (In reply to comment #29) > A version of that patch was posted, there were some comments, and I don't > recall seeing the follow-up yet...? John/Jiri, The likely decision is to completely move it to the user space and remove it completely remove it from the driver code. (In reply to comment #30) > The likely decision is to completely move it to the user space and remove > it completely remove it from the driver code. Ok, could you send a patch to John, so that we can merge it? Or do you expect people to disable it by the module parameter? (In reply to comment #31) > (In reply to comment #30) > > The likely decision is to completely move it to the user space and > remove > > it completely remove it from the driver code. > > Ok, could you send a patch to John, so that we can merge it? > > Or do you expect people to disable it by the module parameter? Jiri for the time being please use that temporary patch which by default disable pm-qos. I will be sending an RFC patch to completely remove the pm-qos thing from the driver. We are taking time as the platform which had this issue had to be validated by the script alone. I think this has taken long enough...I'm going to revert commit 10598c124ecabbbfd7522f74de19b8f7d52a1bee and the related commit 4dc3530df7c0428b41c00399a7ee8c929406d181. (In reply to comment #33) > I think this has taken long enough...I'm going to revert commit > 10598c124ecabbbfd7522f74de19b8f7d52a1bee and the related commit > 4dc3530df7c0428b41c00399a7ee8c929406d181. John please wait I will send a RFC patch now. Reverting commit 98c316e348bedffa730e6f1e4baeb8a3c3e0f28b as well... (In reply to comment #35) > Reverting commit 98c316e348bedffa730e6f1e4baeb8a3c3e0f28b as well... John already sent an RFC patch. (In reply to comment #31) > (In reply to comment #30) > > The likely decision is to completely move it to the user space and > remove > > it completely remove it from the driver code. > > Ok, could you send a patch to John, so that we can merge it? > > Or do you expect people to disable it by the module parameter? the patch which completely disable pm-qos in ath9k is merged in wireless-testing. Attaching the patch. Can some one please close the bug. Created attachment 47952 [details]
final fix for ath9k-prevents CPU entering C3 states
Handled-By : Mohammed Shafi Shajakhan <mshajakhan@atheros.com> Can this be submitted to stable, too? Thanks. (In reply to comment #41) > Can this be submitted to stable, too? Thanks. Ok. (In reply to comment #41) > Can this be submitted to stable, too? Thanks. Hi there, patch to stable is submitted today. merged for .38-rc7: commit 0f5cd45960173ba5b36727decbb4a241cbd35ef9 Author: Mohammed Shafi Shajakhan <mshajakhan@atheros.com> Date: Tue Feb 15 21:29:32 2011 +0530 ath9k: Fix ath9k prevents CPU to enter C3 states |