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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Greg KH a écrit :
On Sun, Jan 29, 2006 at 02:58:51PM -0700, Eric W. Biederman wrote:
Greg KH <[email protected]> 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.


kref abstraction is good.

Its current implementation seems straightforward, so I understand some devs think they can 'just use an atomic_t' themselves.

But using kref is better because some generic improvement could be done at kref level and you dont need to parse all kernel sources to change atomic_t to kref...

Example of improvement in kref_put() :

[PATCH] kref : Avoid an atomic operation in kref_put() when the last reference is dropped. On most platforms, atomic_read() is a plan read of the counter and involves no atomic at all.


Signed-off-by: Eric Dumazet <[email protected]>
--- a/lib/kref.c	2006-01-30 06:11:04.000000000 +0100
+++ b/lib/kref.c	2006-01-30 06:13:32.000000000 +0100
@@ -52,7 +52,12 @@
 	WARN_ON(release == NULL);
 	WARN_ON(release == (void (*)(struct kref *))kfree);
 
-	if (atomic_dec_and_test(&kref->refcount)) {
+	/*
+	 * if current count is one, we are the last user and can release object
+	 * right now, avoiding an atomic operation on 'refcount'
+	 */
+	if ((atomic_read(&kref->refcount) == 1) ||
+	    (atomic_dec_and_test(&kref->refcount))) {
 		release(kref);
 		return 1;
 	}

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux