* kobjects, sysfs and the driver model make my head hurt
@ 2003-07-06 16:33 Matthew Wilcox
2003-07-06 16:42 ` Davide Libenzi
2003-07-06 16:46 ` Greg KH
0 siblings, 2 replies; 7+ messages in thread
From: Matthew Wilcox @ 2003-07-06 16:33 UTC (permalink / raw)
To: Patrick Mochel; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: kobjects, sysfs and the driver model make my head hurt
2003-07-06 16:33 kobjects, sysfs and the driver model make my head hurt Matthew Wilcox
@ 2003-07-06 16:42 ` Davide Libenzi
2003-07-06 17:03 ` James Morris
2003-07-07 7:05 ` Johan.Adolfsson
2003-07-06 16:46 ` Greg KH
1 sibling, 2 replies; 7+ messages in thread
From: Davide Libenzi @ 2003-07-06 16:42 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Patrick Mochel, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: kobjects, sysfs and the driver model make my head hurt
2003-07-06 16:33 kobjects, sysfs and the driver model make my head hurt Matthew Wilcox
2003-07-06 16:42 ` Davide Libenzi
@ 2003-07-06 16:46 ` Greg KH
2003-07-06 16:54 ` Jeff Garzik
1 sibling, 1 reply; 7+ messages in thread
From: Greg KH @ 2003-07-06 16:46 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Patrick Mochel, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: kobjects, sysfs and the driver model make my head hurt
2003-07-06 16:46 ` Greg KH
@ 2003-07-06 16:54 ` Jeff Garzik
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Garzik @ 2003-07-06 16:54 UTC (permalink / raw)
To: Greg KH; +Cc: Matthew Wilcox, Patrick Mochel, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: kobjects, sysfs and the driver model make my head hurt
2003-07-06 16:42 ` Davide Libenzi
@ 2003-07-06 17:03 ` James Morris
2003-07-06 17:15 ` Jeff Garzik
2003-07-07 7:05 ` Johan.Adolfsson
1 sibling, 1 reply; 7+ messages in thread
From: James Morris @ 2003-07-06 17:03 UTC (permalink / raw)
To: Davide Libenzi; +Cc: Matthew Wilcox, Patrick Mochel, linux-kernel
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
<jmorris@intercode.com.au>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: kobjects, sysfs and the driver model make my head hurt
2003-07-06 17:03 ` James Morris
@ 2003-07-06 17:15 ` Jeff Garzik
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Garzik @ 2003-07-06 17:15 UTC (permalink / raw)
To: James Morris; +Cc: Davide Libenzi, Matthew Wilcox, Patrick Mochel, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: kobjects, sysfs and the driver model make my head hurt
2003-07-06 16:42 ` Davide Libenzi
2003-07-06 17:03 ` James Morris
@ 2003-07-07 7:05 ` Johan.Adolfsson
1 sibling, 0 replies; 7+ messages in thread
From: Johan.Adolfsson @ 2003-07-07 7:05 UTC (permalink / raw)
To: Davide Libenzi, Matthew Wilcox; +Cc: Patrick Mochel, linux-kernel
----- Original Message -----
From: "Davide Libenzi" <davidel@xmailserver.org>
To: "Matthew Wilcox" <willy@debian.org>
Cc: "Patrick Mochel" <mochel@osdl.org>; <linux-kernel@vger.kernel.org>
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2003-07-07 6:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-07-06 16:33 kobjects, sysfs and the driver model make my head hurt Matthew Wilcox
2003-07-06 16:42 ` Davide Libenzi
2003-07-06 17:03 ` James Morris
2003-07-06 17:15 ` Jeff Garzik
2003-07-07 7:05 ` Johan.Adolfsson
2003-07-06 16:46 ` Greg KH
2003-07-06 16:54 ` Jeff Garzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).