Lifetime problems of the current driver model
Subsystems (3)
Hello, all.
This document tries to describe lifetime problems of the current device driver model primarily from the point view of device drivers and establish consensus, or at least, start discussion about how to solve these problems. This is primarily based on my experience with IDE and SCSI layers and my knowledge on other drivers is limited, so I might have generalized too aggressively. Feel free to point out.
Table of Contents
1. Why is it this complex?
1-1. Rule #a and #b
1-2. Rule #b and #c
2. How do we solve this?
2-1. Rule #a and #b
2-2. Rule #b and #c
3. How do we get there from here?
4. Some afterthoughts
1. Why is it this complex?
Object lifetime management in device drivers is complex and thus error-prone under the current driver model. The primary reason is that device drivers have to match impedances of three different life time rules.
Rule a. The device itself. Many devices are hot pluggable these days.
Devices come when they come and go when they go.
Rule b. Driver model object lifetime rule. Fundamental data
structures used by device drivers are reference counted and their lifetime can be arbitrarily extended by userspace sysfs access.
Rule c. Module life time rule. Most device drivers can be compiled as
modules and thus module busy count should be managed properly.
1-1. Rule #a and #b
Being what it is, the primary concern of a device driver is the 'device' itself. A device driver should create and release objects representing the devices it manages as they come and go. In one way or another, this just has to be done, so this rule is pretty obvious and intuitive to device driver writers.
The current device driver model mandates implementation of rule #b in device drivers. This makes the object lifetime management complicated and, more importantly, less intuitive to driver writers. As implementing such lifetime management in each device driver is too painful, most device driver midlayers handle this. e.g. SCSI midlayer handles driver model lifetime rules and presents simplified register/unregister-immediately interface to all low level SCSI drivers. IDE tries to do similar and USB and netdev have their own mechanisms too.
I'm not familiar with other layers, but it took quite a while to get this right in the SCSI midlayer. The current code works and low level drivers can do away with most of lifetime management but the added complexity is saddening to look at.
1-2. Rule #b and #c
On the surface, it seems that we're doing this mostly correctly but subtle bugs caused by interaction between driver model lifetime and module lifetime rules aren't difficult to find.
This happens because the two lifetime rules are closely related but the relation is not properly represented in device driver model itself. A module cannot be unloaded unless all device driver objects it hosts are released, but the only way to guarantee that is to always grab module reference whenever grabbing relevant kobject reference. This is ugly to code and too easy to get wrong. Here are two such examples.
Example 1. sysfs_schedule_callback() not grabbing the owning module
This function is recently added to be used by suicidal sysfs nodes such that they don't deadlock when trying to unregister themselves.
+#include <linux/delay.h>
static void sysfs_schedule_callback_work(struct work_struct *work)
{
struct sysfs_schedule_callback_struct *ss = container_of(work,
struct sysfs_schedule_callback_struct, work);+ msleep(100); (ss->func)(ss->data); kobject_put(ss->kobj); kfree(ss);
}
int sysfs_schedule_callback(struct kobject *kobj, void (*func)(void *),
void *data)
{
struct sysfs_schedule_callback_struct *ss;
ss = kmalloc(sizeof(*ss), GFP_KERNEL);
if (!ss)
return -ENOMEM;
kobject_get(kobj);
ss->kobj = kobj;
ss->func = func;
ss->data = data;
INIT_WORK(&ss->work, sysfs_schedule_callback_work);
schedule_work(&ss->work);
return 0;}
Two lines starting with '+' are inserted to make the problem reliably reproducible. With the above changes,
# insmod drivers/scsi/scsi_mod.ko; insmod drivers/scsi/sd_mod.ko; insmod drivers/ata/libata.ko; insmod drivers/ata/ahci.ko # echo 1 > /sys/block/sda/device/delete; rmmod ahci; rmmod libata; rmmod sd_mod; rmmod scsi_mod
It's assumed that ahci detects /dev/sda. The above command sequence causes the following oops.
BUG: unable to handle kernel paging request at virtual address e0984020 [--snip--] EIP is at 0xe0984020 [--snip--] [<c0127132>] run_workqueue+0x92/0x140 [<c01278c7>] worker_thread+0x137/0x160 [<c012a513>] kthread+0xa3/0xd0 [<c0104457>] kernel_thread_helper+0x7/0x10
The problem here is that kobjec_get() in sysfs_schedule_callback() doesn't grab the module backing the kobject it's grabbing. By the time (ss->func)(ss->kobj) runs, scsi_mod is already gone.
Example 2. sysfs attr grabbing the wrong kobject and module
This bug has been there for a long time, probably from the day these attrs are implemented.
# insmod drivers/scsi/scsi_mod.ko; insmod drivers/scsi/sd_mod.ko; insmod drivers/ata/libata.ko; insmod drivers/ata/ahci.ko # cat > /sys/block/sda/device/delete & # rmmod ahci; rmmod libata # kill %%
BUG: unable to handle kernel paging request at virtual address e083b1e8 [--snip--] EIP: 0060:[<e0983310>] Not tainted VLI [--snip--] [<c0127649>] execute_in_process_context+0x19/0x40 [<e09820f2>] scsi_target_reap+0x82/0xa0 [scsi_mod] [<e0984771>] scsi_device_dev_release_usercontext+0xe1/0x110 [scsi_mod] [<c0127649>] execute_in_process_context+0x19/0x40 [<e09835a3>] scsi_device_dev_release+0x13/0x20 [scsi_mod] [<c0287247>] device_release+0x17/0x90 [<c0213a69>] kobject_cleanup+0x49/0x80 [<c0213aab>] kobject_release+0xb/0x10 [<c021482b>] kref_put+0x2b/0x80 [<c0213a14>] kobject_put+0x14/0x20 [<c01969af>] sysfs_release+0x5f/0xb0 [<c015dcb4>] __fput+0xb4/0x1b0 [<c015de18>] fput+0x18/0x20 [<c015b207>] filp_close+0x47/0x70 [<c0119394>] put_files_struct+0xa4/0xc0 [<c011a4bf>] do_exit+0x11f/0x7c0 [<c011ab89>] do_group_exit+0x29/0x70 [<c0123415>] get_signal_to_deliver+0x265/0x3f0 [<c01036ea>] do_notify_resume+0x8a/0x7a0 [<c010425a>] work_notifysig+0x13/0x19
The delete attr's owner is scsi_mod. However, it's created under the sysfs node for the SCSI device under the node for SCSI host which is driven by libata and ahci combination. Opening the delete node grabs 1. struct device for the SCSI device which propagates to the SCSI host node and 2. module reference for sd_mod. So, ahci and libata can go away even while the SCSI host they implement is still alive.
Both examples can be reproduced using any SCSI low level driver. Attrs implemented similarly as in example #2 are everywhere. This is the standard way to implement such attrs.
2. How do we solve this?
2-1. Rule #a and #b
If you think about it, the impedance matching between rule #a and #b should be done somewhere. Maybe doing it in each low level driver is what's intended when the driver model is designed but it's impractically painful and the reality reflects that by doing it in midlayers.
It's better if we do it in higher layers. For example, SCSI has a mechanism to reject new requests and drain request_queue to allow low level driver to just unregister an existing device. IDE currently doesn't have such feature but it would need to do almost the same thing to support hotplug. If request_queue just had a feature to drain and kill itself, both SCSI and IDE could use it. It would be simpler and less error-prone. On the other hand, if it's pushed downward, it will cause much more pain in all the low level drivers.
Orphaning sysfs nodes on unregistration is a big step in this direction. With sysfs reference counting out of the picture, implementing 'disconnect immediate' interface only on a few components (including request_queue) should suffice for most block device drivers. I'm not familiar with other drivers but I don't think they'll be very different.
All in all, I'm hoping something like the following can be done in device drivers, midlayer or low level.
* For binding
alloc resources;
init controller;
register to upper layers;
* For unbinding
unregister from upper layers; (no lingering references or objects) deinit controller;
release resources;
This basically nullifies lifetime rule #b from the POV of drivers.
2-2. Rule #b and #c
One way to solve this problem is to subordinate lifetime rule #b to rule #c. Each kobject points to its owning module such that grabbing a kobject automatically grabs the module. The problem with this approach is that it requires wide update and makes kobject_get heavier.
Another more appealing approach is to do nothing. Solving the problem between rule #a and #b in the way described above means virtually nullifying rule #b. With rule #b gone, rule #c can't conflict with it. IOW, no reference from upper layer would be remaining after a driver unregisters itself from the upper layer - the module can be safely unloaded.
3. How do we get there from here?
The current device driver model is used by most device drivers - huge amount of code. We can't update them at once. Fortunately, the suggested solution can easily be implemented gradually. We can add 'disconnect now' interface to upper driver interface one-by-one and convert its users gradually. The existing reference counting mechanism can be left alone and used to verify that the conversion is correct by verifying reference count is 0 after unregistering.
Once the conversion is complete (maybe after a year), we can remove the reference counting mechanism from device driver interface.
4. Some afterthoughts
* Doing this would make module reference counting more flexible. Instead of being confined by implementation, it can be used to define when to allow and disallow module unloading. (you can't unload a block driver module if a fs on it is mounted but sysfs access doesn't matter.)
* Outside of device driver model, kobject is currently used to supply something sysfs nodes can be attached to, such that both the information supplier and node users have something common to reference count. With orphaning added, there is no need to use kobject this way. IMHO, kobject should have been implementation detail inside the device model proper in the first place.
Thanks.
Signature · 1 line
tejun
Quoted 6 lines · Tejun Heo
Orphaning sysfs nodes on unregistration is a big step in this direction. With sysfs reference counting out of the picture, implementing 'disconnect immediate' interface only on a few components (including
request_queue) should suffice for most block device drivers. I'm not familiar with other drivers but I don't think they'll be very different.
I agree with this statement. The big question in my mind is how do we do it?
The essential problem, and the reason why the lifetime rules are entangled is that fundamentally, sysfs entries are managed by kobjects. The things the device drivers are interested in is struct device, which is usually embedded in driver data structures. Unfortunately, the required kobject is usually embedded in struct device meaning that the relevant driver structure cannot be freed while the sysfs entry still exists.
It seems to me that the only way to Orphan the sysfs entries is to separate the kobject and the struct device so their lifetime rules are no longer entangled. Then we can free the driver structure with the embedded struct device while still keeping the necessary kobject around to perform orphaned sysfs operations.
So it seems to me that what we need to do is to give struct device its own kref and a pointer to a kobject. Then we can manage the separate lifetimes independently. The device would basically allocate and take a reference to the kobject on device_add() and relinquish it again (and null out the kobject pointer) on device_del(). The complexity here is that we now have to allocate the kobject somewhere else ... probably in device_add() (it doesn't come for free with the device structures).
James
Hi James,
Quoted 25 lines · James Bottomley
Quoted 6 lines · Tejun Heo
Orphaning sysfs nodes on unregistration is a big step in this direction. With sysfs reference counting out of the picture, implementing 'disconnect immediate' interface only on a few components (including
request_queue) should suffice for most block device drivers. I'm not familiar with other drivers but I don't think they'll be very different.I agree with this statement. The big question in my mind is how do we do it?
The essential problem, and the reason why the lifetime rules are entangled is that fundamentally, sysfs entries are managed by kobjects. The things the device drivers are interested in is struct device, which is usually embedded in driver data structures. Unfortunately, the required kobject is usually embedded in struct device meaning that the relevant driver structure cannot be freed while the sysfs entry still exists.
It seems to me that the only way to Orphan the sysfs entries is to separate the kobject and the struct device so their lifetime rules are no longer entangled. Then we can free the driver structure with the embedded struct device while still keeping the necessary kobject around to perform orphaned sysfs operations.
Quoted 8 lines
So it seems to me that what we need to do is to give struct device its own kref and a pointer to a kobject. Then we can manage the separate lifetimes independently. The device would basically allocate and take a reference to the kobject on
device_add()and relinquish it again (and null out the kobject pointer) ondevice_del(). The complexity here is that we now have to allocate the kobject somewhere else ... probably indevice_add()(it doesn't come for free with the device structures).
If you want to manage lifetime rules independently you might want to not embed struct device into you subsystems objects but attach them via pointers and use device_create(). Now that we orphan sysfs access upon unregistering device this will severe all ties from driver core to your system once you start teardown of a device and you should be in clear.
However there are simpler subsystems (input, serio, etc.) where there is only one core module which provides services to device drivers and handles registration and deregistration. For such substustems it makes sense to embed struct devices and manage lifetime for all components at once.
Signature · 1 line
Dmitry
Quoted 6 lines · Dmitry Torokhov
If you want to manage lifetime rules independently you might want to not embed struct device into you subsystems objects but attach them via pointers and use
device_create(). Now that we orphan sysfs access upon unregistering device this will severe all ties from driver core to your system once you start teardown of a device and you should be in clear.
But that wouldn't really help ... all objects have lifetimes. What Tejun is pointing out is that we have two different lifetime
requirements: driver internal (and subsystem) objects and sysfs objects ... we still need something embedded in the driver objects, so it may as well be struct device ... trying to manage struct device outside of the driver objects would turn into another nasty refcounting problem. The struct device is usually the generic abstraction of the specific structure it's embedded in, so I think it really does make sense to keep these pieces together.
Quoted 5 lines
However there are simpler subsystems (input, serio, etc.) where there is only one core module which provides services to device drivers and handles registration and deregistration. For such substustems it makes sense to embed struct devices and manage lifetime for all components at once.
James
Quoted 8 lines · James Bottomley
Quoted 6 lines · Dmitry Torokhov
If you want to manage lifetime rules independently you might want to not embed struct device into you subsystems objects but attach them via pointers and use
device_create(). Now that we orphan sysfs access upon unregistering device this will severe all ties from driver core to your system once you start teardown of a device and you should be in clear.
Quoted 4 lines
But that wouldn't really help ... all objects have lifetimes. What Tejun is pointing out is that we have two different lifetime
requirements: driver internal (and subsystem) objects and sysfs objects ...
Exactly. If driver-private data structures have different lifetime than device abstractions (like they seem to have with scsi and ide) then you split them and refcount separately. We are just arguing where to put the line. You want to split devices and kobjects and rework infrastructure and I am saying that we already have infrastructure we need and that split shoud be between generic device structure and driver-managed device structure.
Signature · 1 line
Dmitry
Hi James,
Quoted 25 lines · James Bottomley
Quoted 6 lines · Tejun Heo
Orphaning sysfs nodes on unregistration is a big step in this direction. With sysfs reference counting out of the picture, implementing 'disconnect immediate' interface only on a few components (including
request_queue) should suffice for most block device drivers. I'm not familiar with other drivers but I don't think they'll be very different.I agree with this statement. The big question in my mind is how do we do it?
The essential problem, and the reason why the lifetime rules are entangled is that fundamentally, sysfs entries are managed by kobjects. The things the device drivers are interested in is struct device, which is usually embedded in driver data structures. Unfortunately, the required kobject is usually embedded in struct device meaning that the relevant driver structure cannot be freed while the sysfs entry still exists.
It seems to me that the only way to Orphan the sysfs entries is to separate the kobject and the struct device so their lifetime rules are no longer entangled. Then we can free the driver structure with the embedded struct device while still keeping the necessary kobject around to perform orphaned sysfs operations.
Quoted 8 lines
So it seems to me that what we need to do is to give struct device its own kref and a pointer to a kobject. Then we can manage the separate lifetimes independently. The device would basically allocate and take a reference to the kobject on
device_add()and relinquish it again (and null out the kobject pointer) ondevice_del(). The complexity here is that we now have to allocate the kobject somewhere else ... probably indevice_add()(it doesn't come for free with the device structures).
If you want to manage lifetime rules independently you might want to not embed struct device into you subsystems objects but attach them via pointers and use device_create(). Now that we orphan sysfs access upon unregistering device this will severe all ties from driver core to your system once you start teardown of a device and you should be in clear.
However there are simpler subsystems (input, serio, etc.) where there is only one core module which provides services to device drivers and handles registration and deregistration. For such substustems it makes sense to embed struct devices and manage lifetime for all components at once.
Signature · 1 line
Dmitry
Quoted 25 lines · James Bottomley
Quoted 6 lines · Tejun Heo
Orphaning sysfs nodes on unregistration is a big step in this direction. With sysfs reference counting out of the picture, implementing 'disconnect immediate' interface only on a few components (including
request_queue) should suffice for most block device drivers. I'm not familiar with other drivers but I don't think they'll be very different.I agree with this statement. The big question in my mind is how do we do it?
The essential problem, and the reason why the lifetime rules are entangled is that fundamentally, sysfs entries are managed by kobjects. The things the device drivers are interested in is struct device, which is usually embedded in driver data structures. Unfortunately, the required kobject is usually embedded in struct device meaning that the relevant driver structure cannot be freed while the sysfs entry still exists.
It seems to me that the only way to Orphan the sysfs entries is to separate the kobject and the struct device so their lifetime rules are no longer entangled. Then we can free the driver structure with the embedded struct device while still keeping the necessary kobject around to perform orphaned sysfs operations.
Quoted 7 lines
So it seems to me that what we need to do is to give struct device its own kref and a pointer to a kobject. Then we can manage the separate lifetimes independently. The device would basically allocate and take a reference to the kobject on
device_add()and relinquish it again (and null out the kobject pointer) ondevice_del(). The complexity here is that we now have to allocate the kobject somewhere else ... probably indevice_add()(it doesn't come for free with the device structures).
My plan was to make sysfs more independent from struct device/kobject. e.g. Something like the following.
* kobject_get() on attr/symlink creation
* open() doesn't need extra kobject reference
* deleting a node makes it release the kobject reference and the kobject pointer is nullified.
This way the sysfs reference counting can be put completely out of picture without impacting the rest of code. Handling symlink and suicidal attributes might need some extra attention but I think they can be done.
In the long term, I think sysfs should be independent from driver model and kobject such that an entity which wants to use sysfs but is not a device doesn't have to dance with kobject just to use sysfs.
Thanks.
Signature · 1 line
tejun
Quoted 34 lines · Tejun Heo
Quoted 25 lines · James Bottomley
Quoted 6 lines · Tejun Heo
Orphaning sysfs nodes on unregistration is a big step in this direction. With sysfs reference counting out of the picture, implementing 'disconnect immediate' interface only on a few components (including
request_queue) should suffice for most block device drivers. I'm not familiar with other drivers but I don't think they'll be very different.I agree with this statement. The big question in my mind is how do we do it?
The essential problem, and the reason why the lifetime rules are entangled is that fundamentally, sysfs entries are managed by kobjects. The things the device drivers are interested in is struct device, which is usually embedded in driver data structures. Unfortunately, the required kobject is usually embedded in struct device meaning that the relevant driver structure cannot be freed while the sysfs entry still exists.
It seems to me that the only way to Orphan the sysfs entries is to separate the kobject and the struct device so their lifetime rules are no longer entangled. Then we can free the driver structure with the embedded struct device while still keeping the necessary kobject around to perform orphaned sysfs operations.
Quoted 7 lines
So it seems to me that what we need to do is to give struct device its own kref and a pointer to a kobject. Then we can manage the separate lifetimes independently. The device would basically allocate and take a reference to the kobject on
device_add()and relinquish it again (and null out the kobject pointer) ondevice_del(). The complexity here is that we now have to allocate the kobject somewhere else ... probably indevice_add()(it doesn't come for free with the device structures).
Quoted 7 lines
My plan was to make sysfs more independent from struct device/kobject. e.g. Something like the following.
*
kobject_get()on attr/symlink creation
*open()doesn't need extra kobject reference
* deleting a node makes it release the kobject reference and the kobject pointer is nullified.
Hm, this sounds good, but I think it will be racy.
Although separating the very tight tie between sysfs and kobject would be very good to have. Pat originally wanted to do that years ago, but he's now lost to the land of a million sheep...
So I don't object to this goal at all.
Quoted 4 lines
This way the sysfs reference counting can be put completely out of picture without impacting the rest of code. Handling symlink and suicidal attributes might need some extra attention but I think they can be done.
Ok, I'll but I really want to see that code :)
Quoted 3 lines
In the long term, I think sysfs should be independent from driver model and kobject such that an entity which wants to use sysfs but is not a device doesn't have to dance with kobject just to use sysfs.
I agree.
thanks,
greg k-h
Quoted 2 lines · Tejun Heo
My plan was to make sysfs more independent from struct device/kobject. e.g. Something like the following.
That's sort of what I was reaching for too ... it just looks to me that all the sysfs glue is in kobject, so they make a good candidate for the pure sysfs objects. Whatever we do, there has to be breakable cross linking between the driver's tree and the sysfs representation ... of course, nasty things like ksets get in the way of considering the objects as separate, sigh.
Quoted 4 lines
*
kobject_get()on attr/symlink creation
*open()doesn't need extra kobject reference
* deleting a node makes it release the kobject reference and the kobject pointer is nullified.
To do this, you'll have to move at least the dentry out of the kobject, I think.
Quoted 4 lines
This way the sysfs reference counting can be put completely out of picture without impacting the rest of code. Handling symlink and suicidal attributes might need some extra attention but I think they can be done.
True ... but you'll also have to implement something within sysfs to do refcounting ... it needs to know how long to keep its nodes around.
Quoted 3 lines
In the long term, I think sysfs should be independent from driver model and kobject such that an entity which wants to use sysfs but is not a device doesn't have to dance with kobject just to use sysfs.
I agree ... the question is just how to do it.
I'd favour trying to separate kobject and struct device for this ... move all the sysfs stuff into kobject and device only stuff into struct device ... but that would get us into disentangling the ksets, which, on balance, isn't going to be fun ...
James
Hello, James, Greg.
Quoted 6 lines · James Bottomley
That's sort of what I was reaching for too ... it just looks to me that all the sysfs glue is in kobject, so they make a good candidate for the pure sysfs objects. Whatever we do, there has to be breakable cross linking between the driver's tree and the sysfs representation ... of course, nasty things like ksets get in the way of considering the objects as separate, sigh.
The dependency between kobject and sysfs doesn't seem to be that tight. I managed to break them such that sysfs can disconnect from its kobject on deletion _without_ changing sysfs/kobject API.
Quoted 7 lines
Quoted 4 lines
This way the sysfs reference counting can be put completely out of picture without impacting the rest of code. Handling symlink and suicidal attributes might need some extra attention but I think they can be done.
True ... but you'll also have to implement something within sysfs to do refcounting ... it needs to know how long to keep its nodes around.
sysfs_dirent already had reference counter and plays that role quite nicely.
Okay, here's preliminary patch. I've tested it very lightly so it's likely to contain bugs and definitely needs to be splitted.
Dependencies between sysfs/kobject objects are clearer now and I think I got the locking and referencing correct. This patch immediately fixes 'sysfs attr grabbing the wrong kobject and module' problem - sysfs and module lifetimes are unrelated now. We can drop half-working attr->owner.
* A sysfs node no longer hold reference to its kobject. It just attaches itself to the kobject on creation and detaches on deletion.
* For a dir node, sysfs_dirent no longer directly points to kobject. It points to sysfs_dir which contains pointer to kobject and a rwsem to protect it.
* An open file doesn't hold a reference to kobject. It holds a reference to sysfs_dirent. kobject pointer is verified and show/store are performed with rwsem read-locked. Deletion disconnects the sysfs from its kobject while the rwsem is write-locked. This mechanism replaces buffer orphaning and kobject validation during open.
* attr ops is determined on sysfs node creation not on open. This is a step toward making kobject opaque in sysfs.
* bin files are handled similarly but mmap makes the open file hold reference to the kobject till it's closed. Any better ideas?
* symlink is reimplemented in terms of sysfs_dirents instead of kobjects. sysfs_dirent->s_parent is added and each sysfs_dirent holds reference to its parent. Currently walking up the tree requires read locking and unlocking sysfs_dir at each level. This can be removed if name is added to sysfs_dirent.
* As kobject can be disconnected anytime and sysfs code still needs to look follow dentry link in kobject, kobject->dentry is protected by dcache_lock. Once kobject becomes opaque to sysfs, this hack can go away. All in all, making kobject completely opaque in sysfs isn't too far away after this patch although it would require mass code update.
What do you think about this approach? If it's acceptable, I'll test further and split the patch into logical steps to get it reviewed better.
Thanks.
--- fs/sysfs/bin.c | 121 +++++++++++++++++++++++++++++++--------- fs/sysfs/dir.c | 134 ++++++++++++++++++++++++++++++++++++++------ fs/sysfs/file.c | 158 ++++++++++++++++++++++------------------------------- fs/sysfs/inode.c | 25 -------- fs/sysfs/mount.c | 9 --- fs/sysfs/symlink.c | 115 +++++++++++++++++++++++--------------- fs/sysfs/sysfs.h | 75 +++++-------------------- 7 files changed, 366 insertions(+), 271 deletions(-)
fs/sysfs/bin.c +94 -27
@@ -20,26 +20,41 @@#include "sysfs.h" +struct bin_buffer { +struct mutex mutex; +void *buffer; +int mmapped; +}; +static int fill_read (struct dentry *dentry,char *buffer,loff_t off,size_t count) {struct bin_attribute * attr =to_bin_attr (dentry); -struct kobject * kobj =to_kobj (dentry->d_parent); +struct sysfs_dirent * sd = dentry->d_parent->d_fsdata; +struct kobject * kobj; +int rc;if (!attr->read)return -EIO ; -return attr->read (kobj, buffer, off, count); + kobj =sysfs_get_kobj (sd); +if (!kobj) +return -ENODEV ; + + rc = attr->read (kobj, buffer, off, count); + +sysfs_put_kobj (sd); + +return rc; }static ssize_t read (struct file * file,char __user * userbuf,size_t count,loff_t * off) { -char *buffer = file->private_data; +struct bin_buffer *bb = file->private_data;struct dentry *dentry = file->f_path.dentry;int size = dentry->d_inode->i_size;loff_t offs = *off; -int ret;if (count >PAGE_SIZE ) count =PAGE_SIZE ;
@@ -51,18 +66,23 @@ read(struct file * file, char __user * userbuf, size_t count, loff_t * off) count = size - offs; } - ret =fill_read (dentry, buffer, offs, count); -if (ret <0 ) -return ret; - count = ret; +mutex_lock (&bb->mutex); -if (copy_to_user (userbuf, buffer, count)) -return -EFAULT ; + count =fill_read (dentry, bb->buffer, offs, count); +if (count <0 ) + goto out_unlock; + +if (copy_to_user (userbuf, bb->buffer, count)) { + count = -EFAULT ; + goto out_unlock; + }pr_debug ("offs = %lld, *off = %lld, count = %zd\n" , offs, *off, count); *off = offs + count; + out_unlock: +mutex_unlock (&bb->mutex);return count; }
@@ -70,18 +90,28 @@ static int flush_write(struct dentry *dentry, char *buffer, loff_t offset, size_t count) { struct bin_attribute *attr = to_bin_attr(dentry); -struct kobject *kobj =to_kobj (dentry->d_parent); +struct sysfs_dirent * sd = dentry->d_parent->d_fsdata; +struct kobject * kobj; +int rc;if (!attr->write)return -EIO ; -return attr->write (kobj, buffer, offset, count); + kobj =sysfs_get_kobj (sd); +if (!kobj) +return -ENODEV ; + + rc = attr->write (kobj, buffer, offset, count); + +sysfs_put_kobj (sd); + +return rc; }static ssize_t write (struct file * file,const char __user * userbuf,size_t count,loff_t * off) { -char *buffer = file->private_data; +struct bin_buffer *bb = file->private_data;struct dentry *dentry = file->f_path.dentry;int size = dentry->d_inode->i_size;loff_t offs = *off;
@@ -95,34 +125,60 @@ static ssize_t write(struct file * file, const char __user * userbuf, count = size - offs; } -if (copy_from_user (buffer, userbuf, count)) -return -EFAULT ; +mutex_lock (&bb->mutex); - count =flush_write (dentry, buffer, offs, count); +if (copy_from_user (bb->buffer, userbuf, count)) { + count = -EFAULT ; + goto out_unlock; + } + + count =flush_write (dentry, bb->buffer, offs, count);if (count >0 ) *off = offs + count; + + out_unlock: +mutex_unlock (&bb->mutex);return count; }static int mmap (struct file *file,struct vm_area_struct *vma) { +struct bin_buffer *bb = file->private_data;struct dentry *dentry = file->f_path.dentry;struct bin_attribute *attr =to_bin_attr (dentry); -struct kobject *kobj =to_kobj (dentry->d_parent); +struct sysfs_dirent *sd = dentry->d_parent->d_fsdata; +struct kobject *kobj; +int rc;if (!attr->mmap)return -EINVAL ; -return attr->mmap (kobj, attr, vma); + kobj =sysfs_get_kobj (sd); +if (!kobj) +return -ENODEV ; + +mutex_lock (&bb->mutex); + + rc = attr->mmap (kobj, attr, vma); + +if (rc ==0 && !bb->mmapped) + bb->mmapped =1 ; +else +sysfs_put_kobj (sd); + +mutex_unlock (&bb->mutex); + +return rc; }static int open (struct inode * inode,struct file * file) { -struct kobject *kobj =sysfs_get_kobject (file->f_path.dentry->d_parent); +struct sysfs_dirent *sd =sysfs_get (file->f_path.dentry->d_parent->d_fsdata);struct bin_attribute * attr =to_bin_attr (file->f_path.dentry); +struct bin_buffer *bb =NULL ;int error = -EINVAL ; -if (!kobj || !attr) +if (!attr) goto Done;/* Grab the module reference for this attribute if we have one */
@@ -137,30 +193,41 @@ static int open(struct inode * inode, struct file * file) goto Error; error = -ENOMEM ; - file->private_data =kmalloc (PAGE_SIZE ,GFP_KERNEL ); -if (!file->private_data) + bb =kzalloc (sizeof (*bb),GFP_KERNEL ); +if (!bb) goto Error; + bb->buffer =kmalloc (PAGE_SIZE ,GFP_KERNEL ); +if (!bb->buffer) + goto Error; + +mutex_init (&bb->mutex); + file->private_data = bb; + error =0 ; goto Done; Error: +kfree (bb);module_put (attr->attr.owner); Done:if (error) -kobject_put (kobj); +sysfs_put (sd);return error; }static int release (struct inode * inode,struct file * file) { -struct kobject * kobj =to_kobj (file->f_path.dentry->d_parent); +struct sysfs_dirent * sd = file->f_path.dentry->d_parent->d_fsdata;struct bin_attribute * attr =to_bin_attr (file->f_path.dentry); -u8 * buffer = file->private_data; +struct bin_buffer *bb = file->private_data; -kobject_put (kobj); +if (bb->mmapped) +sysfs_put_kobj (sd); +sysfs_put (sd);module_put (attr->attr.owner); -kfree (buffer); +kfree (bb->buffer); +kfree (bb);return 0 ; }
fs/sysfs/dir.c +115 -19
227 lines not on this page · raw message
fs/sysfs/file.c +68 -90
269 lines not on this page · raw message
fs/sysfs/inode.c +0 -25
@@ -220,24 +220,6 @@ const unsigned char * sysfs_get_name(struct sysfs_dirent *sd)return NULL ; } -static inline void orphan_all_buffers (struct inode *node) -{ -struct sysfs_buffer_collection *set; -struct sysfs_buffer *buf; - -mutex_lock_nested (&node->i_mutex,I_MUTEX_CHILD ); - set = node->i_private; -if (set) { -list_for_each_entry (buf, &set->associates, associates) { -down (&buf->sem); - buf->orphaned =1 ; -up (&buf->sem); - } - } -mutex_unlock (&node->i_mutex); -} - -/* * Unhashes the dentry corresponding to given sysfs_dirent * Called with parent inode's i_mutex held.
@@ -245,23 +227,16 @@ static inline void orphan_all_buffers(struct inode *node)void sysfs_drop_dentry (struct sysfs_dirent * sd,struct dentry * parent) {struct dentry * dentry = sd->s_dentry; -struct inode *inode;if (dentry) {spin_lock (&dcache_lock);spin_lock (&dentry->d_lock);if (!(d_unhashed (dentry) && dentry->d_inode)) { - inode = dentry->d_inode; -spin_lock (&inode->i_lock); -__iget (inode); -spin_unlock (&inode->i_lock);dget_locked (dentry);__d_drop (dentry);spin_unlock (&dentry->d_lock);spin_unlock (&dcache_lock);simple_unlink (parent->d_inode, dentry); -orphan_all_buffers (inode); -iput (inode); }else {spin_unlock (&dentry->d_lock);spin_unlock (&dcache_lock);
fs/sysfs/mount.c +1 -8
@@ -19,15 +19,13 @@ struct vfsmount *sysfs_mount;struct super_block * sysfs_sb =NULL ;struct kmem_cache *sysfs_dir_cachep; -static void sysfs_clear_inode (struct inode *inode); -static const struct super_operations sysfs_ops = { .statfs = simple_statfs, .drop_inode = sysfs_delete_inode, - .clear_inode = sysfs_clear_inode, };static struct sysfs_dirent sysfs_root = { + .s_count =ATOMIC_INIT (1 ), .s_sibling =LIST_HEAD_INIT (sysfs_root.s_sibling), .s_children =LIST_HEAD_INIT (sysfs_root.s_children), .s_element =NULL ,
@@ -35,11 +33,6 @@ static struct sysfs_dirent sysfs_root = { .s_iattr = NULL, }; -static void sysfs_clear_inode (struct inode *inode) -{ -kfree (inode->i_private); -} -static int sysfs_fill_super (struct super_block *sb,void *data,int silent) {struct inode *inode;
fs/sysfs/symlink.c +71 -44
194 lines not on this page · raw message
fs/sysfs/sysfs.h +17 -58
111 lines not on this page · raw message
Quoted 2 lines · Tejun Heo
Okay, here's preliminary patch. I've tested it very lightly so it's likely to contain bugs and definitely needs to be splitted.
Cool. However, there's something fishy there (not sure whether it's in your patch or a latent bug in the ccw bus code that just has been uncovered):
------------[ cut here ]------------ kernel BUG at mm/slab.c:607!
illegal operation: 0001 [#1]
CPU: 1 Not tainted
Process kslowcrw (pid: 36, task: 0000000001a80cc0, ksp:0000000001a87ce0) Krnl PSW : 0404000180000000 00000000000b21b0 (kfree+0x144/0x154) R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:0 CC:0 PM:0 EA:3 Krnl GPRS: 0000000000000000 0000000000000000 00000000009836d8 000003fffd22ffe0 00000000000b20aa 0000000000396ba8 0000000001a6d100 0000000000986c88 0700000001a879d8 0000000000555428 000000000344a5b0 0000000001a879c8 000000000344a500 0000000000390ca8 00000000000b20aa 0000000001a879c8 Krnl Code: 00000000000b21a2: e392c0480024
stg %r9,72(%r2,%r12) 00000000000b21a8: a7f4ffc9 brc
15,1000b213a 00000000000b21ac: a7f40001 brc 15,b21ae
>00000000000b21b0: a7f4ff9d brc 15,1000b20ea
00000000000b21b4: e33030100004 lg %r3,16(%r3)
00000000000b21ba: a7f4ff90 brc 15,1000b20da
00000000000b21be: 0707 bcr 0,%r7
00000000000b21c0: eb8ff0580024 stmg
%r8,%r15,88(%r15) Call Trace:
([<00000000000b20aa>] kfree+0x3e/0x154)
[<00000000001166f2>] release_sysfs_dirent+0x3e/0xf4
[<0000000000114914>] sysfs_hash_and_remove+0x150/0x154
[<0000000000118aec>] remove_files+0x48/0x68
[<0000000000118b84>] sysfs_remove_group+0x78/0xe8
[<00000000001cce3c>] device_remove_groups+0x48/0x68
[<00000000001cd4c0>] device_remove_attrs+0x3c/0x7c
[<00000000001cd70e>] device_del+0x20e/0x3a8
[<00000000001cd8d2>] device_unregister+0x2a/0x44
[<000000000022fb2c>] css_sch_device_unregister+0x3c/0x54
[<0000000000230124>] css_evaluate_subchannel+0x2f0/0x400
[<000000000023041a>] css_trigger_slow_path+0xda/0x154
[<0000000000054c9a>] run_workqueue+0x136/0x1fc
[<0000000000054e3a>] worker_thread+0xda/0x170
[<000000000005b05a>] kthread+0x122/0x15c
[<0000000000019912>] kernel_thread_starter+0x6/0xc
[<000000000001990c>] kernel_thread_starter+0x0/0xcThis happens when I detach a device (which causes it to be unregistered). I'll look at it when I'm fully operational.
Quoted 3 lines · Cornelia Huck
Cool. However, there's something fishy there (not sure whether it's in your patch or a latent bug in the ccw bus code that just has been uncovered):
Similar bug when loading/unloading a module that creates a driver attribute. The winner seems to be kfree(sd->s_element) in release_sysfs_dirent() (in case of an attribute, it will point to the attribute structure, which is usually statically created)...
Quoted 11 lines · Cornelia Huck
Quoted 3 lines · Cornelia Huck
Cool. However, there's something fishy there (not sure whether it's in your patch or a latent bug in the ccw bus code that just has been uncovered):
Similar bug when loading/unloading a module that creates a driver attribute. The winner seems to be
kfree(sd->s_element)inrelease_sysfs_dirent()(in case of an attribute, it will point to the attribute structure, which is usually statically created)...
Thanks for finding it out. I was suspecting that last minute change. The code should be
if (dir node)
kfree(s_element)
else if (symlink node)
do things and kfree()
Thanks.
Signature · 1 line
tejun
Quoted 14 lines · Tejun Heo
Dependencies between sysfs/kobject objects are clearer now and I think I got the locking and referencing correct. This patch immediately fixes 'sysfs attr grabbing the wrong kobject and module' problem - sysfs and module lifetimes are unrelated now. We can drop half-working
attr->owner.* A sysfs node no longer hold reference to its kobject. It just attaches itself to the kobject on creation and detaches on deletion.
* For a dir node,
sysfs_direntno longer directly points to kobject. It points tosysfs_dirwhich contains pointer to kobject and a rwsem to protect it.
Quoted 6 lines
* An open file doesn't hold a reference to kobject. It holds a reference to
sysfs_dirent. kobject pointer is verified and show/store are performed with rwsem read-locked. Deletion disconnects the sysfs from its kobject while the rwsem is write-locked. This mechanism replaces buffer orphaning and kobject validation during open.
Ah, very nice.
Quoted 5 lines
* attr ops is determined on sysfs node creation not on open. This is a step toward making kobject opaque in sysfs.
* bin files are handled similarly but mmap makes the open file hold reference to the kobject till it's closed. Any better ideas?
This is probably needed, and might be acceptable, and I think we can live with it as there is only a very small number of sysfs files that fall into this category.
Quoted 6 lines
* symlink is reimplemented in terms of
sysfs_direntsinstead of kobjects.sysfs_dirent->s_parentis added and eachsysfs_direntholds reference to its parent. Currently walking up the tree requires read locking and unlockingsysfs_dirat each level. This can be removed if name is added tosysfs_dirent.
Quoted 6 lines
* As kobject can be disconnected anytime and sysfs code still needs to look follow dentry link in kobject,
kobject->dentryis protected bydcache_lock. Once kobject becomes opaque to sysfs, this hack can go away. All in all, making kobject completely opaque in sysfs isn't too far away after this patch although it would require mass code update.
What would need to be updated after this?
Quoted 3 lines
What do you think about this approach? If it's acceptable, I'll test further and split the patch into logical steps to get it reviewed better.
At first glance, I think it looks fine, but Maneesh is the one who understands this code better than anyone, so I would like to get his opinion on it.
I think you should start splitting it all up into steps, which will help us review it a whole lot easier, as overall, I think this is a very worthy goal.
thanks so much for doing this work,
greg k-h
Quoted 28 lines · Tejun Heo
Hello, James, Greg.
Quoted 6 lines · James Bottomley
That's sort of what I was reaching for too ... it just looks to me that all the sysfs glue is in kobject, so they make a good candidate for the pure sysfs objects. Whatever we do, there has to be breakable cross linking between the driver's tree and the sysfs representation ... of course, nasty things like ksets get in the way of considering the objects as separate, sigh.
The dependency between kobject and sysfs doesn't seem to be that tight. I managed to break them such that sysfs can disconnect from its kobject on deletion _without_ changing sysfs/kobject API.
Quoted 7 lines
Quoted 4 lines
This way the sysfs reference counting can be put completely out of picture without impacting the rest of code. Handling symlink and suicidal attributes might need some extra attention but I think they can be done.
True ... but you'll also have to implement something within sysfs to do refcounting ... it needs to know how long to keep its nodes around.
sysfs_direntalready had reference counter and plays that role quite nicely.Okay, here's preliminary patch. I've tested it very lightly so it's likely to contain bugs and definitely needs to be splitted.
Quoted 9 lines
Dependencies between sysfs/kobject objects are clearer now and I think I got the locking and referencing correct. This patch immediately fixes 'sysfs attr grabbing the wrong kobject and module' problem - sysfs and module lifetimes are unrelated now. We can drop half-working
attr->owner.* A sysfs node no longer hold reference to its kobject. It just attaches itself to the kobject on creation and detaches on deletion.
Earlier only sysfs leaf nodes held ref to kobject that is also only during open/close for regular files and in create/follow/destroy symlinks.
Quoted 29 lines
* For a dir node,
sysfs_direntno longer directly points to kobject. It points tosysfs_dirwhich contains pointer to kobject and a rwsem to protect it.* An open file doesn't hold a reference to kobject. It holds a reference to
sysfs_dirent. kobject pointer is verified and show/store are performed with rwsem read-locked. Deletion disconnects the sysfs from its kobject while the rwsem is write-locked. This mechanism replaces buffer orphaning and kobject validation during open.* attr ops is determined on sysfs node creation not on open. This is a step toward making kobject opaque in sysfs.
* bin files are handled similarly but mmap makes the open file hold reference to the kobject till it's closed. Any better ideas?
Quoted 1 line
* symlink is reimplemented in terms of
sysfs_direntsinstead ofkobjects.
sysfs_dirent->s_parentis added and eachsysfs_direntholds reference to its parent. Currently walking up the tree requires read locking and unlockingsysfs_dirat each level. This can be removed if name is added tosysfs_dirent.* As kobject can be disconnected anytime and sysfs code still needs to look follow dentry link in kobject,
kobject->dentryis protected bydcache_lock. Once kobject becomes opaque to sysfs, this hack can go away. All in all, making kobject completely opaque in sysfs isn't too far away after this patch although it would require mass code update.
Quoted 4 lines
What do you think about this approach? If it's acceptable, I'll test further and split the patch into logical steps to get it reviewed better.
It does complicate a lot in sysfs code. The code is already quite fragile considering the races (akpm is facing a couple of them, and which I am still working on) from VFS/dentry/inode angle. The previous rework of sysfs was to make dentries/inodes corresponding to sysfs leaf nodes reclaimable, i.e. looking at sysfs from VFS angle and IIUC you are looking at it from the driver layer to solve races between module/driver- layer/sysfs, which is a valid purpose.
Following are the few rules in short to take care
1. directory dentries are pinned
2. regular files and symlink dentries are reclaimable
3. sysfs_dirent tree is protected using the parent inode's (ie directory dentry's inode) i_mutex
4. sysfs rename is protected using a semaphore, sysfs_rename_sem also.
5. sysfs_dirent starts with refcount of 1 at the time of creation, refcount is incremented/decremented when a dentry is attached or detached. So, the refcount at the max can be 2.
6. symlink creation pins the target kobject, so that the corresponding dentry is also pinned and removal releases the kobject.
There were few more additions like sysfs poll and shadow directory support, for which respective authors can review and I can review the VFS related changes but please split the patches as you like but one possible way could be as chunks dealing with changes related to
1. data structure changes
2. creation/removal of directories
3. creation/removal of files
4. creation/removal of symlinks
5. open/read/write/close of a file
As far as testing is considered, doing insmod/rmmod in a tight loop in parallel with working (find, open/read/writhe/close/ file) on /sys, makes a good enough testcase to hit races.
Thanks
Manesh
--
Maneesh Soni
Linux Technology Center,
IBM India Systems and Technology Lab,
Bangalore, India
Quoted 2 lines · --- James Bottomley
I'd favour trying to separate kobject and struct device for this ... move all the sysfs stuff into kobject and device only stuff into struct
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Currently the kobject implementation is pure and well-defined. It is a good implementation [kobject], and I'd hate to see it lost into being convoluted with/into another model.
Currently the infrastructure layers are well defined:
kobject -> (A layer with objects, their behavor and implementation)
device -> (--"--)
sysfs. (--"--)This isn't that bad of an infrastructure.
It is this well defined layering, i.e. objects, their behavior and implementation, that allows different (better/worse) infrastructures to be built on top of it.
It is this well-defined layering which will allow what Tejun wants to be implemented.
Quoted 2 lines
device ... but that would get us into disentangling the ksets, which, on balance, isn't going to be fun ...
Luben
Quoted 25 lines · James Bottomley
Quoted 6 lines · Tejun Heo
Orphaning sysfs nodes on unregistration is a big step in this direction. With sysfs reference counting out of the picture, implementing 'disconnect immediate' interface only on a few components (including
request_queue) should suffice for most block device drivers. I'm not familiar with other drivers but I don't think they'll be very different.I agree with this statement. The big question in my mind is how do we do it?
The essential problem, and the reason why the lifetime rules are entangled is that fundamentally, sysfs entries are managed by kobjects. The things the device drivers are interested in is struct device, which is usually embedded in driver data structures. Unfortunately, the required kobject is usually embedded in struct device meaning that the relevant driver structure cannot be freed while the sysfs entry still exists.
It seems to me that the only way to Orphan the sysfs entries is to separate the kobject and the struct device so their lifetime rules are no longer entangled. Then we can free the driver structure with the embedded struct device while still keeping the necessary kobject around to perform orphaned sysfs operations.
Quoted 7 lines
So it seems to me that what we need to do is to give struct device its own kref and a pointer to a kobject. Then we can manage the separate lifetimes independently. The device would basically allocate and take a reference to the kobject on
device_add()and relinquish it again (and null out the kobject pointer) ondevice_del(). The complexity here is that we now have to allocate the kobject somewhere else ... probably indevice_add()(it doesn't come for free with the device structures).
My plan was to make sysfs more independent from struct device/kobject. e.g. Something like the following.
* kobject_get() on attr/symlink creation
* open() doesn't need extra kobject reference
* deleting a node makes it release the kobject reference and the kobject pointer is nullified.
This way the sysfs reference counting can be put completely out of picture without impacting the rest of code. Handling symlink and suicidal attributes might need some extra attention but I think they can be done.
In the long term, I think sysfs should be independent from driver model and kobject such that an entity which wants to use sysfs but is not a device doesn't have to dance with kobject just to use sysfs.
Thanks.
Signature · 1 line
tejun
Quoted 6 lines · Tejun Heo
Orphaning sysfs nodes on unregistration is a big step in this direction. With sysfs reference counting out of the picture, implementing 'disconnect immediate' interface only on a few components (including
request_queue) should suffice for most block device drivers. I'm not familiar with other drivers but I don't think they'll be very different.
I agree with this statement. The big question in my mind is how do we do it?
The essential problem, and the reason why the lifetime rules are entangled is that fundamentally, sysfs entries are managed by kobjects. The things the device drivers are interested in is struct device, which is usually embedded in driver data structures. Unfortunately, the required kobject is usually embedded in struct device meaning that the relevant driver structure cannot be freed while the sysfs entry still exists.
It seems to me that the only way to Orphan the sysfs entries is to separate the kobject and the struct device so their lifetime rules are no longer entangled. Then we can free the driver structure with the embedded struct device while still keeping the necessary kobject around to perform orphaned sysfs operations.
So it seems to me that what we need to do is to give struct device its own kref and a pointer to a kobject. Then we can manage the separate lifetimes independently. The device would basically allocate and take a reference to the kobject on device_add() and relinquish it again (and null out the kobject pointer) on device_del(). The complexity here is that we now have to allocate the kobject somewhere else ... probably in device_add() (it doesn't come for free with the device structures).
James
Quoted 5 lines · Tejun Heo
One way to solve this problem is to subordinate lifetime rule #b to rule #c. Each kobject points to its owning module such that grabbing a kobject automatically grabs the module. The problem with this approach is that it requires wide update and makes
kobject_getheavier.
Shouldn't getting/putting the module refcount be solely done in kobject.c? Grab the module reference when the kobject is created and release the module reference in kobject_cleanup() after the release function has been called. This doesn't make kobject_get() heavier, and it ensures we don't delete the module until after the last kobject it is supposed to clean up has been released.
Quoted 9 lines · Cornelia Huck
Quoted 5 lines · Tejun Heo
One way to solve this problem is to subordinate lifetime rule #b to rule #c. Each kobject points to its owning module such that grabbing a kobject automatically grabs the module. The problem with this approach is that it requires wide update and makes
kobject_getheavier.
Quoted 6 lines
Shouldn't getting/putting the module refcount be solely done in
kobject.c? Grab the module reference when the kobject is created and release the module reference inkobject_cleanup()after the release function has been called. This doesn't makekobject_get()heavier, and it ensures we don't delete the module until after the last kobject it is supposed to clean up has been released.
If we do that, we wouldn't be able to unload a module if there is any kobject referencing it even when the node has no openers, so no easy way out there. :-(
Signature · 1 line
tejun
Quoted 10 lines · Tejun Heo
Quoted 6 lines
Shouldn't getting/putting the module refcount be solely done in
kobject.c? Grab the module reference when the kobject is created and release the module reference inkobject_cleanup()after the release function has been called. This doesn't makekobject_get()heavier, and it ensures we don't delete the module until after the last kobject it is supposed to clean up has been released.If we do that, we wouldn't be able to unload a module if there is any kobject referencing it even when the node has no openers, so no easy way out there. :-(
But we must not unload the module when there is still a kobject around referencing a release function in the module, or we will oops if the kobject is finally released. If needed, the module must clean up its kobjects in its exit function.
Quoted 9 lines · Cornelia Huck
Quoted 6 lines · Tejun Heo
Quoted 6 lines
Shouldn't getting/putting the module refcount be solely done in
kobject.c? Grab the module reference when the kobject is created and release the module reference inkobject_cleanup()after the release function has been called. This doesn't makekobject_get()heavier, and it ensures we don't delete the module until after the last kobject it is supposed to clean up has been released.
Quoted 8 lines
Quoted 3 lines
If we do that, we wouldn't be able to unload a module if there is any kobject referencing it even when the node has no openers, so no easy way out there. :-(
But we must not unload the module when there is still a kobject around referencing a release function in the module, or we will oops if the kobject is finally released. If needed, the module must clean up its kobjects in its exit function.
It's a little bit more convoluted than that. Module reference count of zero doesn't indicate that there is no one referencing the module. It just means that the module can be unloaded. ie. There still can be any number of kobjects with release function backed by the module but as long as all of them can be deleted and released by module exit function, the module is unloadable at that point.
IOW, module reference count does not count number of objects depending on the module. It counts the number of active usages of those objects.
Signature · 1 line
tejun
Quoted 9 lines · Tejun Heo
It's a little bit more convoluted than that. Module reference count of zero doesn't indicate that there is no one referencing the module. It just means that the module can be unloaded. ie. There still can be any number of kobjects with release function backed by the module but as long as all of them can be deleted and released by module exit function, the module is unloadable at that point.
IOW, module reference count does not count number of objects depending on the module. It counts the number of active usages of those objects.
We must make sure that the module is never deleted while there may be calls to ->release functions - the exit function can only return when all ->release calls have returned. This can be guaranteed if we (1) don't allow the module to unload if there are outstanding kobjects (we may need a "self destruct" knob then) or (2) make sure the ->release functions are outside of the module (see, for example, drivers/s390/s390_rdev.c).
(Gah, that stuff is always giving me headaches. Sorry if I'm not making sense...)
Quoted 13 lines · Cornelia Huck
Quoted 9 lines · Tejun Heo
It's a little bit more convoluted than that. Module reference count of zero doesn't indicate that there is no one referencing the module. It just means that the module can be unloaded. ie. There still can be any number of kobjects with release function backed by the module but as long as all of them can be deleted and released by module exit function, the module is unloadable at that point.
IOW, module reference count does not count number of objects depending on the module. It counts the number of active usages of those objects.
Quoted 7 lines
We must make sure that the module is never deleted while there may be calls to ->release functions - the exit function can only return when all ->release calls have returned. This can be guaranteed if we (1) don't allow the module to unload if there are outstanding kobjects (we may need a "self destruct" knob then) or (2) make sure the ->release functions are outside of the module (see, for example,
drivers/s390/s390_rdev.c).
(3) make sure all existing kobjects are released by module exit function.
For example, let's say there is a hypothetical disk device /dev/dk0 driven by a hypothetical driver mydrv. /dev/dk0 is represented like the following in the sysfs tree.
/sys/devices/pci0000:00/0000:00:1f.0/dk0/{myknob0,myknob1}
Owner of both attrs myknob0 and myknob1 is mydrv and opening either increases the reference counts of dk0 and mydrv and closing does the opposite.
* When there is no opener of either knob and the /dev/dk0 isn't used by anyone. Reference count of dk0 is 1, mydrv 0.
* User issues rmmod mydrv. As mydrv's reference count is zero, unload proceeds and mydrv's exit function is called.
* mydrv's exit function looks like the following.
mydrv_exit()
{
sysfs_remove_file(dk0, myknob0);
sysfs_remove_file(dk1, myknob1);
device_del(dk0);
deinit controller;
release all resources;}
The device_del(dk0) drops dk0's reference count to zero and its ->release is invoked immediately.
This method is widely used to allow modules to be unloaded even when there still are valid objects if there's no active user.
Quoted 2 lines
(Gah, that stuff is always giving me headaches. Sorry if I'm not making sense...)
Yeap, this is confusing. Hope my explanation makes sense.
Signature · 1 line
tejun
Quoted 6 lines · Tejun Heo
(3) make sure all existing kobjects are released by module exit function.
For example, let's say there is a hypothetical disk device
/dev/dk0driven by a hypothetical driver mydrv./dev/dk0is represented like the following in the sysfs tree.
Quoted 8 lines
/sys/devices/pci0000:00/0000:00:1f.0/dk0/{myknob0,myknob1}
Owner of both attrs myknob0 and myknob1 is mydrv and opening either increases the reference counts of dk0 and mydrv and closing does the opposite.
* When there is no opener of either knob and the
/dev/dk0isn't used by anyone. Reference count of dk0 is 1, mydrv 0.
Hm, but as long as dk0 is registered, it can be looked up and someone could get a reference on it.
Quoted 13 lines
* User issues rmmod mydrv. As mydrv's reference count is zero, unload proceeds and mydrv's exit function is called.
* mydrv's exit function looks like the following.
mydrv_exit(){ sysfs_remove_file(dk0, myknob0); sysfs_remove_file(dk1, myknob1); device_del(dk0); deinit controller; release all resources;
Quoted 4 lines
}
The
device_del(dk0)drops dk0's reference count to zero and its ->release is invoked immediately.
And here is the problem if someone else still has a reference. The module will be unloaded, but ->release will not be called until the "someone else" gives up the reference...
Quoted 15 lines · Cornelia Huck
Quoted 12 lines · Tejun Heo
(3) make sure all existing kobjects are released by module exit function.
For example, let's say there is a hypothetical disk device
/dev/dk0driven by a hypothetical driver mydrv./dev/dk0is represented like the following in the sysfs tree./sys/devices/pci0000:00/0000:00:1f.0/dk0/{myknob0,myknob1}
Owner of both attrs myknob0 and myknob1 is mydrv and opening either increases the reference counts of dk0 and mydrv and closing does the opposite.
Quoted 5 lines
Quoted 2 lines
* When there is no opener of either knob and the
/dev/dk0isn't used by anyone. Reference count of dk0 is 1, mydrv 0.Hm, but as long as dk0 is registered, it can be looked up and someone could get a reference on it.
Yeah, exactly. That's why any getting any kobject reference backed by a module must be accompanied by try_module_get().
int mydrv_get_dk(struct dk *dk)
{
rc = try_module_get(mydrv);
if (rc)
return rc;
kobject_get(&dk->kobj);
return 0;
}Quoted 14 lines
Quoted 14 lines
* User issues rmmod mydrv. As mydrv's reference count is zero, unload proceeds and mydrv's exit function is called.
* mydrv's exit function looks like the following.
mydrv_exit(){ sysfs_remove_file(dk0, myknob0); sysfs_remove_file(dk1, myknob1); device_del(dk0); deinit controller; release all resources;}
Quoted 6 lines
Quoted 2 lines
The
device_del(dk0)drops dk0's reference count to zero and its ->release is invoked immediately.And here is the problem if someone else still has a reference. The module will be unloaded, but ->release will not be called until the "someone else" gives up the reference...
Exactly, in that case, module reference count must not be zero. You and I are saying the same thing. Why are we running in circle?
Signature · 1 line
tejun
Quoted 24 lines · Tejun Heo
Quoted 11 lines · Cornelia Huck
Quoted 8 lines · Tejun Heo
(3) make sure all existing kobjects are released by module exit function.
For example, let's say there is a hypothetical disk device
/dev/dk0driven by a hypothetical driver mydrv./dev/dk0is represented like the following in the sysfs tree./sys/devices/pci0000:00/0000:00:1f.0/dk0/{myknob0,myknob1}
Quoted 8 lines
Quoted 6 lines
Owner of both attrs myknob0 and myknob1 is mydrv and opening either increases the reference counts of dk0 and mydrv and closing does the opposite.
* When there is no opener of either knob and the
/dev/dk0isn't used by anyone. Reference count of dk0 is 1, mydrv 0.Hm, but as long as dk0 is registered, it can be looked up and someone could get a reference on it.
Yeah, exactly. That's why any getting any kobject reference backed by a module must be accompanied by
try_module_get().
Quoted 8 lines
int
mydrv_get_dk(struct dk *dk){ rc = try_module_get(mydrv); if (rc) return rc; kobject_get(&dk->kobj); return 0; }
And one more thing just in case. In the above code, try_module_get() and kobject_get() must be and is atomic w.r.t. try_stop_module(). That's why we do the following.
stop_machine_run(__try_stop_module, &sref, NR_CPUS);.
Signature · 1 line
tejun
Quoted 6 lines · Tejun Heo
Quoted 2 lines
Hm, but as long as dk0 is registered, it can be looked up and someone could get a reference on it.
Yeah, exactly. That's why any getting any kobject reference backed by a module must be accompanied by
try_module_get().
Quoted 8 lines
int
mydrv_get_dk(struct dk *dk){ rc = try_module_get(mydrv); if (rc) return rc; kobject_get(&dk->kobj); return 0; }
This works if the caller always knows which module to grab (I was thinking about some tree-walking code).
Quoted 2 lines
Exactly, in that case, module reference count must not be zero. You and I are saying the same thing. Why are we running in circle?
I hope we're not, it just makes one dizzy :)
I'll think some more about it...
Quoted 17 lines · Cornelia Huck
Quoted 5 lines · Tejun Heo
Quoted 2 lines
Hm, but as long as dk0 is registered, it can be looked up and someone could get a reference on it.
Yeah, exactly. That's why any getting any kobject reference backed by a module must be accompanied by
try_module_get().Quoted 8 lines
int
mydrv_get_dk(struct dk *dk){ rc = try_module_get(mydrv); if (rc) return rc; kobject_get(&dk->kobj); return 0; }
Quoted 2 lines
This works if the caller always knows which module to grab (I was thinking about some tree-walking code).
Yeah, that's why we have so many ->owner's and getting it wrong results in jumping to non-existent address.
Signature · 1 line
tejun
Quoted 16 lines · --- Tejun Heo
Quoted 15 lines · Cornelia Huck
Quoted 12 lines · Tejun Heo
(3) make sure all existing kobjects are released by module exit function.
For example, let's say there is a hypothetical disk device
/dev/dk0driven by a hypothetical driver mydrv./dev/dk0is represented like the following in the sysfs tree./sys/devices/pci0000:00/0000:00:1f.0/dk0/{myknob0,myknob1}
Owner of both attrs myknob0 and myknob1 is mydrv and opening either increases the reference counts of dk0 and mydrv and closing does the opposite.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
* When there is no opener of either knob and the
/dev/dk0isn't used by anyone. Reference count of dk0 is 1, mydrv 0.Hm, but as long as dk0 is registered, it can be looked up and someone could get a reference on it.
Yeah, exactly. That's why any getting any kobject reference backed by a module must be accompanied by
try_module_get().
Tejun,
This has always been the case.
By design, the kobject infrastructure is one such that allows for different behavior implementations. You could use it to accomplish what you want and describe in your RFD, or you could use it to implement what we have today or you could use it to implement completely different (refcount) infrastructure.
Luben
Quoted 41 lines
int
mydrv_get_dk(struct dk *dk){ rc = try_module_get(mydrv); if (rc) return rc; kobject_get(&dk->kobj); return 0; }Quoted 14 lines
Quoted 14 lines
* User issues rmmod mydrv. As mydrv's reference count is zero, unload proceeds and mydrv's exit function is called.
* mydrv's exit function looks like the following.
mydrv_exit(){ sysfs_remove_file(dk0, myknob0); sysfs_remove_file(dk1, myknob1); device_del(dk0); deinit controller; release all resources;}
Quoted 6 lines
Quoted 2 lines
The
device_del(dk0)drops dk0's reference count to zero and its ->release is invoked immediately.And here is the problem if someone else still has a reference. The module will be unloaded, but ->release will not be called until the "someone else" gives up the reference...
Exactly, in that case, module reference count must not be zero. You and I are saying the same thing. Why are we running in circle?
Signature · 5 lines
tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 13 lines · Cornelia Huck
Quoted 9 lines · Tejun Heo
It's a little bit more convoluted than that. Module reference count of zero doesn't indicate that there is no one referencing the module. It just means that the module can be unloaded. ie. There still can be any number of kobjects with release function backed by the module but as long as all of them can be deleted and released by module exit function, the module is unloadable at that point.
IOW, module reference count does not count number of objects depending on the module. It counts the number of active usages of those objects.
Quoted 7 lines
We must make sure that the module is never deleted while there may be calls to ->release functions - the exit function can only return when all ->release calls have returned. This can be guaranteed if we (1) don't allow the module to unload if there are outstanding kobjects (we may need a "self destruct" knob then) or (2) make sure the ->release functions are outside of the module (see, for example,
drivers/s390/s390_rdev.c).
(3) make sure all existing kobjects are released by module exit function.
For example, let's say there is a hypothetical disk device /dev/dk0 driven by a hypothetical driver mydrv. /dev/dk0 is represented like the following in the sysfs tree.
/sys/devices/pci0000:00/0000:00:1f.0/dk0/{myknob0,myknob1}
Owner of both attrs myknob0 and myknob1 is mydrv and opening either increases the reference counts of dk0 and mydrv and closing does the opposite.
* When there is no opener of either knob and the /dev/dk0 isn't used by anyone. Reference count of dk0 is 1, mydrv 0.
* User issues rmmod mydrv. As mydrv's reference count is zero, unload proceeds and mydrv's exit function is called.
* mydrv's exit function looks like the following.
mydrv_exit()
{
sysfs_remove_file(dk0, myknob0);
sysfs_remove_file(dk1, myknob1);
device_del(dk0);
deinit controller;
release all resources;}
The device_del(dk0) drops dk0's reference count to zero and its ->release is invoked immediately.
This method is widely used to allow modules to be unloaded even when there still are valid objects if there's no active user.
Quoted 2 lines
(Gah, that stuff is always giving me headaches. Sorry if I'm not making sense...)
Yeap, this is confusing. Hope my explanation makes sense.
Signature · 1 line
tejun
Quoted 9 lines · Tejun Heo
It's a little bit more convoluted than that. Module reference count of zero doesn't indicate that there is no one referencing the module. It just means that the module can be unloaded. ie. There still can be any number of kobjects with release function backed by the module but as long as all of them can be deleted and released by module exit function, the module is unloadable at that point.
IOW, module reference count does not count number of objects depending on the module. It counts the number of active usages of those objects.
We must make sure that the module is never deleted while there may be calls to ->release functions - the exit function can only return when all ->release calls have returned. This can be guaranteed if we (1) don't allow the module to unload if there are outstanding kobjects (we may need a "self destruct" knob then) or (2) make sure the ->release functions are outside of the module (see, for example, drivers/s390/s390_rdev.c).
(Gah, that stuff is always giving me headaches. Sorry if I'm not making sense...)
Quoted 9 lines · Cornelia Huck
Quoted 6 lines · Tejun Heo
Quoted 6 lines
Shouldn't getting/putting the module refcount be solely done in
kobject.c? Grab the module reference when the kobject is created and release the module reference inkobject_cleanup()after the release function has been called. This doesn't makekobject_get()heavier, and it ensures we don't delete the module until after the last kobject it is supposed to clean up has been released.
Quoted 8 lines
Quoted 3 lines
If we do that, we wouldn't be able to unload a module if there is any kobject referencing it even when the node has no openers, so no easy way out there. :-(
But we must not unload the module when there is still a kobject around referencing a release function in the module, or we will oops if the kobject is finally released. If needed, the module must clean up its kobjects in its exit function.
It's a little bit more convoluted than that. Module reference count of zero doesn't indicate that there is no one referencing the module. It just means that the module can be unloaded. ie. There still can be any number of kobjects with release function backed by the module but as long as all of them can be deleted and released by module exit function, the module is unloadable at that point.
IOW, module reference count does not count number of objects depending on the module. It counts the number of active usages of those objects.
Signature · 1 line
tejun
Quoted 10 lines · Tejun Heo
Quoted 6 lines
Shouldn't getting/putting the module refcount be solely done in
kobject.c? Grab the module reference when the kobject is created and release the module reference inkobject_cleanup()after the release function has been called. This doesn't makekobject_get()heavier, and it ensures we don't delete the module until after the last kobject it is supposed to clean up has been released.If we do that, we wouldn't be able to unload a module if there is any kobject referencing it even when the node has no openers, so no easy way out there. :-(
But we must not unload the module when there is still a kobject around referencing a release function in the module, or we will oops if the kobject is finally released. If needed, the module must clean up its kobjects in its exit function.
Quoted 9 lines · Cornelia Huck
Quoted 5 lines · Tejun Heo
One way to solve this problem is to subordinate lifetime rule #b to rule #c. Each kobject points to its owning module such that grabbing a kobject automatically grabs the module. The problem with this approach is that it requires wide update and makes
kobject_getheavier.
Quoted 6 lines
Shouldn't getting/putting the module refcount be solely done in
kobject.c? Grab the module reference when the kobject is created and release the module reference inkobject_cleanup()after the release function has been called. This doesn't makekobject_get()heavier, and it ensures we don't delete the module until after the last kobject it is supposed to clean up has been released.
If we do that, we wouldn't be able to unload a module if there is any kobject referencing it even when the node has no openers, so no easy way out there. :-(
Signature · 1 line
tejun
Quoted 5 lines · Tejun Heo
One way to solve this problem is to subordinate lifetime rule #b to rule #c. Each kobject points to its owning module such that grabbing a kobject automatically grabs the module. The problem with this approach is that it requires wide update and makes
kobject_getheavier.
Shouldn't getting/putting the module refcount be solely done in kobject.c? Grab the module reference when the kobject is created and release the module reference in kobject_cleanup() after the release function has been called. This doesn't make kobject_get() heavier, and it ensures we don't delete the module until after the last kobject it is supposed to clean up has been released.
Quoted 8 lines · Tejun Heo
Hello, all.
This document tries to describe lifetime problems of the current device driver model primarily from the point view of device drivers and establish consensus, or at least, start discussion about how to solve these problems. This is primarily based on my experience with IDE and SCSI layers and my knowledge on other drivers is limited, so I might have generalized too aggressively. Feel free to point out.
Very nice summary of the problems we are trying to address here, thanks for doing this.
Quoted 10 lines
2. How do we solve this?
2-1. Rule #a and #b
If you think about it, the impedance matching between rule #a and #b should be done somewhere. Maybe doing it in each low level driver is what's intended when the driver model is designed but it's impractically painful and the reality reflects that by doing it in midlayers.
Quoted 8 lines
It's better if we do it in higher layers. For example, SCSI has a mechanism to reject new requests and drain
request_queueto allow low level driver to just unregister an existing device. IDE currently doesn't have such feature but it would need to do almost the same thing to support hotplug. Ifrequest_queuejust had a feature to drain and kill itself, both SCSI and IDE could use it. It would be simpler and less error-prone. On the other hand, if it's pushed downward, it will cause much more pain in all the low level drivers.
No, we should not push this kind of thing down to the lower drivers.
In fact, my goal is to try to keep a "normal" driver writer from having to know anything about the driver model, except for how to add and remove a sysfs file that is bound to the device controlled by the driver.
And for the majority of the busses, this is true, it's only people like you who are mucking around in the bus specific code that know how hard this whole thing is :)
Quoted 16 lines
Orphaning sysfs nodes on unregistration is a big step in this direction. With sysfs reference counting out of the picture, implementing 'disconnect immediate' interface only on a few components (including
request_queue) should suffice for most block device drivers. I'm not familiar with other drivers but I don't think they'll be very different.All in all, I'm hoping something like the following can be done in device drivers, midlayer or low level.
* For binding
alloc resources;
init controller;
register to upper layers;
Quoted 7 lines
* For unbinding
unregister from upper layers; (no lingering references or objects) deinit controller;
release resources;This basically nullifies lifetime rule #b from the POV of drivers.
I agree, that should be our goal. And for almost all busses, this is true today, right?
Quoted 7 lines
2-2. Rule #b and #c
One way to solve this problem is to subordinate lifetime rule #b to rule #c. Each kobject points to its owning module such that grabbing a kobject automatically grabs the module. The problem with this approach is that it requires wide update and makes
kobject_getheavier.
Why would kobject_get become heavier?
Quoted 6 lines
Another more appealing approach is to do nothing. Solving the problem between rule #a and #b in the way described above means virtually nullifying rule #b. With rule #b gone, rule #c can't conflict with it. IOW, no reference from upper layer would be remaining after a driver unregisters itself from the upper layer - the module can be safely unloaded.
Yes, but you have to watch out for devices that add their own files with callbacks to that driver. That is a problem today that Oliver has tried to address with his recent patches.
Quoted 4 lines
3. How do we get there from here?
The current device driver model is used by most device drivers - huge amount of code. We can't update them at once.
No, the majority of the logic is in the busses, and we can update them pretty easily.
Quoted 9 lines
Fortunately, the
suggested solution can easily be implemented gradually. We can add 'disconnect now' interface to upper driver interface one-by-one and convert its users gradually. The existing reference counting mechanism can be left alone and used to verify that the conversion is correct by verifying reference count is 0 after unregistering.Once the conversion is complete (maybe after a year), we can remove the reference counting mechanism from device driver interface.
I'd be interested in seeing how we can do this. Do you have any specific patches started in this area?
Quoted 7 lines
4. Some afterthoughts
* Doing this would make module reference counting more flexible. Instead of being confined by implementation, it can be used to define when to allow and disallow module unloading. (you can't unload a block driver module if a fs on it is mounted but sysfs access doesn't matter.)
Well, remember, module unloading is racy and unrecommended to use anyway :)
Also, look at the network drivers who all have their own special type of lock, with no module reference count increment when they are in use. Is this what you want to do for everything else?
Quoted 6 lines
* Outside of device driver model, kobject is currently used to supply something sysfs nodes can be attached to, such that both the information supplier and node users have something common to reference count. With orphaning added, there is no need to use kobject this way. IMHO, kobject should have been implementation detail inside the device model proper in the first place.
Well, kobject was created because Al Viro wanted to use them for the block devices and it didn't make sense to use a 'struct device' for them at the time. Also, the class code wasn't even around then (this is _very_ early 2.5 timeframe.
So yes, any cleanups and refactoring that we can do in this area would be very good and welcome. Kay has had some ideas for how we can clean up the whole subsystem/kset mess too. Hopefully something comes of that soon.
thanks,
greg k-h