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

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

Greg KH <greg@kroah.com> writes:

> On Sun, Jan 29, 2006 at 02:58:51PM -0700, Eric W. Biederman wrote:
>> 
>> What does the kref abstraction buy?  How does it simplify things?
>> We already have equivalent functions in atomic_t on which it is built.
>
> It ensures that you get the logic of the reference counting correctly.
> It forces you to do the logic of the get and put and release properly.
>
> To roughly quote Andrew Morton, "When I see a kref, I know it is used
> properly, otherwise I am forced to read through the code to see if the
> author got the reference counting logic correct."
>
> It costs _nothing_ to use it, and let's everyone know you got the logic
> correct.
>
> So don't feel it is a "abstraction", it's a helper for both the author
> (who doesn't have to get the atomic_t calls correct), and for everyone
> else who has to read the code.

So looking at kref and looking at my code I have a small amount of feedback.
It looks like using kref would increase the size of my code as I would
have to add an additional function.  Further I don't see that it would
simplify anything in my code. 

The extra debugging checks in krefs are nice, but not something I
am lusting over.

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.

The biggest issue I can find with kref is the name.  kref is not a
reference it is a count.  A reference count if you want to be long
about it. Just skimming code using it looks like kref is a pointer to
a generic object that you are getting and putting. 

It also doesn't help that current krefs are used in little enough code
that I still find it jarring to see one.  One more abstraction to
read.  Whereas atomic_t are everywhere, so I am familiar with them.

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.

Eric