kobjects, sysfs and the driver model make my head hurt

kobjects, sysfs and the driver model make my head hurt

[Matthew Wilcox]

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

Re: kobjects, sysfs and the driver model make my head hurt

[Davide Libenzi]

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

Re: kobjects, sysfs and the driver model make my head hurt

[James Morris]

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>

Re: kobjects, sysfs and the driver model make my head hurt

[Jeff Garzik]

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

Re: kobjects, sysfs and the driver model make my head hurt

[Johan.Adolfsson]

----- 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

Re: kobjects, sysfs and the driver model make my head hurt

[Greg KH]

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

Re: kobjects, sysfs and the driver model make my head hurt

[Jeff Garzik]

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