[RFC] Potential kobject functionality (two stage delete, single delete)

[RFC] Potential kobject functionality (two stage delete, single delete)

[Kent Overstreet]

I've been working on reference counting in my own code, and it seemed to 
me that some of this stuff would be best added to the generic code - I 
can't be the only one who's needed to solve these particular problems. 
But kobjects aren't new, maybe someone knows if any of this has been 
tried before?

The easy one would be a flag marking an object as deleted when there's 
still references. The idea is if you've got multiple unserialized ways 
of closing/deleting something - error handling paths and/or a way for a 
user to request that it be closed - you need to make sure you drop only 
one reference.

This is trivial to handle without adding to the kobject code, but it 
seems to me it ought to be common enough to warrant adding it - I 
wouldn't be surprised if there's driver code that doesn't handle it 
correctly, it's easy enough to miss if you don't think about the 
particular case. We need to add an atomic bitflag to struct kobject; 
another callback in struct kobj_type might be useful too. Then something 
like the following should be it:

void kobject_delete(struct kobject *k)
{
	if (!test_and_set_bit(deleted)) {
		if (delete_fn)
			delete_fn(k);
		kobject_put(k);
	}
}

The more annoying one is two stage delete. Unless my google-fu has 
failed me, I don't see a reasonable way of using kobject refcounting if 
you need to drop a refcount from atomic context. Without modifying the 
kobject code, I'd have to have a second refcount and combined with RCU 
things become an unreadable mess of callbacks.

I think all that would be needed would be to add a flag to kobj_type 
indicating that the release function will call kobject_cleanup; the 
release function would then be free to punt to a workqueue.

The only potential problem I see is that either the code to remove a 
kobject from sysfs has to be made safe for atomic context, or you'd have 
dead kobjects sitting in sysfs an arbitrary amount of time - it looks 
like if this was a real issue it would be with the current code too 
though, if a callback for an attribute could take a reference to a kobject.

Comments? I'll work up some patches if no one convinces me either idea's 
insane.

Re: [RFC] Potential kobject functionality (two stage delete, single delete)

[Greg KH]

On Tue, Oct 05, 2010 at 06:23:19AM -0700, Kent Overstreet wrote:
> I've been working on reference counting in my own code, and it
> seemed to me that some of this stuff would be best added to the
> generic code - I can't be the only one who's needed to solve these
> particular problems. But kobjects aren't new, maybe someone knows if
> any of this has been tried before?

Oh yeah, it's come up lots of times before, see the lkml archives :)

> The easy one would be a flag marking an object as deleted when
> there's still references. The idea is if you've got multiple
> unserialized ways of closing/deleting something - error handling
> paths and/or a way for a user to request that it be closed - you
> need to make sure you drop only one reference.

Yes, very common.

> This is trivial to handle without adding to the kobject code,

Yes, and in the end, it should be handled by the code needing it, not in
the kobject code itself.  That's why it hasn't been added.

> but it
> seems to me it ought to be common enough to warrant adding it - I
> wouldn't be surprised if there's driver code that doesn't handle it
> correctly, it's easy enough to miss if you don't think about the
> particular case. We need to add an atomic bitflag to struct kobject;
> another callback in struct kobj_type might be useful too. Then
> something like the following should be it:
> 
> void kobject_delete(struct kobject *k)
> {
> 	if (!test_and_set_bit(deleted)) {
> 		if (delete_fn)
> 			delete_fn(k);
> 		kobject_put(k);
> 	}
> }

Every time we have tried to do something like this, it ends up not being
correct, and missused, so we don't.

> The more annoying one is two stage delete. Unless my google-fu has
> failed me, I don't see a reasonable way of using kobject refcounting
> if you need to drop a refcount from atomic context.

You can't call kfree from atomic context?

Anyway, code does handle this properly, look at the scsi code for
example, we have a waitqueue-like infrastructure to do this somewhere,
perhaps it's within the driver core, I can't remember it this early in
the morning.

thanks,

greg k-h

Re: [RFC] Potential kobject functionality (two stage delete, single delete)

[Kent Overstreet]

On 10/05/2010 06:57 AM, Greg KH wrote:
> On Tue, Oct 05, 2010 at 06:23:19AM -0700, Kent Overstreet wrote:
>> I've been working on reference counting in my own code, and it
>> seemed to me that some of this stuff would be best added to the
>> generic code - I can't be the only one who's needed to solve these
>> particular problems. But kobjects aren't new, maybe someone knows if
>> any of this has been tried before?
>
> Oh yeah, it's come up lots of times before, see the lkml archives :)

Figures :)

>> void kobject_delete(struct kobject *k)
>> {
>> 	if (!test_and_set_bit(deleted)) {
>> 		if (delete_fn)
>> 			delete_fn(k);
>> 		kobject_put(k);
>> 	}
>> }
>
> Every time we have tried to do something like this, it ends up not being
> correct, and missused, so we don't.

Well, past experience is hard to argue with. I'd be curious what 
previous implementations looked like, hopefully my google-fu is stronger 
this time... I just have a hard time seeing a good reason not do it once 
correctly, if previous interfaces were prone to misuse it still ought to 
be possible to do it right.

>> The more annoying one is two stage delete. Unless my google-fu has
>> failed me, I don't see a reasonable way of using kobject refcounting
>> if you need to drop a refcount from atomic context.
>
> You can't call kfree from atomic context?

Well, kobject_cleanup() does more than kfree() - thus I don't see how 
you'd use kobject_put() in atomic context; it seems to me it wouldn't be 
entirely unreasonable to replace kobject_put with a kref_put wrapper 
specific to your code, then you could queue up the object somewhere and 
run kobject_cleanup() yourself - except kobject_cleanup() is static.

So unless I've completely missed something, you have to use 
kobject_put() to free a kobject, but kobject_put() can't be called from 
atomic context - at least if the kobject was present in sysfs... perhaps 
that's where the confusion comes from? Going over the code again it 
looks like kobject_cleanup() doesn't do anything but kfree() if the 
kobject wasn't in sysfs.

Anyways, in that case the end result is I need my own refcount so when 
it goes to 0 I can do the right thing - the kobject's refcount then 
serves no purpose, it's just pointless duplication. Am I making any more 
sense now?

> Anyway, code does handle this properly, look at the scsi code for
> example, we have a waitqueue-like infrastructure to do this somewhere,
> perhaps it's within the driver core, I can't remember it this early in
> the morning.

I'm not arguing it can't be done, just would like something cleaner than 
what I've got now :)

Grepping around for kobject in drivers/scsi and elsewhere isn't getting 
me anything, will see where googling gets me...

> thanks,
>
> greg k-h

Sorry to have to impose upon your time :) Thanks!