It's just all too complex. There's just too many levels of indirection
and structures which do almost the same thing and misleading functions.
It needs to be thoroughly simplified. Here's one particularly misleading
function:
/**
* kobject_get - increment refcount for object.
* @kobj: object.
*/
struct kobject * kobject_get(struct kobject * kobj)
{
struct kobject * ret = kobj;
if (kobj) {
WARN_ON(!atomic_read(&kobj->refcount));
atomic_inc(&kobj->refcount);
} else
ret = NULL;
return ret;
}
Why on earth does it return the value of its argument? And why's it
written in such a convoluted way? Here's a simpler form which retains
all the existing semantics:
struct kobject * kobject_get(struct kobject * kobj)
{
if (kobj) {
WARN_ON(!atomic_read(&kobj->refcount));
atomic_inc(&kobj->refcount);
}
return kobj;
}
or maybe better:
{
if (!kobj)
return NULL;
WARN_ON(!atomic_read(&kobj->refcount));
atomic_inc(&kobj->refcount);
return kobj;
}
But why return anything? Which looks clearer?
(a) kobj = kobject_get(kobj);
(b) kobject_get(kobj);
The first one makes me think that kobject_get might return a different
kobject than the one I passed in. That doesn't make much sense.
There's much more in this vein, but this email is long enough already.
<rmk> "you are in a maze of structures, all alike. There is a kset here."
--
"It's not Hollywood. War is real, war is primarily not about defeat or
victory, it is about death. I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk
On Sun, Jul 06, 2003 at 05:33:53PM +0100, Matthew Wilcox wrote:
>
> struct kobject * kobject_get(struct kobject * kobj)
> {
> if (kobj) {
> WARN_ON(!atomic_read(&kobj->refcount));
> atomic_inc(&kobj->refcount);
> }
> return kobj;
> }
That's nice. Remember, we used to have a lock in there, that's why the
code doesn't look that clean after it was removed.
> But why return anything? Which looks clearer?
>
> (a) kobj = kobject_get(kobj);
This is the way to call kobject_get(), as the object we get after the
function returns is the one we can then safely use.
> The first one makes me think that kobject_get might return a different
> kobject than the one I passed in. That doesn't make much sense.
Think of it as, "now we can use this kobject, not the one before calling
kobject_get()".
thanks,
greg k-h
On Sun, 6 Jul 2003, Matthew Wilcox wrote:
> Why on earth does it return the value of its argument?
Maybe for the same reason 'strcpy' returns 'dest'. It allows you to use
the function in a function parameter :
obj *get(obj *o);
int rel(obj *o);
int foo(int q, obj *o);
foo(17, get(o));
rel(o);
- Davide
Greg KH wrote:
>>(a) kobj = kobject_get(kobj);
>
>
> This is the way to call kobject_get(), as the object we get after the
> function returns is the one we can then safely use.
[...]
> Think of it as, "now we can use this kobject, not the one before calling
> kobject_get()".
Doesn't matter. There is still absolutely no reason for the additional
pointer storage. I agree with with you "Thinks of it as", but also add
my own: think of it as a spinlock function. It doesn't return any
value, but you can't touch the locked object(s) before you call the
function.
The alloc functions return pointers. The _get functions never need to,
because logically there should always we at least one ref when we are
calling _get. (unless we want _get to notice an OBJ_FREEING flag and
fail, that is...)
Jeff
On Sun, 6 Jul 2003, Davide Libenzi wrote:
> On Sun, 6 Jul 2003, Matthew Wilcox wrote:
>
> > Why on earth does it return the value of its argument?
>
> Maybe for the same reason 'strcpy' returns 'dest'. It allows you to use
> the function in a function parameter :
It also makes calling code cleaner when copying refcounted objects:
e.g.
new->foo = foo_get(old->foo);
new->bar = bar_get(old->bar);
otherwise, you'd have to do:
foo_get(old->foo);
new->foo = old->foo;
bar_get(old->bar);
new->bar = old->bar;
- James
--
James Morris
<[email protected]>
James Morris wrote:
> On Sun, 6 Jul 2003, Davide Libenzi wrote:
>
>
>>On Sun, 6 Jul 2003, Matthew Wilcox wrote:
>>
>>
>>>Why on earth does it return the value of its argument?
>>
>>Maybe for the same reason 'strcpy' returns 'dest'. It allows you to use
>>the function in a function parameter :
>
>
> It also makes calling code cleaner when copying refcounted objects:
>
> e.g.
> new->foo = foo_get(old->foo);
> new->bar = bar_get(old->bar);
>
> otherwise, you'd have to do:
>
> foo_get(old->foo);
> new->foo = old->foo;
> bar_get(old->bar);
> new->bar = old->bar;
well...
struct blah *foo_ref = foo;
... not using foo_ref ...
foo_get(foo_ref);
... using foo_ref ...
foo_put(foo_ref);
versus
struct blah *foo_ref;
... not using foo_ref ...
foo_ref = foo_get(foo);
... using foo_ref ...
foo_put(foo_ref);
I suppose it's a matter of taste rather than necessity.
As a tangent, if kobject_get is so small now, why not just make it
static inline to optimize this case?
Jeff
----- Original Message -----
From: "Davide Libenzi" <[email protected]>
To: "Matthew Wilcox" <[email protected]>
Cc: "Patrick Mochel" <[email protected]>; <[email protected]>
Sent: Sunday, July 06, 2003 6:42 PM
Subject: Re: kobjects, sysfs and the driver model make my head hurt
> On Sun, 6 Jul 2003, Matthew Wilcox wrote:
>
> > Why on earth does it return the value of its argument?
>
> Maybe for the same reason 'strcpy' returns 'dest'. It allows you to use
> the function in a function parameter :
Another possible benefit (although I'm not sure we should care)
is that if the return variable is the same as the first argument,
the compiler can save an instruction or two on at least some archs.
Simple example:
char *tst(char *p, int i)
{
return p;
}
void tst2(char *p, int i)
{
*p = i;
p = tst(p,i);
p[1]=i;
}
void tst3(char *p, int i)
{
*p = i;
tst(p,i);
p[1]=i;
}
i386 ts2 saves 3 instructions compared to tst3
tst2:
pushl %ebp
movl %esp,%ebp
subl $20,%esp
pushl %ebx
movl 8(%ebp),%eax
movl 12(%ebp),%ebx
movb %bl,(%eax)
addl $-8,%esp
pushl %ebx
pushl %eax
call tst
movb %bl,1(%eax)
movl -24(%ebp),%ebx
leave
ret
.Lfe2:
.size tst2,.Lfe2-tst2
.align 4
.globl tst3
.type tst3,@function
tst3:
pushl %ebp
movl %esp,%ebp
subl $16,%esp
pushl %esi
pushl %ebx
movl 8(%ebp),%esi
movl 12(%ebp),%ebx
movb %bl,(%esi)
addl $-8,%esp
pushl %ebx
pushl %esi
call tst
movb %bl,1(%esi)
leal -24(%ebp),%esp
popl %ebx
popl %esi
leave
ret
.Lfe3:
On CRIS you save one register on stack instead of two
tst2:
Push $srp
subq 4,$sp
movem $r0,[$sp]
move.d $r10,$r9
move.d $r11,$r0
move.b $r11,[$r9]
Jsr tst
move.b $r0,[$r10+1]
movem [$sp+],$r0
Jump [$sp+]
.Lfe2:
.size tst2,.Lfe2-tst2
.align 1
.global tst3
.type tst3,@function
tst3:
Push $srp
subq 8,$sp
movem $r1,[$sp]
move.d $r10,$r0
move.d $r11,$r1
move.b $r11,[$r0+]
Jsr tst
move.b $r1,[$r0]
movem [$sp+],$r1
Jump [$sp+]
.Lfe3:
/Johan