Messages in this thread | | | Date | Thu, 14 Aug 2014 11:16:40 -0700 | From | Saravana Kannan <> | Subject | Re: [PATCH] cpufreq, store_scaling_governor requires policy->rwsem to be held for duration of changing governors [v2] |
| |
On 08/13/2014 12:57 PM, Prarit Bhargava wrote: > > > On 08/05/2014 06:51 PM, Saravana Kannan wrote: > >> >> I definitely have a fix for this and the original race you reported. It's >> basically reverting that commit you reverted + a fix for the deadlock. That's >> the only way to fix the scaling_governor issue. >> >> You fix the deadlock by moving the governor attribute group removing to the >> framework code and doing it before STOP+EXIT to governor without holding the >> policy lock. And the reverse for INIT+STOP. >> > > I'm still not convinced of the deadlock so I did a bit of additional research > and am pretty close to saying that this is a false positive from the lockdep > code in the kernfs area. > > A few things that have caused me concern about the lockdep splat we're seeing: > > 1. The splat occurs when we hit __kernfs_remove+0x25b/0x360 which resolves to > > if (kernfs_lockdep(kn)) { > rwsem_acquire(&kn->dep_map, 0, 0, _RET_IP_); <<< RIGHT HERE > if (atomic_read(&kn->active) != KN_DEACTIVATED_BIAS) > lock_contended(&kn->dep_map, _RET_IP_); > } > > ie) the *ONLY* way we hit a "deadlock" in this code is if we have LOCKDEP > configured in the kernfs. > > It should be noted, that having kernfs_lockdep() always return 0 [1], results in > NO additional lockdep warnings. > > Additionally the splat contains > > [ 107.428421] CPU0 CPU1 > [ 107.433482] ---- ---- > [ 107.438544] lock(&policy->rwsem); > [ 107.442459] lock(s_active#98); > [ 107.448916] lock(&policy->rwsem); > [ 107.455650] lock(s_active#98); > > which also points to the situation above (s_active is the default naming used in > the kernfs lockdep code). > > In short -- there is no deadlock here. It only happens in the lockdep code > itself, not because lockdep has identified a real problem. > > 2. I then started asking myself why this was occurring. The reason appears to > be that the attribute for scaling_governor is deleting other sysfs attributes > and that got me to wondering if there were other areas where this occurred and I > remembered it does! In the USB code writing and reading to the bConfiguration > of a device may lead to the removal of "down stream" attributes. The reading > and writing of bConfiguration occurs in > drivers/usb/core/sysfs.c:79 > > > /* configuration value is always present, and r/w */ > usb_actconfig_show(bConfigurationValue, "%u\n"); > > static ssize_t bConfigurationValue_store(struct device *dev, > struct device_attribute *attr, > const char *buf, size_t count) > { > struct usb_device *udev = to_usb_device(dev); > int config, value; > > if (sscanf(buf, "%d", &config) != 1 || config < -1 || config > 255) > return -EINVAL; > usb_lock_device(udev); > value = usb_set_configuration(udev, config); > usb_unlock_device(udev); > return (value < 0) ? value : count; > } > > ... and the next lines are IMO important here: > > static DEVICE_ATTR_IGNORE_LOCKDEP(bConfigurationValue, S_IRUGO | S_IWUSR, > bConfigurationValue_show, bConfigurationValue_store); > > FWIW, it isn't *exactly* the same ... but commit > 356c05d58af05d582e634b54b40050c73609617b explains a similarity between what is > happening with our lockdep splat and the lockdep issues seen in USB.
This seems VERY different from our situation. I don't see an equivalent of a policy lock that's grabbed from both threads, but in opposite order.
If I'm not mistaken, the sysfs entry here uses some wait/complete pair to wait for something. But that's an equivalent of a semaphore with max count of 1. Lockdep just seems to be making it obvious by adding semaphore calls.
So, a semaphore equivalent deadlock with another semaphore. I believe this is a read deadlock.
-Saravana
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
| |