lkml.org 
[lkml]   [2006]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/5] pid: Implement task references.
    Greg KH a écrit :
    > On Sun, Jan 29, 2006 at 02:58:51PM -0700, Eric W. Biederman wrote:
    >> Greg KH <greg@kroah.com> 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 <dada1@cosmosbay.com>
    --- 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;
    }
    \
     
     \ /
      Last update: 2006-01-30 06:22    [W:2.971 / U:0.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site