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

[Greg KH]

On Mon, Jan 30, 2006 at 01:13:12PM -0700, Eric W. Biederman wrote:
> Greg KH <greg@xxxxxxxxx> 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. 

What function would you have to add?  A release function?  You should
have that logic somewhere already, so net gain should be the same.

> 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.

> 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.

> 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. 

Yes.  You are "getting" and "putting" your structure, without having to
handle the reference count.  I don't see the issue here...

> It also doesn't help that current krefs are used in little enough code
> that I still find it jarring to see one.

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.)

> One more abstraction to read.  Whereas atomic_t are everywhere, so I
> am familiar with them.

They should not be "everywhere" as reference counts, except for the vm
and mm cores.

> 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.

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/