Re: [PATCH 1/5] pid: Implement task references.

[ebiederm@xmission.com (Eric W. Biederman)]

Greg KH <greg@kroah.com> writes:

> On Mon, Jan 30, 2006 at 01:13:12PM -0700, Eric W. Biederman wrote:
>
> What function would you have to add?  A release function?  You should
> have that logic somewhere already, so net gain should be the same.

Yes inline in my if statement all 3 lines of it. Adding a function
makes the code less clear.

>> The extra debugging checks in krefs are nice, but not something I
>> am lusting over.
>
> Heh, but they do catch a lot of improper usages.

Agreed.

>> As for code recognition I don't see how recognizing the atomic_t idiom
>> versus the kref idiom is much different.  But I haven't had this issue
>> come up in a code review so I have no practical experience there.
>
> It's not a different "idiom", but rather, a way to implement the
> reference counting logic without having to code it all by hand, and
> ensure that you get it correct.

Yes a way to implement the reference counting logic that requires more
lines of code to implement and obscures my code in this instance.

> They are used all the time, and there's not that many atomic_t usages in
> the kernel that do reference counting that have not been already
> converted to use kref (the vfs not-withstanding, for other reasons.)

Well either that is all code that is still to be merged in the
mainline or I am looking with a very different filter than you are.

A quick grep for atomic_t in the kernel include files (filtering
out the asm headers where it was defined) I got several screen fulls
of hits.  Things like the network stack, shared process resources are
places I have worked lately and where atomic_t is used, all of the
core bits of the kernel.  My similar grep for kref should barely returned
a screen full of hits.

>> So those are all of the nits I can pick. :)  I don't kref seems to be
>> a perfectly usable piece of code but not one that seems to help my
>> code.  Unless it becomes a mandate from the code correctness police.
>
> As you are adding a new reference count, it is highly encouraged that
> you not reimplement the same logic, but rather use the functions already
> provided in the kernel to do that work for you.  That way you don't have
> to write the extra code, and we don't have to check the logic for you.

If there was something to implement I would agree with you.  In drivers
out at the fringes of the kernel where looking at some of the code submissions
you wonder if the implementors know C I can totally understand anything
to make things less error prone.  But that isn't what this code is.

Given how short my code is and what the effects are I am actually
disappointed that the review only got as far as noticing I was using
an atomic_t.  Oh well.  Thanks for getting that far.

Eric