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

[Greg KH]

On Sun, Jan 29, 2006 at 02:58:51PM -0700, Eric W. Biederman wrote:
> Greg KH <greg@xxxxxxxxx> writes:
> 
> > On Sun, Jan 29, 2006 at 12:22:34AM -0700, Eric W. Biederman wrote:
> >> +struct task_ref
> >> +{
> >> +	atomic_t count;
> >
> > Please use a struct kref here, instead of your own atomic_t, as that's
> > why it is in the kernel :)
> >
> >> +	enum pid_type type;
> >> +	struct task_struct *task;
> >> +};
> >
> > thanks,
> 
> I would rather not. Whenever I look at struct kref it seems to be an over
> abstraction, and as such I find it confusing to work with.  I know
> whenever I look at the sysfs code I have to actively remind myself
> that the kref in the structure is not a pointer to a kref.
> 
> 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.

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/