Messages in this thread | | | Date | Thu, 31 Jul 2014 06:21:33 -0400 | From | Prarit Bhargava <> | Subject | Re: [PATCH] cpufreq, store_scaling_governor requires policy->rwsem to be held for duration of changing governors [v2] |
| |
On 07/31/2014 06:16 AM, Prarit Bhargava wrote: > > > On 07/30/2014 10:16 PM, Rafael J. Wysocki wrote: >> On Wednesday, July 30, 2014 06:36:00 PM Saravana Kannan wrote: >>> On 07/30/2014 02:40 PM, Rafael J. Wysocki wrote: >>>> On Wednesday, July 30, 2014 10:18:25 AM Prarit Bhargava wrote: >>>>> >>>>> On 07/29/2014 08:03 PM, Rafael J. Wysocki wrote: >>>>>> On Tuesday, July 29, 2014 07:46:02 AM Prarit Bhargava wrote: >>>> >>>> [cut] >>>> >>>>>>> This patch effectively reverts commit 955ef483. >>> >>> The issue reported in this patch is valid. We are seeing that internally >>> too. I believe I reported it in another thread (within the past month). >>> >>> However, the original patch fixes a real deadlock issue (I'm too tired >>> to look it up now). We can revet the original, but it's going to bring >>> back the original issue. I just want to make sure Prarit and Raphael >>> realize this before proceeding. > > Hi Saravana, > > Thanks for your input. I went back to the code and confirmed my original > statement about this patch. > > Note: in a previous email I erroneously wrote "buffer->mutex" when I should > have identified the lock as sysfs_mutex. Sorry 'bout that, and apologies > for any confusion that may have caused. > > From my commit message: > > "In any case, the current linux.git code no longer can reproduce the original > failure; the locking in the sysfs release code has changed." > > The original patch attempted to fix this deadlock: > > A cpufreq driver on a file read did: > > -> #0 (&per_cpu(cpu_policy_rwsem, cpu)){+++++.}: > [<c0055253>] __lock_acquire+0xef3/0x13dc > [<c0055a79>] lock_acquire+0x61/0xbc > [<c03ee1f5>] down_read+0x25/0x30 > [<c02f6179>] lock_policy_rwsem_read+0x25/0x34 > [<c02f6edd>] show+0x21/0x58 > [<c00f9c0f>] sysfs_read_file+0x67/0xcc > [<c00b40a7>] vfs_read+0x63/0xd8 > [<c00b41fb>] sys_read+0x2f/0x50 > [<c000cdc1>] ret_fast_syscall+0x1/0x52 > > lock(s_active#41) [ which is actually the acquisition of sysfs_mutex ] > lock(&per_cpu(cpu_policy_rwsem, cpu)); > > and on the governor switch (notably the EXIT of the existing governor), the > opposite occurs > > -> #1 (s_active#41){++++.+}: > [<c0055a79>] lock_acquire+0x61/0xbc > [<c00fabf1>] sysfs_addrm_finish+0xc1/0x128 > [<c00f9819>] sysfs_hash_and_remove+0x35/0x64 > [<c00fbe6f>] remove_files.isra.0+0x1b/0x24 > [<c00fbea5>] sysfs_remove_group+0x2d/0xa8 > [<c02f9a0b>] cpufreq_governor_interactive+0x13b/0x35c > [<c02f61df>] __cpufreq_governor+0x2b/0x8c > [<c02f6579>] __cpufreq_set_policy+0xa9/0xf8 > [<c02f6b75>] store_scaling_governor+0x61/0x100 > [<c02f6f4d>] store+0x39/0x60 > [<c00f9b81>] sysfs_write_file+0xed/0x114 > [<c00b3fd1>] vfs_write+0x65/0xd8 > [<c00b424b>] sys_write+0x2f/0x50 > [<c000cdc1>] ret_fast_syscall+0x1/0x52 > > > lock(&per_cpu(cpu_policy_rwsem, cpu)); > lock(s_active#41) [ which is actually the acquisition of sysfs_mutex ] > > The sysfs_mutex no longer blocks in the sysfs path, and I have built with > LOCKDEP on and off to confirm that I do not see any tracebacks or hangs. I > tested this by doing a few reads of the current governor, and then doing a > governor switch (to at least initiate the LOCKDEP warning). IIUC the traceback > above that is the way to reproduce this LOCKDEP warning.
^^^ this should not be taken as 'I did only a few reads ...'. I tested quite extensively across 15 different systems and added a read of the scaling_governor files in my little reproducer.
P.
| |