Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
viOn Tue, 2014-04-22 at 11:34 +0800, Li Zhong wrote:
> On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
> > Hello,
> >
> > On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
> >
> > Proper /** function comment would be nice.
>
> Ok, will try to write some in next version.
>
> >
> > > +struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
> > > +struct device_attribute *attr)
> >
> > I can see why you did this but let's please not require the user of
> > this function to see how the thing is working internally. Let's
> > return int and keep track of (or look up again) the kernfs_node
> > internally.
>
> Ok, it also makes the prototype of lock and unlock look more consistent
> and comfortable.
When trying to do an new version of the patch, I find that if the device
is really removed, then we couldn't look up using the parent, and
attribute name again in unlock. So I guess maybe I could add one more
argument, e.g. kn_out,:q to track this kernfs node.
Code will be posted soon for your review.
Thanks, Zhong
>
> >
> > > {
> > ...
> > > + /*
> > > + * We assume device_hotplug_lock must be acquired before removing
> >
> > Is this assumption true? If so, can we add lockdep assertions in
> > places to verify and enforce this? If not, aren't we just feeling
> > good when the reality is broken?
>
> It seems not true ... I think there are devices that don't have the
> online/offline concept, we just need to add it, remove it, like ethernet
> cards.
>
> Maybe we could change the comments above, like:
> /* We assume device_hotplug_lock must be acquired before
>* removing devices, which have online/offline sysfs knob,
>* and some locks are needed to serialize the online/offline
>* callbacks and device removing. ...
> ?
>
> And we could add lockdep assertions in cpu and memory related code? e.g.
> remove_memory(), unregister_cpu()
>
> Currently, remove_memory() has comments for the function:
>
> * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> * and online/offline operations before this call, as required by
> * try_offline_node().
> */
>
> maybe it could be removed with the lockdep assertion.
>
> > ...
> >
> > Function comment please.
>
> OK.
>
> Thanks, Zhong
>
> > > +void unlock_device_hotplug_sysfs(struct device *dev,
> > > + struct kernfs_node *kn)
> > > +{
> > > + unlock_device_hotplug();
> > > + kernfs_unbreak_active_protection(kn);
> > > + put_device(dev);
> > > + kernfs_put(kn);
> > > }
> >
> > Thanks.
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
viOn Tue, 2014-04-22 at 11:34 +0800, Li Zhong wrote:
On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
Hello,
On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
Proper /** function comment would be nice.
Ok, will try to write some in next version.
+struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
+struct device_attribute *attr)
I can see why you did this but let's please not require the user of
this function to see how the thing is working internally. Let's
return int and keep track of (or look up again) the kernfs_node
internally.
Ok, it also makes the prototype of lock and unlock look more consistent
and comfortable.
When trying to do an new version of the patch, I find that if the device
is really removed, then we couldn't look up using the parent, and
attribute name again in unlock. So I guess maybe I could add one more
argument, e.g. kn_out,:q to track this kernfs node.
Code will be posted soon for your review.
Thanks, Zhong
{
...
+ /*
+ * We assume device_hotplug_lock must be acquired before removing
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
It seems not true ... I think there are devices that don't have the
online/offline concept, we just need to add it, remove it, like ethernet
cards.
Maybe we could change the comments above, like:
/* We assume device_hotplug_lock must be acquired before
* removing devices, which have online/offline sysfs knob,
* and some locks are needed to serialize the online/offline
* callbacks and device removing. ...
?
And we could add lockdep assertions in cpu and memory related code? e.g.
remove_memory(), unregister_cpu()
Currently, remove_memory() has comments for the function:
* NOTE: The caller must call lock_device_hotplug() to serialize hotplug
* and online/offline operations before this call, as required by
* try_offline_node().
*/
maybe it could be removed with the lockdep assertion.
...
Function comment please.
OK.
Thanks, Zhong
+void unlock_device_hotplug_sysfs(struct device *dev,
+ struct kernfs_node *kn)
+{
+ unlock_device_hotplug();
+ kernfs_unbreak_active_protection(kn);
+ put_device(dev);
+ kernfs_put(kn);
}
Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Fri, 2014-04-25 at 14:47 +0200, Rafael J. Wysocki wrote: > On 4/25/2014 3:46 AM, Li Zhong wrote: > > On Thu, 2014-04-24 at 12:02 +0200, Rafael J. Wysocki wrote: > >> On 4/24/2014 10:59 AM, Li Zhong wrote: > >>> On Wed, 2014-04-23 at 18:12 +0200, Rafael J. Wysocki wrote: > On 4/23/2014 4:23 PM, Tejun Heo wrote: > > Hello, Rafael. > Hi, > > > On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: > >> Can you please elaborate a bit? > > Because it can get involved in larger locking dependency issues by > > joining dependency graphs of two otherwise largely disjoint > > subsystems. It has potential to create possible deadlocks which don't > > need to exist. > Well, I do my best not to add unnecessary locks if that's what you mean. > > >> It is there to protect hotplug operations involving multiple devices > >> (in different subsystems) from racing with each other. Why exactly > >> is it bad? > > But why would different subsystems, say cpu and memory, use the same > > lock? Wouldn't those subsystems already have proper locking inside > > their own subsystems? > That locking is not sufficient. > > > Why add this additional global lock across multiple subsystems? > That basically is because of how eject works when it is triggered via > ACPI. > > It is signaled for a device at the top of a subtree. It may be a > container of some sort and the eject involves everything below that > device in the ACPI namespace. That may involve multiple subsystem > (CPUs, memory, PCI host bridge, etc.). > > We do that in two steps, offline (which can fail) and eject proper > (which can't fail and makes all of the involved devices go away). All > that has to be done in one go with respect to the sysfs-triggered > offline/online and that's why the lock is there. > >>> Thank you for the education. It's more clear to me now why we need this > >>> lock. > >>> > >>> I still have some small questions about when this lock is needed: > >>> > >>> I could understand sysfs-triggered online is not acceptable when > >>> removing devices in multiple subsystems. But maybe concurrent offline > >>> and remove(with proper per subsystem locks) seems not harmful? > >>> > >>> And if we just want to remove some devices in a specific subsystem, e.g. > >>> like writing /cpu/release, if it just wants to offline and remove some > >>> cpus, then maybe we don't require the device_hotplug_lock to be taken? > >> No and no. > >> > >> If the offline phase fails for any device in the subtree, we roll back > >> the operation > >> and online devices that have already been offlined by it. Also the ACPI > >> hot-addition > >> needs to acquire device_hotplug_lock so that it doesn't race with ejects > >> and so > >> that lock needs to be taken by sysfs-triggered offline too. > > I can understand that hot-addition needs the device_hotplug lock, but > > still not very clear about the offline. > > > > I guess your are describing following scenario: > > > > user A: (trying remove cpu 1 and memory 1-10) > > > > lock_device_hotplug > > offline cpu with cpu locks -- successful > > offline memories with memory locks -- failed, e.g. for memory8 > > online cpu and memory with their locks > > unlock_device_hotplug > > What about if all is successful and CPU1 is gone before > device_hotplug_lock is released? You mean user B will try to offline an already removed cpu1? But I think the cpu subsys locks should be able to handle such situation? > > > user B: (trying offline cpu 1) > > > > offline cpu with cpu locks > > > > But I don't see any problem for user B not taking the device_hotplug > > lock. The result may be different for user B to take or not take the > > lock. But I think it could be seen as concurrent online/offline for cpu1 > > under cpu hotplug locks, which just depends on which is executed last? > > > > Or did I miss something here? > > Yes, you could do offline in parallel with user A without taking > device_hotplug_lock, but the result may be surprising to user B then. > > With device _hotplug_lock user B will always see CPU1 off line (or gone) > after his offline in this scenario, while without taking the lock user B > may sometimes see CPU1 on line after his offline. I don't think that's > a good thing. That seems complicated after some more thinking. I think I missed something when describing the steps for A. I think the initial online/offline state needs to be recorded by offline operations in A, so the rollback could be done based on the initial state. If adding the above, then 1) B offline cpu 1 before A offline cpu 1 A could see the initial state of cpu1 as offline, and the rollback would not put cpu1 online again. In the code, I think the check is done at if (!cpu_online(cpu)) return -EINVAL; So the pn->put_online is kept as false.
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Fri, 2014-04-25 at 14:47 +0200, Rafael J. Wysocki wrote: On 4/25/2014 3:46 AM, Li Zhong wrote: On Thu, 2014-04-24 at 12:02 +0200, Rafael J. Wysocki wrote: On 4/24/2014 10:59 AM, Li Zhong wrote: On Wed, 2014-04-23 at 18:12 +0200, Rafael J. Wysocki wrote: On 4/23/2014 4:23 PM, Tejun Heo wrote: Hello, Rafael. Hi, On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. Well, I do my best not to add unnecessary locks if that's what you mean. It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? That locking is not sufficient. Why add this additional global lock across multiple subsystems? That basically is because of how eject works when it is triggered via ACPI. It is signaled for a device at the top of a subtree. It may be a container of some sort and the eject involves everything below that device in the ACPI namespace. That may involve multiple subsystem (CPUs, memory, PCI host bridge, etc.). We do that in two steps, offline (which can fail) and eject proper (which can't fail and makes all of the involved devices go away). All that has to be done in one go with respect to the sysfs-triggered offline/online and that's why the lock is there. Thank you for the education. It's more clear to me now why we need this lock. I still have some small questions about when this lock is needed: I could understand sysfs-triggered online is not acceptable when removing devices in multiple subsystems. But maybe concurrent offline and remove(with proper per subsystem locks) seems not harmful? And if we just want to remove some devices in a specific subsystem, e.g. like writing /cpu/release, if it just wants to offline and remove some cpus, then maybe we don't require the device_hotplug_lock to be taken? No and no. If the offline phase fails for any device in the subtree, we roll back the operation and online devices that have already been offlined by it. Also the ACPI hot-addition needs to acquire device_hotplug_lock so that it doesn't race with ejects and so that lock needs to be taken by sysfs-triggered offline too. I can understand that hot-addition needs the device_hotplug lock, but still not very clear about the offline. I guess your are describing following scenario: user A: (trying remove cpu 1 and memory 1-10) lock_device_hotplug offline cpu with cpu locks -- successful offline memories with memory locks -- failed, e.g. for memory8 online cpu and memory with their locks unlock_device_hotplug What about if all is successful and CPU1 is gone before device_hotplug_lock is released? You mean user B will try to offline an already removed cpu1? But I think the cpu subsys locks should be able to handle such situation? user B: (trying offline cpu 1) offline cpu with cpu locks But I don't see any problem for user B not taking the device_hotplug lock. The result may be different for user B to take or not take the lock. But I think it could be seen as concurrent online/offline for cpu1 under cpu hotplug locks, which just depends on which is executed last? Or did I miss something here? Yes, you could do offline in parallel with user A without taking device_hotplug_lock, but the result may be surprising to user B then. With device _hotplug_lock user B will always see CPU1 off line (or gone) after his offline in this scenario, while without taking the lock user B may sometimes see CPU1 on line after his offline. I don't think that's a good thing. That seems complicated after some more thinking. I think I missed something when describing the steps for A. I think the initial online/offline state needs to be recorded by offline operations in A, so the rollback could be done based on the initial state. If adding the above, then 1) B offline cpu 1 before A offline cpu 1 A could see the initial state of cpu1 as offline, and the rollback would not put cpu1 online again. In the code, I think the check is done at if (!cpu_online(cpu)) return -EINVAL; So the pn-put_online is kept as false. So the result is cpu1 offline. 2) B offline cpu 1 after A offline cpu1 then the rollback would online cpu1 2.1) B offline cpu1 after A rollback The result is cpu1 offline, good. 2.2) B offline cpu1 before A rollback B would see a -EINVAL error, and the result is cpu1 online. I guess this is the case you mentioned.
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On 4/25/2014 3:46 AM, Li Zhong wrote: On Thu, 2014-04-24 at 12:02 +0200, Rafael J. Wysocki wrote: On 4/24/2014 10:59 AM, Li Zhong wrote: On Wed, 2014-04-23 at 18:12 +0200, Rafael J. Wysocki wrote: On 4/23/2014 4:23 PM, Tejun Heo wrote: Hello, Rafael. Hi, On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. Well, I do my best not to add unnecessary locks if that's what you mean. It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? That locking is not sufficient. Why add this additional global lock across multiple subsystems? That basically is because of how eject works when it is triggered via ACPI. It is signaled for a device at the top of a subtree. It may be a container of some sort and the eject involves everything below that device in the ACPI namespace. That may involve multiple subsystem (CPUs, memory, PCI host bridge, etc.). We do that in two steps, offline (which can fail) and eject proper (which can't fail and makes all of the involved devices go away). All that has to be done in one go with respect to the sysfs-triggered offline/online and that's why the lock is there. Thank you for the education. It's more clear to me now why we need this lock. I still have some small questions about when this lock is needed: I could understand sysfs-triggered online is not acceptable when removing devices in multiple subsystems. But maybe concurrent offline and remove(with proper per subsystem locks) seems not harmful? And if we just want to remove some devices in a specific subsystem, e.g. like writing /cpu/release, if it just wants to offline and remove some cpus, then maybe we don't require the device_hotplug_lock to be taken? No and no. If the offline phase fails for any device in the subtree, we roll back the operation and online devices that have already been offlined by it. Also the ACPI hot-addition needs to acquire device_hotplug_lock so that it doesn't race with ejects and so that lock needs to be taken by sysfs-triggered offline too. I can understand that hot-addition needs the device_hotplug lock, but still not very clear about the offline. I guess your are describing following scenario: user A: (trying remove cpu 1 and memory 1-10) lock_device_hotplug offline cpu with cpu locks -- successful offline memories with memory locks -- failed, e.g. for memory8 online cpu and memory with their locks unlock_device_hotplug What about if all is successful and CPU1 is gone before device_hotplug_lock is released? user B: (trying offline cpu 1) offline cpu with cpu locks But I don't see any problem for user B not taking the device_hotplug lock. The result may be different for user B to take or not take the lock. But I think it could be seen as concurrent online/offline for cpu1 under cpu hotplug locks, which just depends on which is executed last? Or did I miss something here? Yes, you could do offline in parallel with user A without taking device_hotplug_lock, but the result may be surprising to user B then. With device _hotplug_lock user B will always see CPU1 off line (or gone) after his offline in this scenario, while without taking the lock user B may sometimes see CPU1 on line after his offline. I don't think that's a good thing. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On 4/25/2014 3:46 AM, Li Zhong wrote: On Thu, 2014-04-24 at 12:02 +0200, Rafael J. Wysocki wrote: On 4/24/2014 10:59 AM, Li Zhong wrote: On Wed, 2014-04-23 at 18:12 +0200, Rafael J. Wysocki wrote: On 4/23/2014 4:23 PM, Tejun Heo wrote: Hello, Rafael. Hi, On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. Well, I do my best not to add unnecessary locks if that's what you mean. It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? That locking is not sufficient. Why add this additional global lock across multiple subsystems? That basically is because of how eject works when it is triggered via ACPI. It is signaled for a device at the top of a subtree. It may be a container of some sort and the eject involves everything below that device in the ACPI namespace. That may involve multiple subsystem (CPUs, memory, PCI host bridge, etc.). We do that in two steps, offline (which can fail) and eject proper (which can't fail and makes all of the involved devices go away). All that has to be done in one go with respect to the sysfs-triggered offline/online and that's why the lock is there. Thank you for the education. It's more clear to me now why we need this lock. I still have some small questions about when this lock is needed: I could understand sysfs-triggered online is not acceptable when removing devices in multiple subsystems. But maybe concurrent offline and remove(with proper per subsystem locks) seems not harmful? And if we just want to remove some devices in a specific subsystem, e.g. like writing /cpu/release, if it just wants to offline and remove some cpus, then maybe we don't require the device_hotplug_lock to be taken? No and no. If the offline phase fails for any device in the subtree, we roll back the operation and online devices that have already been offlined by it. Also the ACPI hot-addition needs to acquire device_hotplug_lock so that it doesn't race with ejects and so that lock needs to be taken by sysfs-triggered offline too. I can understand that hot-addition needs the device_hotplug lock, but still not very clear about the offline. I guess your are describing following scenario: user A: (trying remove cpu 1 and memory 1-10) lock_device_hotplug offline cpu with cpu locks -- successful offline memories with memory locks -- failed, e.g. for memory8 online cpu and memory with their locks unlock_device_hotplug What about if all is successful and CPU1 is gone before device_hotplug_lock is released? user B: (trying offline cpu 1) offline cpu with cpu locks But I don't see any problem for user B not taking the device_hotplug lock. The result may be different for user B to take or not take the lock. But I think it could be seen as concurrent online/offline for cpu1 under cpu hotplug locks, which just depends on which is executed last? Or did I miss something here? Yes, you could do offline in parallel with user A without taking device_hotplug_lock, but the result may be surprising to user B then. With device _hotplug_lock user B will always see CPU1 off line (or gone) after his offline in this scenario, while without taking the lock user B may sometimes see CPU1 on line after his offline. I don't think that's a good thing. Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Thu, 2014-04-24 at 12:02 +0200, Rafael J. Wysocki wrote: > On 4/24/2014 10:59 AM, Li Zhong wrote: > > On Wed, 2014-04-23 at 18:12 +0200, Rafael J. Wysocki wrote: > >> On 4/23/2014 4:23 PM, Tejun Heo wrote: > >>> Hello, Rafael. > >> Hi, > >> > >>> On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: > Can you please elaborate a bit? > >>> Because it can get involved in larger locking dependency issues by > >>> joining dependency graphs of two otherwise largely disjoint > >>> subsystems. It has potential to create possible deadlocks which don't > >>> need to exist. > >> Well, I do my best not to add unnecessary locks if that's what you mean. > >> > It is there to protect hotplug operations involving multiple devices > (in different subsystems) from racing with each other. Why exactly > is it bad? > >>> But why would different subsystems, say cpu and memory, use the same > >>> lock? Wouldn't those subsystems already have proper locking inside > >>> their own subsystems? > >> That locking is not sufficient. > >> > >>> Why add this additional global lock across multiple subsystems? > >> That basically is because of how eject works when it is triggered via ACPI. > >> > >> It is signaled for a device at the top of a subtree. It may be a > >> container of some sort and the eject involves everything below that > >> device in the ACPI namespace. That may involve multiple subsystem > >> (CPUs, memory, PCI host bridge, etc.). > >> > >> We do that in two steps, offline (which can fail) and eject proper > >> (which can't fail and makes all of the involved devices go away). All > >> that has to be done in one go with respect to the sysfs-triggered > >> offline/online and that's why the lock is there. > > Thank you for the education. It's more clear to me now why we need this > > lock. > > > > I still have some small questions about when this lock is needed: > > > > I could understand sysfs-triggered online is not acceptable when > > removing devices in multiple subsystems. But maybe concurrent offline > > and remove(with proper per subsystem locks) seems not harmful? > > > > And if we just want to remove some devices in a specific subsystem, e.g. > > like writing /cpu/release, if it just wants to offline and remove some > > cpus, then maybe we don't require the device_hotplug_lock to be taken? > > No and no. > > If the offline phase fails for any device in the subtree, we roll back > the operation > and online devices that have already been offlined by it. Also the ACPI > hot-addition > needs to acquire device_hotplug_lock so that it doesn't race with ejects > and so > that lock needs to be taken by sysfs-triggered offline too. I can understand that hot-addition needs the device_hotplug lock, but still not very clear about the offline. I guess your are describing following scenario: user A: (trying remove cpu 1 and memory 1-10) lock_device_hotplug offline cpu with cpu locks -- successful offline memories with memory locks -- failed, e.g. for memory8 online cpu and memory with their locks unlock_device_hotplug user B: (trying offline cpu 1) offline cpu with cpu locks But I don't see any problem for user B not taking the device_hotplug lock. The result may be different for user B to take or not take the lock. But I think it could be seen as concurrent online/offline for cpu1 under cpu hotplug locks, which just depends on which is executed last? Or did I miss something here? Thanks, Zhong > Thanks, > Rafael > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On 4/24/2014 10:59 AM, Li Zhong wrote: On Wed, 2014-04-23 at 18:12 +0200, Rafael J. Wysocki wrote: On 4/23/2014 4:23 PM, Tejun Heo wrote: Hello, Rafael. Hi, On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. Well, I do my best not to add unnecessary locks if that's what you mean. It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? That locking is not sufficient. Why add this additional global lock across multiple subsystems? That basically is because of how eject works when it is triggered via ACPI. It is signaled for a device at the top of a subtree. It may be a container of some sort and the eject involves everything below that device in the ACPI namespace. That may involve multiple subsystem (CPUs, memory, PCI host bridge, etc.). We do that in two steps, offline (which can fail) and eject proper (which can't fail and makes all of the involved devices go away). All that has to be done in one go with respect to the sysfs-triggered offline/online and that's why the lock is there. Thank you for the education. It's more clear to me now why we need this lock. I still have some small questions about when this lock is needed: I could understand sysfs-triggered online is not acceptable when removing devices in multiple subsystems. But maybe concurrent offline and remove(with proper per subsystem locks) seems not harmful? And if we just want to remove some devices in a specific subsystem, e.g. like writing /cpu/release, if it just wants to offline and remove some cpus, then maybe we don't require the device_hotplug_lock to be taken? No and no. If the offline phase fails for any device in the subtree, we roll back the operation and online devices that have already been offlined by it. Also the ACPI hot-addition needs to acquire device_hotplug_lock so that it doesn't race with ejects and so that lock needs to be taken by sysfs-triggered offline too. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Wed, 2014-04-23 at 18:12 +0200, Rafael J. Wysocki wrote: > On 4/23/2014 4:23 PM, Tejun Heo wrote: > > Hello, Rafael. > > Hi, > > > On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: > >> Can you please elaborate a bit? > > Because it can get involved in larger locking dependency issues by > > joining dependency graphs of two otherwise largely disjoint > > subsystems. It has potential to create possible deadlocks which don't > > need to exist. > > Well, I do my best not to add unnecessary locks if that's what you mean. > > >> It is there to protect hotplug operations involving multiple devices > >> (in different subsystems) from racing with each other. Why exactly > >> is it bad? > > But why would different subsystems, say cpu and memory, use the same > > lock? Wouldn't those subsystems already have proper locking inside > > their own subsystems? > > That locking is not sufficient. > > > Why add this additional global lock across multiple subsystems? > > That basically is because of how eject works when it is triggered via ACPI. > > It is signaled for a device at the top of a subtree. It may be a > container of some sort and the eject involves everything below that > device in the ACPI namespace. That may involve multiple subsystem > (CPUs, memory, PCI host bridge, etc.). > > We do that in two steps, offline (which can fail) and eject proper > (which can't fail and makes all of the involved devices go away). All > that has to be done in one go with respect to the sysfs-triggered > offline/online and that's why the lock is there. Thank you for the education. It's more clear to me now why we need this lock. I still have some small questions about when this lock is needed: I could understand sysfs-triggered online is not acceptable when removing devices in multiple subsystems. But maybe concurrent offline and remove(with proper per subsystem locks) seems not harmful? And if we just want to remove some devices in a specific subsystem, e.g. like writing /cpu/release, if it just wants to offline and remove some cpus, then maybe we don't require the device_hotplug_lock to be taken? Thanks, Zhong > > Thanks, > Rafael > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Wed, 2014-04-23 at 18:12 +0200, Rafael J. Wysocki wrote: On 4/23/2014 4:23 PM, Tejun Heo wrote: Hello, Rafael. Hi, On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. Well, I do my best not to add unnecessary locks if that's what you mean. It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? That locking is not sufficient. Why add this additional global lock across multiple subsystems? That basically is because of how eject works when it is triggered via ACPI. It is signaled for a device at the top of a subtree. It may be a container of some sort and the eject involves everything below that device in the ACPI namespace. That may involve multiple subsystem (CPUs, memory, PCI host bridge, etc.). We do that in two steps, offline (which can fail) and eject proper (which can't fail and makes all of the involved devices go away). All that has to be done in one go with respect to the sysfs-triggered offline/online and that's why the lock is there. Thank you for the education. It's more clear to me now why we need this lock. I still have some small questions about when this lock is needed: I could understand sysfs-triggered online is not acceptable when removing devices in multiple subsystems. But maybe concurrent offline and remove(with proper per subsystem locks) seems not harmful? And if we just want to remove some devices in a specific subsystem, e.g. like writing /cpu/release, if it just wants to offline and remove some cpus, then maybe we don't require the device_hotplug_lock to be taken? Thanks, Zhong Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On 4/24/2014 10:59 AM, Li Zhong wrote: On Wed, 2014-04-23 at 18:12 +0200, Rafael J. Wysocki wrote: On 4/23/2014 4:23 PM, Tejun Heo wrote: Hello, Rafael. Hi, On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. Well, I do my best not to add unnecessary locks if that's what you mean. It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? That locking is not sufficient. Why add this additional global lock across multiple subsystems? That basically is because of how eject works when it is triggered via ACPI. It is signaled for a device at the top of a subtree. It may be a container of some sort and the eject involves everything below that device in the ACPI namespace. That may involve multiple subsystem (CPUs, memory, PCI host bridge, etc.). We do that in two steps, offline (which can fail) and eject proper (which can't fail and makes all of the involved devices go away). All that has to be done in one go with respect to the sysfs-triggered offline/online and that's why the lock is there. Thank you for the education. It's more clear to me now why we need this lock. I still have some small questions about when this lock is needed: I could understand sysfs-triggered online is not acceptable when removing devices in multiple subsystems. But maybe concurrent offline and remove(with proper per subsystem locks) seems not harmful? And if we just want to remove some devices in a specific subsystem, e.g. like writing /cpu/release, if it just wants to offline and remove some cpus, then maybe we don't require the device_hotplug_lock to be taken? No and no. If the offline phase fails for any device in the subtree, we roll back the operation and online devices that have already been offlined by it. Also the ACPI hot-addition needs to acquire device_hotplug_lock so that it doesn't race with ejects and so that lock needs to be taken by sysfs-triggered offline too. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Thu, 2014-04-24 at 12:02 +0200, Rafael J. Wysocki wrote: On 4/24/2014 10:59 AM, Li Zhong wrote: On Wed, 2014-04-23 at 18:12 +0200, Rafael J. Wysocki wrote: On 4/23/2014 4:23 PM, Tejun Heo wrote: Hello, Rafael. Hi, On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. Well, I do my best not to add unnecessary locks if that's what you mean. It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? That locking is not sufficient. Why add this additional global lock across multiple subsystems? That basically is because of how eject works when it is triggered via ACPI. It is signaled for a device at the top of a subtree. It may be a container of some sort and the eject involves everything below that device in the ACPI namespace. That may involve multiple subsystem (CPUs, memory, PCI host bridge, etc.). We do that in two steps, offline (which can fail) and eject proper (which can't fail and makes all of the involved devices go away). All that has to be done in one go with respect to the sysfs-triggered offline/online and that's why the lock is there. Thank you for the education. It's more clear to me now why we need this lock. I still have some small questions about when this lock is needed: I could understand sysfs-triggered online is not acceptable when removing devices in multiple subsystems. But maybe concurrent offline and remove(with proper per subsystem locks) seems not harmful? And if we just want to remove some devices in a specific subsystem, e.g. like writing /cpu/release, if it just wants to offline and remove some cpus, then maybe we don't require the device_hotplug_lock to be taken? No and no. If the offline phase fails for any device in the subtree, we roll back the operation and online devices that have already been offlined by it. Also the ACPI hot-addition needs to acquire device_hotplug_lock so that it doesn't race with ejects and so that lock needs to be taken by sysfs-triggered offline too. I can understand that hot-addition needs the device_hotplug lock, but still not very clear about the offline. I guess your are describing following scenario: user A: (trying remove cpu 1 and memory 1-10) lock_device_hotplug offline cpu with cpu locks -- successful offline memories with memory locks -- failed, e.g. for memory8 online cpu and memory with their locks unlock_device_hotplug user B: (trying offline cpu 1) offline cpu with cpu locks But I don't see any problem for user B not taking the device_hotplug lock. The result may be different for user B to take or not take the lock. But I think it could be seen as concurrent online/offline for cpu1 under cpu hotplug locks, which just depends on which is executed last? Or did I miss something here? Thanks, Zhong Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Wed, 2014-04-23 at 12:58 +0200, Rafael J. Wysocki wrote:
> On Wednesday, April 23, 2014 01:03:42 PM Li Zhong wrote:
> > On Tue, 2014-04-22 at 16:44 -0400, Tejun Heo wrote:
> > > Hello,
> > >
> > > On Tue, Apr 22, 2014 at 11:34:39AM +0800, Li Zhong wrote:
> > > > > Is this assumption true? If so, can we add lockdep assertions in
> > > > > places to verify and enforce this? If not, aren't we just feeling
> > > > > good when the reality is broken?
> > > >
> > > > It seems not true ... I think there are devices that don't have the
> > > > online/offline concept, we just need to add it, remove it, like ethernet
> > > > cards.
> > > >
> > > > Maybe we could change the comments above, like:
> > > > /* We assume device_hotplug_lock must be acquired before
> > > > * removing devices, which have online/offline sysfs knob,
> > > > * and some locks are needed to serialize the online/offline
> > > > * callbacks and device removing. ...
> > > > ?
> > > >
> > > > And we could add lockdep assertions in cpu and memory related code? e.g.
> > > > remove_memory(), unregister_cpu()
> > > >
> > > > Currently, remove_memory() has comments for the function:
> > > >
> > > > * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> > > > * and online/offline operations before this call, as required by
> > > > * try_offline_node().
> > > > */
> > > >
> > > > maybe it could be removed with the lockdep assertion.
> > >
> > > I'm confused about the overall locking scheme. What's the role of
> > > device_hotplug_lock? Is that solely to prevent the sysfs deadlock
> > > issue? Or does it serve other synchronization purposes depending on
> > > the specific subsystem? If the former, the lock no longer needs to
> > > exist. The only thing necessary would be synchronization between
> > > device_del() deleting the sysfs file and the unbreak helper invoking
> > > device-specific callback. If the latter, we probably should change
> > > that. Sharing hotplug lock across multiple subsystems through driver
> > > core sounds like a pretty bad idea.
> >
> > I think it's the latter.
>
> Actually, no, this is not the case if I understand you correctly.
Oh, Sorry, I didn't read carefully. Yes, it's not specific subsystem.
After seeing your reply, I understand it is for protecting device hot
remove involving multiple subsystems.
>
> > I think device_{on|off}line is better to be
> > done in some sort of lock which prevents the device from being removed,
> > including some preparation work that needs be done before device_del().
>
> Quite frankly, you should be confident that you understand the code you're
> trying to modify or please don't touch it.
>
> I'll have a deeper look at this issue later today or tomorrow and will get
> back to you then.
Ok, thank you,
Zhong
>
> Thanks!
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Wed, 2014-04-23 at 12:54 +0200, Rafael J. Wysocki wrote:
> On Wednesday, April 23, 2014 09:50:32 AM Li Zhong wrote:
> > On Tue, 2014-04-22 at 12:11 +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, April 22, 2014 11:34:39 AM Li Zhong wrote:
> > > > On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
> > > > > Hello,
> > > > >
> > > > > On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
> > > > >
> > > > > Proper /** function comment would be nice.
> > > >
> > > > Ok, will try to write some in next version.
> > > >
> > > > >
> > > > > > +struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
> > > > > > + struct device_attribute
> > > > > > *attr)
> > > > >
> > > > > I can see why you did this but let's please not require the user of
> > > > > this function to see how the thing is working internally. Let's
> > > > > return int and keep track of (or look up again) the kernfs_node
> > > > > internally.
> > > >
> > > > Ok, it also makes the prototype of lock and unlock look more consistent
> > > > and comfortable.
> > > >
> > > > >
> > > > > > {
> > > > > ...
> > > > > > + /*
> > > > > > +* We assume device_hotplug_lock must be acquired before
> > > > > > removing
> > > > >
> > > > > Is this assumption true? If so, can we add lockdep assertions in
> > > > > places to verify and enforce this? If not, aren't we just feeling
> > > > > good when the reality is broken?
> > > >
> > > > It seems not true ... I think there are devices that don't have the
> > > > online/offline concept, we just need to add it, remove it, like ethernet
> > > > cards.
> > >
> > > Well, I haven't been following this closely (I was travelling, sorry), but
> > > there certainly are devices without online/offline. That currently is
> > > only
> > > present for CPUs, memory blocks and ACPI containers (if I remember
> > > correctly).
> > >
> > > >
> > > > Maybe we could change the comments above, like:
> > > > /* We assume device_hotplug_lock must be acquired before
> > > > * removing devices, which have online/offline sysfs knob,
> > > > * and some locks are needed to serialize the online/offline
> > > > * callbacks and device removing. ...
> > > > ?
> > >
> > > Lockdep assertions would be better than this in my opinion.
> >
> > This is talking about the lock required in the other process, the device
> > removing process, e.g. that in remove_memory() below. So I guess no
> > lockdep assertions needed here. Or I misunderstand your point?
>
> I mean if you assume certain lock to be held somewhere, it is better to use
> lockdep annotations to express that assumption, because that will cause users
> to *see* the problem when it happens.
OK, I see, I think you were suggesting the same thing as Tejun, just I
misunderstood it.
Thanks, Zhong
>
> Thanks!
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
Hello, Rafael. On Wed, Apr 23, 2014 at 06:12:01PM +0200, Rafael J. Wysocki wrote: > >Why add this additional global lock across multiple subsystems? > > That basically is because of how eject works when it is triggered via ACPI. > > It is signaled for a device at the top of a subtree. It may be a > container of some sort and the eject involves everything below that > device in the ACPI namespace. That may involve multiple subsystem > (CPUs, memory, PCI host bridge, etc.). > > We do that in two steps, offline (which can fail) and eject proper > (which can't fail and makes all of the involved devices go away). > All that has to be done in one go with respect to the > sysfs-triggered offline/online and that's why the lock is there. Ah, okay, so it's actually synchronzing across multiple subsystems. I think it definitely calls for more documentation. The requirement is pretty unusual after all. Thanks! -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On 4/23/2014 4:23 PM, Tejun Heo wrote: Hello, Rafael. Hi, On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. Well, I do my best not to add unnecessary locks if that's what you mean. It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? That locking is not sufficient. Why add this additional global lock across multiple subsystems? That basically is because of how eject works when it is triggered via ACPI. It is signaled for a device at the top of a subtree. It may be a container of some sort and the eject involves everything below that device in the ACPI namespace. That may involve multiple subsystem (CPUs, memory, PCI host bridge, etc.). We do that in two steps, offline (which can fail) and eject proper (which can't fail and makes all of the involved devices go away). All that has to be done in one go with respect to the sysfs-triggered offline/online and that's why the lock is there. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
Hello, Rafael. On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: > Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. > It is there to protect hotplug operations involving multiple devices > (in different subsystems) from racing with each other. Why exactly > is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? Why add this additional global lock across multiple subsystems? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Wednesday, April 23, 2014 01:03:42 PM Li Zhong wrote:
> On Tue, 2014-04-22 at 16:44 -0400, Tejun Heo wrote:
> > Hello,
> >
> > On Tue, Apr 22, 2014 at 11:34:39AM +0800, Li Zhong wrote:
> > > > Is this assumption true? If so, can we add lockdep assertions in
> > > > places to verify and enforce this? If not, aren't we just feeling
> > > > good when the reality is broken?
> > >
> > > It seems not true ... I think there are devices that don't have the
> > > online/offline concept, we just need to add it, remove it, like ethernet
> > > cards.
> > >
> > > Maybe we could change the comments above, like:
> > > /* We assume device_hotplug_lock must be acquired before
> > >* removing devices, which have online/offline sysfs knob,
> > >* and some locks are needed to serialize the online/offline
> > >* callbacks and device removing. ...
> > > ?
> > >
> > > And we could add lockdep assertions in cpu and memory related code? e.g.
> > > remove_memory(), unregister_cpu()
> > >
> > > Currently, remove_memory() has comments for the function:
> > >
> > > * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> > > * and online/offline operations before this call, as required by
> > > * try_offline_node().
> > > */
> > >
> > > maybe it could be removed with the lockdep assertion.
> >
> > I'm confused about the overall locking scheme. What's the role of
> > device_hotplug_lock? Is that solely to prevent the sysfs deadlock
> > issue? Or does it serve other synchronization purposes depending on
> > the specific subsystem? If the former, the lock no longer needs to
> > exist. The only thing necessary would be synchronization between
> > device_del() deleting the sysfs file and the unbreak helper invoking
> > device-specific callback. If the latter, we probably should change
> > that. Sharing hotplug lock across multiple subsystems through driver
> > core sounds like a pretty bad idea.
>
> I think it's the latter.
Actually, no, this is not the case if I understand you correctly.
> I think device_{on|off}line is better to be
> done in some sort of lock which prevents the device from being removed,
> including some preparation work that needs be done before device_del().
Quite frankly, you should be confident that you understand the code you're
trying to modify or please don't touch it.
I'll have a deeper look at this issue later today or tomorrow and will get
back to you then.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Wednesday, April 23, 2014 09:50:32 AM Li Zhong wrote:
> On Tue, 2014-04-22 at 12:11 +0200, Rafael J. Wysocki wrote:
> > On Tuesday, April 22, 2014 11:34:39 AM Li Zhong wrote:
> > > On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
> > > > Hello,
> > > >
> > > > On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
> > > >
> > > > Proper /** function comment would be nice.
> > >
> > > Ok, will try to write some in next version.
> > >
> > > >
> > > > > +struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
> > > > > +struct device_attribute
> > > > > *attr)
> > > >
> > > > I can see why you did this but let's please not require the user of
> > > > this function to see how the thing is working internally. Let's
> > > > return int and keep track of (or look up again) the kernfs_node
> > > > internally.
> > >
> > > Ok, it also makes the prototype of lock and unlock look more consistent
> > > and comfortable.
> > >
> > > >
> > > > > {
> > > > ...
> > > > > + /*
> > > > > + * We assume device_hotplug_lock must be acquired before
> > > > > removing
> > > >
> > > > Is this assumption true? If so, can we add lockdep assertions in
> > > > places to verify and enforce this? If not, aren't we just feeling
> > > > good when the reality is broken?
> > >
> > > It seems not true ... I think there are devices that don't have the
> > > online/offline concept, we just need to add it, remove it, like ethernet
> > > cards.
> >
> > Well, I haven't been following this closely (I was travelling, sorry), but
> > there certainly are devices without online/offline. That currently is only
> > present for CPUs, memory blocks and ACPI containers (if I remember
> > correctly).
> >
> > >
> > > Maybe we could change the comments above, like:
> > > /* We assume device_hotplug_lock must be acquired before
> > >* removing devices, which have online/offline sysfs knob,
> > >* and some locks are needed to serialize the online/offline
> > >* callbacks and device removing. ...
> > > ?
> >
> > Lockdep assertions would be better than this in my opinion.
>
> This is talking about the lock required in the other process, the device
> removing process, e.g. that in remove_memory() below. So I guess no
> lockdep assertions needed here. Or I misunderstand your point?
I mean if you assume certain lock to be held somewhere, it is better to use
lockdep annotations to express that assumption, because that will cause users
to *see* the problem when it happens.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Wed, 2014-04-23 at 12:54 +0200, Rafael J. Wysocki wrote:
On Wednesday, April 23, 2014 09:50:32 AM Li Zhong wrote:
On Tue, 2014-04-22 at 12:11 +0200, Rafael J. Wysocki wrote:
On Tuesday, April 22, 2014 11:34:39 AM Li Zhong wrote:
On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
Hello,
On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
Proper /** function comment would be nice.
Ok, will try to write some in next version.
+struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
+ struct device_attribute
*attr)
I can see why you did this but let's please not require the user of
this function to see how the thing is working internally. Let's
return int and keep track of (or look up again) the kernfs_node
internally.
Ok, it also makes the prototype of lock and unlock look more consistent
and comfortable.
{
...
+ /*
+* We assume device_hotplug_lock must be acquired before
removing
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
It seems not true ... I think there are devices that don't have the
online/offline concept, we just need to add it, remove it, like ethernet
cards.
Well, I haven't been following this closely (I was travelling, sorry), but
there certainly are devices without online/offline. That currently is
only
present for CPUs, memory blocks and ACPI containers (if I remember
correctly).
Maybe we could change the comments above, like:
/* We assume device_hotplug_lock must be acquired before
* removing devices, which have online/offline sysfs knob,
* and some locks are needed to serialize the online/offline
* callbacks and device removing. ...
?
Lockdep assertions would be better than this in my opinion.
This is talking about the lock required in the other process, the device
removing process, e.g. that in remove_memory() below. So I guess no
lockdep assertions needed here. Or I misunderstand your point?
I mean if you assume certain lock to be held somewhere, it is better to use
lockdep annotations to express that assumption, because that will cause users
to *see* the problem when it happens.
OK, I see, I think you were suggesting the same thing as Tejun, just I
misunderstood it.
Thanks, Zhong
Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Wed, 2014-04-23 at 12:58 +0200, Rafael J. Wysocki wrote:
On Wednesday, April 23, 2014 01:03:42 PM Li Zhong wrote:
On Tue, 2014-04-22 at 16:44 -0400, Tejun Heo wrote:
Hello,
On Tue, Apr 22, 2014 at 11:34:39AM +0800, Li Zhong wrote:
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
It seems not true ... I think there are devices that don't have the
online/offline concept, we just need to add it, remove it, like ethernet
cards.
Maybe we could change the comments above, like:
/* We assume device_hotplug_lock must be acquired before
* removing devices, which have online/offline sysfs knob,
* and some locks are needed to serialize the online/offline
* callbacks and device removing. ...
?
And we could add lockdep assertions in cpu and memory related code? e.g.
remove_memory(), unregister_cpu()
Currently, remove_memory() has comments for the function:
* NOTE: The caller must call lock_device_hotplug() to serialize hotplug
* and online/offline operations before this call, as required by
* try_offline_node().
*/
maybe it could be removed with the lockdep assertion.
I'm confused about the overall locking scheme. What's the role of
device_hotplug_lock? Is that solely to prevent the sysfs deadlock
issue? Or does it serve other synchronization purposes depending on
the specific subsystem? If the former, the lock no longer needs to
exist. The only thing necessary would be synchronization between
device_del() deleting the sysfs file and the unbreak helper invoking
device-specific callback. If the latter, we probably should change
that. Sharing hotplug lock across multiple subsystems through driver
core sounds like a pretty bad idea.
I think it's the latter.
Actually, no, this is not the case if I understand you correctly.
Oh, Sorry, I didn't read carefully. Yes, it's not specific subsystem.
After seeing your reply, I understand it is for protecting device hot
remove involving multiple subsystems.
I think device_{on|off}line is better to be
done in some sort of lock which prevents the device from being removed,
including some preparation work that needs be done before device_del().
Quite frankly, you should be confident that you understand the code you're
trying to modify or please don't touch it.
I'll have a deeper look at this issue later today or tomorrow and will get
back to you then.
Ok, thank you,
Zhong
Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Wednesday, April 23, 2014 09:50:32 AM Li Zhong wrote:
On Tue, 2014-04-22 at 12:11 +0200, Rafael J. Wysocki wrote:
On Tuesday, April 22, 2014 11:34:39 AM Li Zhong wrote:
On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
Hello,
On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
Proper /** function comment would be nice.
Ok, will try to write some in next version.
+struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
+struct device_attribute
*attr)
I can see why you did this but let's please not require the user of
this function to see how the thing is working internally. Let's
return int and keep track of (or look up again) the kernfs_node
internally.
Ok, it also makes the prototype of lock and unlock look more consistent
and comfortable.
{
...
+ /*
+ * We assume device_hotplug_lock must be acquired before
removing
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
It seems not true ... I think there are devices that don't have the
online/offline concept, we just need to add it, remove it, like ethernet
cards.
Well, I haven't been following this closely (I was travelling, sorry), but
there certainly are devices without online/offline. That currently is only
present for CPUs, memory blocks and ACPI containers (if I remember
correctly).
Maybe we could change the comments above, like:
/* We assume device_hotplug_lock must be acquired before
* removing devices, which have online/offline sysfs knob,
* and some locks are needed to serialize the online/offline
* callbacks and device removing. ...
?
Lockdep assertions would be better than this in my opinion.
This is talking about the lock required in the other process, the device
removing process, e.g. that in remove_memory() below. So I guess no
lockdep assertions needed here. Or I misunderstand your point?
I mean if you assume certain lock to be held somewhere, it is better to use
lockdep annotations to express that assumption, because that will cause users
to *see* the problem when it happens.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Wednesday, April 23, 2014 01:03:42 PM Li Zhong wrote:
On Tue, 2014-04-22 at 16:44 -0400, Tejun Heo wrote:
Hello,
On Tue, Apr 22, 2014 at 11:34:39AM +0800, Li Zhong wrote:
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
It seems not true ... I think there are devices that don't have the
online/offline concept, we just need to add it, remove it, like ethernet
cards.
Maybe we could change the comments above, like:
/* We assume device_hotplug_lock must be acquired before
* removing devices, which have online/offline sysfs knob,
* and some locks are needed to serialize the online/offline
* callbacks and device removing. ...
?
And we could add lockdep assertions in cpu and memory related code? e.g.
remove_memory(), unregister_cpu()
Currently, remove_memory() has comments for the function:
* NOTE: The caller must call lock_device_hotplug() to serialize hotplug
* and online/offline operations before this call, as required by
* try_offline_node().
*/
maybe it could be removed with the lockdep assertion.
I'm confused about the overall locking scheme. What's the role of
device_hotplug_lock? Is that solely to prevent the sysfs deadlock
issue? Or does it serve other synchronization purposes depending on
the specific subsystem? If the former, the lock no longer needs to
exist. The only thing necessary would be synchronization between
device_del() deleting the sysfs file and the unbreak helper invoking
device-specific callback. If the latter, we probably should change
that. Sharing hotplug lock across multiple subsystems through driver
core sounds like a pretty bad idea.
I think it's the latter.
Actually, no, this is not the case if I understand you correctly.
I think device_{on|off}line is better to be
done in some sort of lock which prevents the device from being removed,
including some preparation work that needs be done before device_del().
Quite frankly, you should be confident that you understand the code you're
trying to modify or please don't touch it.
I'll have a deeper look at this issue later today or tomorrow and will get
back to you then.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
Hello, Rafael. On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? Why add this additional global lock across multiple subsystems? Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On 4/23/2014 4:23 PM, Tejun Heo wrote: Hello, Rafael. Hi, On Wed, Apr 23, 2014 at 12:21:33AM +0200, Rafael J. Wysocki wrote: Can you please elaborate a bit? Because it can get involved in larger locking dependency issues by joining dependency graphs of two otherwise largely disjoint subsystems. It has potential to create possible deadlocks which don't need to exist. Well, I do my best not to add unnecessary locks if that's what you mean. It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? But why would different subsystems, say cpu and memory, use the same lock? Wouldn't those subsystems already have proper locking inside their own subsystems? That locking is not sufficient. Why add this additional global lock across multiple subsystems? That basically is because of how eject works when it is triggered via ACPI. It is signaled for a device at the top of a subtree. It may be a container of some sort and the eject involves everything below that device in the ACPI namespace. That may involve multiple subsystem (CPUs, memory, PCI host bridge, etc.). We do that in two steps, offline (which can fail) and eject proper (which can't fail and makes all of the involved devices go away). All that has to be done in one go with respect to the sysfs-triggered offline/online and that's why the lock is there. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
Hello, Rafael. On Wed, Apr 23, 2014 at 06:12:01PM +0200, Rafael J. Wysocki wrote: Why add this additional global lock across multiple subsystems? That basically is because of how eject works when it is triggered via ACPI. It is signaled for a device at the top of a subtree. It may be a container of some sort and the eject involves everything below that device in the ACPI namespace. That may involve multiple subsystem (CPUs, memory, PCI host bridge, etc.). We do that in two steps, offline (which can fail) and eject proper (which can't fail and makes all of the involved devices go away). All that has to be done in one go with respect to the sysfs-triggered offline/online and that's why the lock is there. Ah, okay, so it's actually synchronzing across multiple subsystems. I think it definitely calls for more documentation. The requirement is pretty unusual after all. Thanks! -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Tue, 2014-04-22 at 16:44 -0400, Tejun Heo wrote:
> Hello,
>
> On Tue, Apr 22, 2014 at 11:34:39AM +0800, Li Zhong wrote:
> > > Is this assumption true? If so, can we add lockdep assertions in
> > > places to verify and enforce this? If not, aren't we just feeling
> > > good when the reality is broken?
> >
> > It seems not true ... I think there are devices that don't have the
> > online/offline concept, we just need to add it, remove it, like ethernet
> > cards.
> >
> > Maybe we could change the comments above, like:
> > /* We assume device_hotplug_lock must be acquired before
> > * removing devices, which have online/offline sysfs knob,
> > * and some locks are needed to serialize the online/offline
> > * callbacks and device removing. ...
> > ?
> >
> > And we could add lockdep assertions in cpu and memory related code? e.g.
> > remove_memory(), unregister_cpu()
> >
> > Currently, remove_memory() has comments for the function:
> >
> > * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> > * and online/offline operations before this call, as required by
> > * try_offline_node().
> > */
> >
> > maybe it could be removed with the lockdep assertion.
>
> I'm confused about the overall locking scheme. What's the role of
> device_hotplug_lock? Is that solely to prevent the sysfs deadlock
> issue? Or does it serve other synchronization purposes depending on
> the specific subsystem? If the former, the lock no longer needs to
> exist. The only thing necessary would be synchronization between
> device_del() deleting the sysfs file and the unbreak helper invoking
> device-specific callback. If the latter, we probably should change
> that. Sharing hotplug lock across multiple subsystems through driver
> core sounds like a pretty bad idea.
I think it's the latter. I think device_{on|off}line is better to be
done in some sort of lock which prevents the device from being removed,
including some preparation work that needs be done before device_del().
Thanks, Zhong
>
> Thanks.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Tue, 2014-04-22 at 12:11 +0200, Rafael J. Wysocki wrote:
> On Tuesday, April 22, 2014 11:34:39 AM Li Zhong wrote:
> > On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
> > > Hello,
> > >
> > > On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
> > >
> > > Proper /** function comment would be nice.
> >
> > Ok, will try to write some in next version.
> >
> > >
> > > > +struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
> > > > + struct device_attribute
> > > > *attr)
> > >
> > > I can see why you did this but let's please not require the user of
> > > this function to see how the thing is working internally. Let's
> > > return int and keep track of (or look up again) the kernfs_node
> > > internally.
> >
> > Ok, it also makes the prototype of lock and unlock look more consistent
> > and comfortable.
> >
> > >
> > > > {
> > > ...
> > > > + /*
> > > > +* We assume device_hotplug_lock must be acquired before
> > > > removing
> > >
> > > Is this assumption true? If so, can we add lockdep assertions in
> > > places to verify and enforce this? If not, aren't we just feeling
> > > good when the reality is broken?
> >
> > It seems not true ... I think there are devices that don't have the
> > online/offline concept, we just need to add it, remove it, like ethernet
> > cards.
>
> Well, I haven't been following this closely (I was travelling, sorry), but
> there certainly are devices without online/offline. That currently is only
> present for CPUs, memory blocks and ACPI containers (if I remember correctly).
>
> >
> > Maybe we could change the comments above, like:
> > /* We assume device_hotplug_lock must be acquired before
> > * removing devices, which have online/offline sysfs knob,
> > * and some locks are needed to serialize the online/offline
> > * callbacks and device removing. ...
> > ?
>
> Lockdep assertions would be better than this in my opinion.
This is talking about the lock required in the other process, the device
removing process, e.g. that in remove_memory() below. So I guess no
lockdep assertions needed here. Or I misunderstand your point?
> >
> > And we could add lockdep assertions in cpu and memory related code? e.g.
> > remove_memory(), unregister_cpu()
> >
> > Currently, remove_memory() has comments for the function:
> >
> > * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> > * and online/offline operations before this call, as required by
> > * try_offline_node().
> > */
> >
> > maybe it could be removed with the lockdep assertion.
>
> No, please don't remove it. It is there to explain where the locking
> requirement
> comes from.
OK, I see. I think I'll just add lockdep assertions, and keep the
comments there.
Thanks, Zhong
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On 4/22/2014 10:44 PM, Tejun Heo wrote: Hello, On Tue, Apr 22, 2014 at 11:34:39AM +0800, Li Zhong wrote: Is this assumption true? If so, can we add lockdep assertions in places to verify and enforce this? If not, aren't we just feeling good when the reality is broken? It seems not true ... I think there are devices that don't have the online/offline concept, we just need to add it, remove it, like ethernet cards. Maybe we could change the comments above, like: /* We assume device_hotplug_lock must be acquired before * removing devices, which have online/offline sysfs knob, * and some locks are needed to serialize the online/offline * callbacks and device removing. ... ? And we could add lockdep assertions in cpu and memory related code? e.g. remove_memory(), unregister_cpu() Currently, remove_memory() has comments for the function: * NOTE: The caller must call lock_device_hotplug() to serialize hotplug * and online/offline operations before this call, as required by * try_offline_node(). */ maybe it could be removed with the lockdep assertion. I'm confused about the overall locking scheme. What's the role of device_hotplug_lock? Is that solely to prevent the sysfs deadlock issue? Or does it serve other synchronization purposes depending on the specific subsystem? If the former, the lock no longer needs to exist. The only thing necessary would be synchronization between device_del() deleting the sysfs file and the unbreak helper invoking device-specific callback. If the latter, we probably should change that. Sharing hotplug lock across multiple subsystems through driver core sounds like a pretty bad idea. Can you please elaborate a bit? It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
Hello, On Tue, Apr 22, 2014 at 11:34:39AM +0800, Li Zhong wrote: > > Is this assumption true? If so, can we add lockdep assertions in > > places to verify and enforce this? If not, aren't we just feeling > > good when the reality is broken? > > It seems not true ... I think there are devices that don't have the > online/offline concept, we just need to add it, remove it, like ethernet > cards. > > Maybe we could change the comments above, like: > /* We assume device_hotplug_lock must be acquired before >* removing devices, which have online/offline sysfs knob, >* and some locks are needed to serialize the online/offline >* callbacks and device removing. ... > ? > > And we could add lockdep assertions in cpu and memory related code? e.g. > remove_memory(), unregister_cpu() > > Currently, remove_memory() has comments for the function: > > * NOTE: The caller must call lock_device_hotplug() to serialize hotplug > * and online/offline operations before this call, as required by > * try_offline_node(). > */ > > maybe it could be removed with the lockdep assertion. I'm confused about the overall locking scheme. What's the role of device_hotplug_lock? Is that solely to prevent the sysfs deadlock issue? Or does it serve other synchronization purposes depending on the specific subsystem? If the former, the lock no longer needs to exist. The only thing necessary would be synchronization between device_del() deleting the sysfs file and the unbreak helper invoking device-specific callback. If the latter, we probably should change that. Sharing hotplug lock across multiple subsystems through driver core sounds like a pretty bad idea. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Tuesday, April 22, 2014 11:34:39 AM Li Zhong wrote:
> On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
> > Hello,
> >
> > On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
> >
> > Proper /** function comment would be nice.
>
> Ok, will try to write some in next version.
>
> >
> > > +struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
> > > +struct device_attribute *attr)
> >
> > I can see why you did this but let's please not require the user of
> > this function to see how the thing is working internally. Let's
> > return int and keep track of (or look up again) the kernfs_node
> > internally.
>
> Ok, it also makes the prototype of lock and unlock look more consistent
> and comfortable.
>
> >
> > > {
> > ...
> > > + /*
> > > + * We assume device_hotplug_lock must be acquired before removing
> >
> > Is this assumption true? If so, can we add lockdep assertions in
> > places to verify and enforce this? If not, aren't we just feeling
> > good when the reality is broken?
>
> It seems not true ... I think there are devices that don't have the
> online/offline concept, we just need to add it, remove it, like ethernet
> cards.
Well, I haven't been following this closely (I was travelling, sorry), but
there certainly are devices without online/offline. That currently is only
present for CPUs, memory blocks and ACPI containers (if I remember correctly).
>
> Maybe we could change the comments above, like:
> /* We assume device_hotplug_lock must be acquired before
>* removing devices, which have online/offline sysfs knob,
>* and some locks are needed to serialize the online/offline
>* callbacks and device removing. ...
> ?
Lockdep assertions would be better than this in my opinion.
>
> And we could add lockdep assertions in cpu and memory related code? e.g.
> remove_memory(), unregister_cpu()
>
> Currently, remove_memory() has comments for the function:
>
> * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> * and online/offline operations before this call, as required by
> * try_offline_node().
> */
>
> maybe it could be removed with the lockdep assertion.
No, please don't remove it. It is there to explain where the locking
requirement
comes from.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Tuesday, April 22, 2014 11:34:39 AM Li Zhong wrote:
On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
Hello,
On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
Proper /** function comment would be nice.
Ok, will try to write some in next version.
+struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
+struct device_attribute *attr)
I can see why you did this but let's please not require the user of
this function to see how the thing is working internally. Let's
return int and keep track of (or look up again) the kernfs_node
internally.
Ok, it also makes the prototype of lock and unlock look more consistent
and comfortable.
{
...
+ /*
+ * We assume device_hotplug_lock must be acquired before removing
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
It seems not true ... I think there are devices that don't have the
online/offline concept, we just need to add it, remove it, like ethernet
cards.
Well, I haven't been following this closely (I was travelling, sorry), but
there certainly are devices without online/offline. That currently is only
present for CPUs, memory blocks and ACPI containers (if I remember correctly).
Maybe we could change the comments above, like:
/* We assume device_hotplug_lock must be acquired before
* removing devices, which have online/offline sysfs knob,
* and some locks are needed to serialize the online/offline
* callbacks and device removing. ...
?
Lockdep assertions would be better than this in my opinion.
And we could add lockdep assertions in cpu and memory related code? e.g.
remove_memory(), unregister_cpu()
Currently, remove_memory() has comments for the function:
* NOTE: The caller must call lock_device_hotplug() to serialize hotplug
* and online/offline operations before this call, as required by
* try_offline_node().
*/
maybe it could be removed with the lockdep assertion.
No, please don't remove it. It is there to explain where the locking
requirement
comes from.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
Hello, On Tue, Apr 22, 2014 at 11:34:39AM +0800, Li Zhong wrote: Is this assumption true? If so, can we add lockdep assertions in places to verify and enforce this? If not, aren't we just feeling good when the reality is broken? It seems not true ... I think there are devices that don't have the online/offline concept, we just need to add it, remove it, like ethernet cards. Maybe we could change the comments above, like: /* We assume device_hotplug_lock must be acquired before * removing devices, which have online/offline sysfs knob, * and some locks are needed to serialize the online/offline * callbacks and device removing. ... ? And we could add lockdep assertions in cpu and memory related code? e.g. remove_memory(), unregister_cpu() Currently, remove_memory() has comments for the function: * NOTE: The caller must call lock_device_hotplug() to serialize hotplug * and online/offline operations before this call, as required by * try_offline_node(). */ maybe it could be removed with the lockdep assertion. I'm confused about the overall locking scheme. What's the role of device_hotplug_lock? Is that solely to prevent the sysfs deadlock issue? Or does it serve other synchronization purposes depending on the specific subsystem? If the former, the lock no longer needs to exist. The only thing necessary would be synchronization between device_del() deleting the sysfs file and the unbreak helper invoking device-specific callback. If the latter, we probably should change that. Sharing hotplug lock across multiple subsystems through driver core sounds like a pretty bad idea. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On 4/22/2014 10:44 PM, Tejun Heo wrote: Hello, On Tue, Apr 22, 2014 at 11:34:39AM +0800, Li Zhong wrote: Is this assumption true? If so, can we add lockdep assertions in places to verify and enforce this? If not, aren't we just feeling good when the reality is broken? It seems not true ... I think there are devices that don't have the online/offline concept, we just need to add it, remove it, like ethernet cards. Maybe we could change the comments above, like: /* We assume device_hotplug_lock must be acquired before * removing devices, which have online/offline sysfs knob, * and some locks are needed to serialize the online/offline * callbacks and device removing. ... ? And we could add lockdep assertions in cpu and memory related code? e.g. remove_memory(), unregister_cpu() Currently, remove_memory() has comments for the function: * NOTE: The caller must call lock_device_hotplug() to serialize hotplug * and online/offline operations before this call, as required by * try_offline_node(). */ maybe it could be removed with the lockdep assertion. I'm confused about the overall locking scheme. What's the role of device_hotplug_lock? Is that solely to prevent the sysfs deadlock issue? Or does it serve other synchronization purposes depending on the specific subsystem? If the former, the lock no longer needs to exist. The only thing necessary would be synchronization between device_del() deleting the sysfs file and the unbreak helper invoking device-specific callback. If the latter, we probably should change that. Sharing hotplug lock across multiple subsystems through driver core sounds like a pretty bad idea. Can you please elaborate a bit? It is there to protect hotplug operations involving multiple devices (in different subsystems) from racing with each other. Why exactly is it bad? Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Tue, 2014-04-22 at 12:11 +0200, Rafael J. Wysocki wrote:
On Tuesday, April 22, 2014 11:34:39 AM Li Zhong wrote:
On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
Hello,
On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
Proper /** function comment would be nice.
Ok, will try to write some in next version.
+struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
+ struct device_attribute
*attr)
I can see why you did this but let's please not require the user of
this function to see how the thing is working internally. Let's
return int and keep track of (or look up again) the kernfs_node
internally.
Ok, it also makes the prototype of lock and unlock look more consistent
and comfortable.
{
...
+ /*
+* We assume device_hotplug_lock must be acquired before
removing
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
It seems not true ... I think there are devices that don't have the
online/offline concept, we just need to add it, remove it, like ethernet
cards.
Well, I haven't been following this closely (I was travelling, sorry), but
there certainly are devices without online/offline. That currently is only
present for CPUs, memory blocks and ACPI containers (if I remember correctly).
Maybe we could change the comments above, like:
/* We assume device_hotplug_lock must be acquired before
* removing devices, which have online/offline sysfs knob,
* and some locks are needed to serialize the online/offline
* callbacks and device removing. ...
?
Lockdep assertions would be better than this in my opinion.
This is talking about the lock required in the other process, the device
removing process, e.g. that in remove_memory() below. So I guess no
lockdep assertions needed here. Or I misunderstand your point?
And we could add lockdep assertions in cpu and memory related code? e.g.
remove_memory(), unregister_cpu()
Currently, remove_memory() has comments for the function:
* NOTE: The caller must call lock_device_hotplug() to serialize hotplug
* and online/offline operations before this call, as required by
* try_offline_node().
*/
maybe it could be removed with the lockdep assertion.
No, please don't remove it. It is there to explain where the locking
requirement
comes from.
OK, I see. I think I'll just add lockdep assertions, and keep the
comments there.
Thanks, Zhong
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Tue, 2014-04-22 at 16:44 -0400, Tejun Heo wrote:
Hello,
On Tue, Apr 22, 2014 at 11:34:39AM +0800, Li Zhong wrote:
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
It seems not true ... I think there are devices that don't have the
online/offline concept, we just need to add it, remove it, like ethernet
cards.
Maybe we could change the comments above, like:
/* We assume device_hotplug_lock must be acquired before
* removing devices, which have online/offline sysfs knob,
* and some locks are needed to serialize the online/offline
* callbacks and device removing. ...
?
And we could add lockdep assertions in cpu and memory related code? e.g.
remove_memory(), unregister_cpu()
Currently, remove_memory() has comments for the function:
* NOTE: The caller must call lock_device_hotplug() to serialize hotplug
* and online/offline operations before this call, as required by
* try_offline_node().
*/
maybe it could be removed with the lockdep assertion.
I'm confused about the overall locking scheme. What's the role of
device_hotplug_lock? Is that solely to prevent the sysfs deadlock
issue? Or does it serve other synchronization purposes depending on
the specific subsystem? If the former, the lock no longer needs to
exist. The only thing necessary would be synchronization between
device_del() deleting the sysfs file and the unbreak helper invoking
device-specific callback. If the latter, we probably should change
that. Sharing hotplug lock across multiple subsystems through driver
core sounds like a pretty bad idea.
I think it's the latter. I think device_{on|off}line is better to be
done in some sort of lock which prevents the device from being removed,
including some preparation work that needs be done before device_del().
Thanks, Zhong
Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
> Hello,
>
> On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
>
> Proper /** function comment would be nice.
Ok, will try to write some in next version.
>
> > +struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
> > + struct device_attribute *attr)
>
> I can see why you did this but let's please not require the user of
> this function to see how the thing is working internally. Let's
> return int and keep track of (or look up again) the kernfs_node
> internally.
Ok, it also makes the prototype of lock and unlock look more consistent
and comfortable.
>
> > {
> ...
> > + /*
> > +* We assume device_hotplug_lock must be acquired before removing
>
> Is this assumption true? If so, can we add lockdep assertions in
> places to verify and enforce this? If not, aren't we just feeling
> good when the reality is broken?
It seems not true ... I think there are devices that don't have the
online/offline concept, we just need to add it, remove it, like ethernet
cards.
Maybe we could change the comments above, like:
/* We assume device_hotplug_lock must be acquired before
* removing devices, which have online/offline sysfs knob,
* and some locks are needed to serialize the online/offline
* callbacks and device removing. ...
?
And we could add lockdep assertions in cpu and memory related code? e.g.
remove_memory(), unregister_cpu()
Currently, remove_memory() has comments for the function:
* NOTE: The caller must call lock_device_hotplug() to serialize hotplug
* and online/offline operations before this call, as required by
* try_offline_node().
*/
maybe it could be removed with the lockdep assertion.
> ...
>
> Function comment please.
OK.
Thanks, Zhong
> > +void unlock_device_hotplug_sysfs(struct device *dev,
> > +struct kernfs_node *kn)
> > +{
> > + unlock_device_hotplug();
> > + kernfs_unbreak_active_protection(kn);
> > + put_device(dev);
> > + kernfs_put(kn);
> > }
>
> Thanks.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
Hello,
On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
Proper /** function comment would be nice.
> +struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
> +struct device_attribute *attr)
I can see why you did this but let's please not require the user of
this function to see how the thing is working internally. Let's
return int and keep track of (or look up again) the kernfs_node
internally.
> {
...
> + /*
> + * We assume device_hotplug_lock must be acquired before removing
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
...
Function comment please.
> +void unlock_device_hotplug_sysfs(struct device *dev,
> + struct kernfs_node *kn)
> +{
> + unlock_device_hotplug();
> + kernfs_unbreak_active_protection(kn);
> + put_device(dev);
> + kernfs_put(kn);
> }
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
This patch tries to solve the device hot remove locking issues in a
different way from commit 5e33bc41, as kernfs already has a mechanism
to break active protection.
The active protection is there to keep the file alive by blocking
deletion while operations are on-going in the file. This blocking
creates a dependency loop when an operation running off a sysfs knob
ends up grabbing a lock which may be held while removing the said sysfs
knob.
So the problem here is the order of s_active, and the series of hotplug
related locks.
commit 5e33bc41 solves it by taking out the first of the series of
hoplug related locks, device_hotplug_lock, with a try lock. And if that
try lock fails, it returns a restart syscall error, and drops s_active
temporarily to allow the other process to remove the sysfs knob.
This doesn't help with lockdep warnings reported against s_active and
other hotplug related locks in the series.
This patch instead tries to take s_active out of the lock dependency
graph using the active protection breaking mechanism.
lock_device_hotplug_sysfs() function name is kept here, two more
arguments are added, dev, attr, to help find the kernfs_node
corresponding to the sysfs knob (which should always be able to be
found, WARN if not). The found kernfs_node is recorded as the return
value, to be released in unlock_device_hotplug_sysfs().
As we break the active protection here, the device removing process
might remove the sysfs entries after that point. So after we grab the
device_hotplug_lock, we need check whether that really happens. If so,
we should abort and not invoke the online/offline callbacks anymore. In
this case, NULL is returned to the caller, so it could return with
ENODEV.
Signed-off-by: Li Zhong
---
drivers/base/core.c| 63 ++
drivers/base/memory.c | 9
include/linux/device.h | 5 +++-
3 files changed, 62 insertions(+), 15 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0dd6528..4d37a2b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -61,14 +61,55 @@ void unlock_device_hotplug(void)
mutex_unlock(_hotplug_lock);
}
-int lock_device_hotplug_sysfs(void)
+struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
+ struct device_attribute *attr)
{
- if (mutex_trylock(_hotplug_lock))
- return 0;
+ struct kernfs_node *kn;
+
+ kn = kernfs_find_and_get(dev->kobj.sd, attr->attr.name);
+
+ if (WARN_ON_ONCE(!kn))
+ return NULL;
+
+ /*
+* Break active protection here to avoid deadlocks with device
+* removing process, which tries to remove sysfs entries including this
+* "online" attribute while holding some hotplug related locks.
+*
+* @dev needs to be protected here, or it could go away any time after
+* dropping active protection.
+*/
+
+ get_device(dev);
+ kernfs_break_active_protection(kn);
+
+ lock_device_hotplug();
+
+ /*
+* We assume device_hotplug_lock must be acquired before removing
+* device, we can check here whether the device has been removed, so
+* we don't call device_{on|off}line against removed device.
+*/
- /* Avoid busy looping (5 ms of sleep should do). */
- msleep(5);
- return restart_syscall();
+ if (!dev->kobj.sd) {
+ /* device_del() already called on @dev, we should also abort */
+ unlock_device_hotplug();
+ kernfs_unbreak_active_protection(kn);
+ put_device(dev);
+ kernfs_put(kn);
+ return NULL;
+ }
+
+ return kn;
+}
+
+void unlock_device_hotplug_sysfs(struct device *dev,
+struct kernfs_node *kn)
+{
+ unlock_device_hotplug();
+ kernfs_unbreak_active_protection(kn);
+ put_device(dev);
+ kernfs_put(kn);
}
#ifdef CONFIG_BLOCK
@@ -439,17 +480,19 @@ static ssize_t online_store(struct device *dev, struct
device_attribute *attr,
{
bool val;
int ret;
+ struct kernfs_node *kn;
ret = strtobool(buf, );
if (ret < 0)
return ret;
- ret = lock_device_hotplug_sysfs();
- if (ret)
- return ret;
+ kn = lock_device_hotplug_sysfs(dev, attr);
+ if (!kn)
+ return -ENODEV;
ret = val ? device_online(dev) : device_offline(dev);
- unlock_device_hotplug();
+ unlock_device_hotplug_sysfs(dev, kn);
+
return ret < 0 ? ret : count;
}
static DEVICE_ATTR_RW(online);
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index bece691..c2b66d4 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -320,10 +320,11 @@ store_mem_state(struct device *dev,
{
struct memory_block *mem = to_memory_block(dev);
int ret,
[RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
This patch tries to solve the device hot remove locking issues in a
different way from commit 5e33bc41, as kernfs already has a mechanism
to break active protection.
The active protection is there to keep the file alive by blocking
deletion while operations are on-going in the file. This blocking
creates a dependency loop when an operation running off a sysfs knob
ends up grabbing a lock which may be held while removing the said sysfs
knob.
So the problem here is the order of s_active, and the series of hotplug
related locks.
commit 5e33bc41 solves it by taking out the first of the series of
hoplug related locks, device_hotplug_lock, with a try lock. And if that
try lock fails, it returns a restart syscall error, and drops s_active
temporarily to allow the other process to remove the sysfs knob.
This doesn't help with lockdep warnings reported against s_active and
other hotplug related locks in the series.
This patch instead tries to take s_active out of the lock dependency
graph using the active protection breaking mechanism.
lock_device_hotplug_sysfs() function name is kept here, two more
arguments are added, dev, attr, to help find the kernfs_node
corresponding to the sysfs knob (which should always be able to be
found, WARN if not). The found kernfs_node is recorded as the return
value, to be released in unlock_device_hotplug_sysfs().
As we break the active protection here, the device removing process
might remove the sysfs entries after that point. So after we grab the
device_hotplug_lock, we need check whether that really happens. If so,
we should abort and not invoke the online/offline callbacks anymore. In
this case, NULL is returned to the caller, so it could return with
ENODEV.
Signed-off-by: Li Zhong zh...@linux.vnet.ibm.com
---
drivers/base/core.c| 63 ++
drivers/base/memory.c | 9
include/linux/device.h | 5 +++-
3 files changed, 62 insertions(+), 15 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0dd6528..4d37a2b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -61,14 +61,55 @@ void unlock_device_hotplug(void)
mutex_unlock(device_hotplug_lock);
}
-int lock_device_hotplug_sysfs(void)
+struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
+ struct device_attribute *attr)
{
- if (mutex_trylock(device_hotplug_lock))
- return 0;
+ struct kernfs_node *kn;
+
+ kn = kernfs_find_and_get(dev-kobj.sd, attr-attr.name);
+
+ if (WARN_ON_ONCE(!kn))
+ return NULL;
+
+ /*
+* Break active protection here to avoid deadlocks with device
+* removing process, which tries to remove sysfs entries including this
+* online attribute while holding some hotplug related locks.
+*
+* @dev needs to be protected here, or it could go away any time after
+* dropping active protection.
+*/
+
+ get_device(dev);
+ kernfs_break_active_protection(kn);
+
+ lock_device_hotplug();
+
+ /*
+* We assume device_hotplug_lock must be acquired before removing
+* device, we can check here whether the device has been removed, so
+* we don't call device_{on|off}line against removed device.
+*/
- /* Avoid busy looping (5 ms of sleep should do). */
- msleep(5);
- return restart_syscall();
+ if (!dev-kobj.sd) {
+ /* device_del() already called on @dev, we should also abort */
+ unlock_device_hotplug();
+ kernfs_unbreak_active_protection(kn);
+ put_device(dev);
+ kernfs_put(kn);
+ return NULL;
+ }
+
+ return kn;
+}
+
+void unlock_device_hotplug_sysfs(struct device *dev,
+struct kernfs_node *kn)
+{
+ unlock_device_hotplug();
+ kernfs_unbreak_active_protection(kn);
+ put_device(dev);
+ kernfs_put(kn);
}
#ifdef CONFIG_BLOCK
@@ -439,17 +480,19 @@ static ssize_t online_store(struct device *dev, struct
device_attribute *attr,
{
bool val;
int ret;
+ struct kernfs_node *kn;
ret = strtobool(buf, val);
if (ret 0)
return ret;
- ret = lock_device_hotplug_sysfs();
- if (ret)
- return ret;
+ kn = lock_device_hotplug_sysfs(dev, attr);
+ if (!kn)
+ return -ENODEV;
ret = val ? device_online(dev) : device_offline(dev);
- unlock_device_hotplug();
+ unlock_device_hotplug_sysfs(dev, kn);
+
return ret 0 ? ret : count;
}
static DEVICE_ATTR_RW(online);
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index bece691..c2b66d4 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -320,10 +320,11 @@ store_mem_state(struct device *dev,
{
struct memory_block *mem =
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
Hello,
On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
Proper /** function comment would be nice.
+struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
+struct device_attribute *attr)
I can see why you did this but let's please not require the user of
this function to see how the thing is working internally. Let's
return int and keep track of (or look up again) the kernfs_node
internally.
{
...
+ /*
+ * We assume device_hotplug_lock must be acquired before removing
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
...
Function comment please.
+void unlock_device_hotplug_sysfs(struct device *dev,
+ struct kernfs_node *kn)
+{
+ unlock_device_hotplug();
+ kernfs_unbreak_active_protection(kn);
+ put_device(dev);
+ kernfs_put(kn);
}
Thanks.
--
tejun
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v5 2/2] Use kernfs_break_active_protection() for device online store callbacks
On Mon, 2014-04-21 at 18:46 -0400, Tejun Heo wrote:
Hello,
On Mon, Apr 21, 2014 at 05:23:50PM +0800, Li Zhong wrote:
Proper /** function comment would be nice.
Ok, will try to write some in next version.
+struct kernfs_node *lock_device_hotplug_sysfs(struct device *dev,
+ struct device_attribute *attr)
I can see why you did this but let's please not require the user of
this function to see how the thing is working internally. Let's
return int and keep track of (or look up again) the kernfs_node
internally.
Ok, it also makes the prototype of lock and unlock look more consistent
and comfortable.
{
...
+ /*
+* We assume device_hotplug_lock must be acquired before removing
Is this assumption true? If so, can we add lockdep assertions in
places to verify and enforce this? If not, aren't we just feeling
good when the reality is broken?
It seems not true ... I think there are devices that don't have the
online/offline concept, we just need to add it, remove it, like ethernet
cards.
Maybe we could change the comments above, like:
/* We assume device_hotplug_lock must be acquired before
* removing devices, which have online/offline sysfs knob,
* and some locks are needed to serialize the online/offline
* callbacks and device removing. ...
?
And we could add lockdep assertions in cpu and memory related code? e.g.
remove_memory(), unregister_cpu()
Currently, remove_memory() has comments for the function:
* NOTE: The caller must call lock_device_hotplug() to serialize hotplug
* and online/offline operations before this call, as required by
* try_offline_node().
*/
maybe it could be removed with the lockdep assertion.
...
Function comment please.
OK.
Thanks, Zhong
+void unlock_device_hotplug_sysfs(struct device *dev,
+struct kernfs_node *kn)
+{
+ unlock_device_hotplug();
+ kernfs_unbreak_active_protection(kn);
+ put_device(dev);
+ kernfs_put(kn);
}
Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
