[Bug? Report] kref problem

[Bug? Report] kref problem

[Artem B. Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

Hello Greg,

I've hit on a kref problem Please, glance at the attached test module.

The idea of the test is to create 2 kobjects (a and b), create dir A
with kobject A, and dir B with kobject B, so that A is B's parent. E.g.,
we'll have /sys/A/B.

I see the following output of the test:

a inited, kref 1
b inited, kref 1
dir A created, A kref 1, B kref 1
dir B created, A kref 1, B kref 1
b_release
a_release
kobj B put, A kref 0, B kref 0
kobj A put, A kref -1, B kref 0


So what I don't like is this "A kref -1". Why when I remove directory B,
kobj a is released? For me it looks like a bug.

The kernel is 2.6.16-rc6.

Thanks.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

[-- Attachment #2: kreftest.c --]
[-- Type: text/x-csrc, Size: 2266 bytes --]

#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/module.h>
#include <linux/kobject.h>
#include <linux/stat.h>

static void a_release(struct kobject *kobj);

static struct kobj_type a_ktype = {
        .release   = a_release
};

static void b_release(struct kobject *kobj);

static struct kobj_type b_ktype = {
        .release   = b_release
};

struct my_obj_a
{
	struct kobject kobj;
} a;

struct my_obj_b
{
	struct kobject kobj;
} b;

static __init int test_init(void)
{
	int err;

	kobject_init(&a.kobj);
	a.kobj.ktype = &a_ktype;
	err = kobject_set_name(&a.kobj, "A");
	if (err)
		return err;
	printk("a inited, kref %d\n", atomic_read(&a.kobj.kref.refcount));
	
	kobject_init(&b.kobj);
	b.kobj.ktype = &b_ktype;
	b.kobj.parent = &a.kobj;
	err = kobject_set_name(&b.kobj, "B");
	if (err)
		goto out_a;
	printk("b inited, kref %d\n", atomic_read(&b.kobj.kref.refcount));

	err = sysfs_create_dir(&a.kobj);
	if (err)
		goto out_b;
	printk("dir A created, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));
	
	err = sysfs_create_dir(&b.kobj);
	if (err)
		goto out_a_dir;
	printk("dir B created, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));

	kobject_put(&b.kobj);
	printk("kobj B put, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));

	kobject_put(&a.kobj);
	printk("kobj A put, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));

	return 0;

out_a_dir:
	sysfs_remove_dir(&a.kobj);
out_b:
	kobject_put(&b.kobj);
out_a:
	kobject_put(&a.kobj);
	return err;
}
module_init(test_init);

static void a_release(struct kobject *kobj)
{
	struct my_obj_a *a;
	
	printk("%s\n", __FUNCTION__);
	a = container_of(kobj, struct my_obj_a, kobj);
	sysfs_remove_dir(&a->kobj);
}

static void b_release(struct kobject *kobj)
{
	struct my_obj_b *b;

	printk("%s\n", __FUNCTION__);
	b = container_of(kobj, struct my_obj_b, kobj);
	sysfs_remove_dir(&b->kobj);
}

static __exit void test_exit(void)
{
}
module_exit(test_exit);

MODULE_VERSION("1");
MODULE_DESCRIPTION("kref test");
MODULE_AUTHOR("Artem B. Bityutskiy");
MODULE_LICENSE("GPL");

Re: [Bug? Report] kref problem

[Greg KH]

On Thu, Mar 16, 2006 at 02:41:19PM +0300, Artem B. Bityutskiy wrote:
> Hello Greg,
> 
> I've hit on a kref problem Please, glance at the attached test module.
> 
> The idea of the test is to create 2 kobjects (a and b), create dir A
> with kobject A, and dir B with kobject B, so that A is B's parent. E.g.,
> we'll have /sys/A/B.
> 
> I see the following output of the test:
> 
> a inited, kref 1
> b inited, kref 1
> dir A created, A kref 1, B kref 1
> dir B created, A kref 1, B kref 1
> b_release
> a_release
> kobj B put, A kref 0, B kref 0
> kobj A put, A kref -1, B kref 0
> 
> 
> So what I don't like is this "A kref -1". Why when I remove directory B,
> kobj a is released? For me it looks like a bug.

Sample code please?

Also, creating sysfs directories does not change the reference count on
kobjects, they are two separate things.

thanks,

greg k-h

Re: [Bug? Report] kref problem

[Greg KH]

On Thu, Mar 16, 2006 at 08:47:12AM -0800, Greg KH wrote:
> On Thu, Mar 16, 2006 at 02:41:19PM +0300, Artem B. Bityutskiy wrote:
> > Hello Greg,
> > 
> > I've hit on a kref problem Please, glance at the attached test module.
> > 
> > The idea of the test is to create 2 kobjects (a and b), create dir A
> > with kobject A, and dir B with kobject B, so that A is B's parent. E.g.,
> > we'll have /sys/A/B.
> > 
> > I see the following output of the test:
> > 
> > a inited, kref 1
> > b inited, kref 1
> > dir A created, A kref 1, B kref 1
> > dir B created, A kref 1, B kref 1
> > b_release
> > a_release
> > kobj B put, A kref 0, B kref 0
> > kobj A put, A kref -1, B kref 0
> > 
> > 
> > So what I don't like is this "A kref -1". Why when I remove directory B,
> > kobj a is released? For me it looks like a bug.
> 
> Sample code please?

Oops, you did that already, I'll look at it now, still waking up, sorry.

greg k-h

Re: [Bug? Report] kref problem

[Artem B. Bityutskiy]

On Thu, 2006-03-16 at 08:47 -0800, Greg KH wrote:
> Sample code please?
> 
> Also, creating sysfs directories does not change the reference count on
> kobjects, they are two separate things.

Err, didn't I attache the test module?

Here it goes:


#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/module.h>
#include <linux/kobject.h>
#include <linux/stat.h>

static void a_release(struct kobject *kobj);

static struct kobj_type a_ktype = {
        .release   = a_release
};

static void b_release(struct kobject *kobj);

static struct kobj_type b_ktype = {
        .release   = b_release
};

struct my_obj_a
{
	struct kobject kobj;
} a;

struct my_obj_b
{
	struct kobject kobj;
} b;

static __init int test_init(void)
{
	int err;

	kobject_init(&a.kobj);
	a.kobj.ktype = &a_ktype;
	err = kobject_set_name(&a.kobj, "A");
	if (err)
		return err;
	printk("a inited, kref %d\n", atomic_read(&a.kobj.kref.refcount));
	
	kobject_init(&b.kobj);
	b.kobj.ktype = &b_ktype;
	b.kobj.parent = &a.kobj;
	err = kobject_set_name(&b.kobj, "B");
	if (err)
		goto out_a;
	printk("b inited, kref %d\n", atomic_read(&b.kobj.kref.refcount));

	err = sysfs_create_dir(&a.kobj);
	if (err)
		goto out_b;
	printk("dir A created, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount),
atomic_read(&b.kobj.kref.refcount));
	
	err = sysfs_create_dir(&b.kobj);
	if (err)
		goto out_a_dir;
	printk("dir B created, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount),
atomic_read(&b.kobj.kref.refcount));

	kobject_put(&b.kobj);
	printk("kobj B put, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount),
atomic_read(&b.kobj.kref.refcount));

	kobject_put(&a.kobj);
	printk("kobj A put, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount),
atomic_read(&b.kobj.kref.refcount));

	return 0;

out_a_dir:
	sysfs_remove_dir(&a.kobj);
out_b:
	kobject_put(&b.kobj);
out_a:
	kobject_put(&a.kobj);
	return err;
}
module_init(test_init);

static void a_release(struct kobject *kobj)
{
	struct my_obj_a *a;
	
	printk("%s\n", __FUNCTION__);
	a = container_of(kobj, struct my_obj_a, kobj);
	sysfs_remove_dir(&a->kobj);
}

static void b_release(struct kobject *kobj)
{
	struct my_obj_b *b;

	printk("%s\n", __FUNCTION__);
	b = container_of(kobj, struct my_obj_b, kobj);
	sysfs_remove_dir(&b->kobj);
}

static __exit void test_exit(void)
{
}
module_exit(test_exit);

MODULE_VERSION("1");
MODULE_DESCRIPTION("kref test");
MODULE_AUTHOR("Artem B. Bityutskiy");
MODULE_LICENSE("GPL");

Re: [Bug? Report] kref problem

[Greg KH]

On Thu, Mar 16, 2006 at 02:41:19PM +0300, Artem B. Bityutskiy wrote:
> struct my_obj_a
> {
> 	struct kobject kobj;
> } a;
> 
> struct my_obj_b
> {
> 	struct kobject kobj;
> } b;

Don't statically create kobjects, it's not nice.  But the real problem
is below...

> static __init int test_init(void)
> {
> 	int err;
> 
> 	kobject_init(&a.kobj);
> 	a.kobj.ktype = &a_ktype;
> 	err = kobject_set_name(&a.kobj, "A");
> 	if (err)
> 		return err;
> 	printk("a inited, kref %d\n", atomic_read(&a.kobj.kref.refcount));
> 	
> 	kobject_init(&b.kobj);
> 	b.kobj.ktype = &b_ktype;
> 	b.kobj.parent = &a.kobj;
> 	err = kobject_set_name(&b.kobj, "B");
> 	if (err)
> 		goto out_a;
> 	printk("b inited, kref %d\n", atomic_read(&b.kobj.kref.refcount));
> 
> 	err = sysfs_create_dir(&a.kobj);
> 	if (err)
> 		goto out_b;
> 	printk("dir A created, A kref %d, B kref %d\n",
> 	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));
> 	
> 	err = sysfs_create_dir(&b.kobj);
> 	if (err)
> 		goto out_a_dir;
> 	printk("dir B created, A kref %d, B kref %d\n",
> 	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));
> 
> 	kobject_put(&b.kobj);
> 	printk("kobj B put, A kref %d, B kref %d\n",
> 	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));
> 
> 	kobject_put(&a.kobj);
> 	printk("kobj A put, A kref %d, B kref %d\n",
> 	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));
> 
> 	return 0;
> 
> out_a_dir:
> 	sysfs_remove_dir(&a.kobj);
> out_b:
> 	kobject_put(&b.kobj);
> out_a:
> 	kobject_put(&a.kobj);
> 	return err;
> }
> module_init(test_init);
> 
> static void a_release(struct kobject *kobj)
> {
> 	struct my_obj_a *a;
> 	
> 	printk("%s\n", __FUNCTION__);
> 	a = container_of(kobj, struct my_obj_a, kobj);
> 	sysfs_remove_dir(&a->kobj);

Woah, don't do that here, the kobject core already does this.  A release
function is for you to release the memory you have created with this
kobject, not to mess with sysfs.

So, don't do this and everything should work just fine.

thanks,

greg k-h

Re: [Bug? Report] kref problem

[Artem B. Bityutskiy]

On Thu, 2006-03-16 at 08:53 -0800, Greg KH wrote: 
> > static void a_release(struct kobject *kobj)
> > {
> > 	struct my_obj_a *a;
> > 	
> > 	printk("%s\n", __FUNCTION__);
> > 	a = container_of(kobj, struct my_obj_a, kobj);
> > 	sysfs_remove_dir(&a->kobj);
> 
> Woah, don't do that here, the kobject core already does this.  A release
> function is for you to release the memory you have created with this
> kobject, not to mess with sysfs.
So do you mean this (attached) ? Anyway I end up with -1 kref.

My real task is: I have sysfs directory /sys/A which corresponds to my
module, to my subsystem. There I want to create subdirectories
like /sys/A/B/ and delete them from time to time. So the problem is that
whenver I remove B I end up with A's kref decremented. The attached test
demonstrates this. P;ease, look at its output:

a inited, kref 1
b inited, kref 1
dir A created, A kref 1, B kref 1
dir B created, A kref 1, B kref 1
dir B removed, A kref 1, B kref 1
b_release
a_release       <--- What is this? I removed B, not A ???
kobj B put, A kref 0, B kref 0
dir A removed, A kref 0, B kref 0
kobj A put, A kref -1, B kref 0

Thanks.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [Bug? Report] kref problem

[Artem B. Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

On Thu, 2006-03-16 at 20:07 +0300, Artem B. Bityutskiy wrote:
> On Thu, 2006-03-16 at 08:53 -0800, Greg KH wrote: 
> > > static void a_release(struct kobject *kobj)
> > > {
> > > 	struct my_obj_a *a;
> > > 	
> > > 	printk("%s\n", __FUNCTION__);
> > > 	a = container_of(kobj, struct my_obj_a, kobj);
> > > 	sysfs_remove_dir(&a->kobj);
> > 
> > Woah, don't do that here, the kobject core already does this.  A release
> > function is for you to release the memory you have created with this
> > kobject, not to mess with sysfs.
> So do you mean this (attached) ? Anyway I end up with -1 kref.
> 
Pardon, forgot to attach.

[-- Attachment #2: kreftest1.c --]
[-- Type: text/x-csrc, Size: 2910 bytes --]

#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/module.h>
#include <linux/kobject.h>
#include <linux/stat.h>

static void a_release(struct kobject *kobj);

static struct kobj_type a_ktype = {
        .release   = a_release
};

static void b_release(struct kobject *kobj);

static struct kobj_type b_ktype = {
        .release   = b_release
};

struct my_obj_a
{
	struct kobject kobj;
} a;

struct my_obj_b
{
	struct kobject kobj;
} b;

static __init int test_init(void)
{
	int err;

	kobject_init(&a.kobj);
	a.kobj.ktype = &a_ktype;
	err = kobject_set_name(&a.kobj, "A");
	if (err)
		return err;
	printk("a inited, kref %d\n", atomic_read(&a.kobj.kref.refcount));
	
	kobject_init(&b.kobj);
	b.kobj.ktype = &b_ktype;
	b.kobj.parent = &a.kobj;
	err = kobject_set_name(&b.kobj, "B");
	if (err)
		goto out_a;
	printk("b inited, kref %d\n", atomic_read(&b.kobj.kref.refcount));

	err = sysfs_create_dir(&a.kobj);
	if (err)
		goto out_b;
	printk("dir A created, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));
	
	err = sysfs_create_dir(&b.kobj);
	if (err)
		goto out_a_dir;
	printk("dir B created, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));

	sysfs_remove_dir(&b.kobj);
	printk("dir B removed, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));

	kobject_put(&b.kobj);
	printk("kobj B put, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));

	sysfs_remove_dir(&a.kobj);
	printk("dir A removed, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));

	kobject_put(&a.kobj);
	printk("kobj A put, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));

	return 0;

out_a_dir:
	sysfs_remove_dir(&a.kobj);
	printk("dir A removed, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));
out_b:
	kobject_put(&b.kobj);
	printk("kobj B put, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));
out_a:
	kobject_put(&a.kobj);
	printk("kobj A put, A kref %d, B kref %d\n",
	       atomic_read(&a.kobj.kref.refcount), atomic_read(&b.kobj.kref.refcount));
	return err;
}
module_init(test_init);

static void a_release(struct kobject *kobj)
{
	struct my_obj_a *a;
	
	a = container_of(kobj, struct my_obj_a, kobj);
	printk("%s\n", __FUNCTION__);
}

static void b_release(struct kobject *kobj)
{
	struct my_obj_b *b;

	b = container_of(kobj, struct my_obj_b, kobj);
	printk("%s\n", __FUNCTION__);
}

static __exit void test_exit(void)
{
}
module_exit(test_exit);

MODULE_VERSION("1");
MODULE_DESCRIPTION("kref test");
MODULE_AUTHOR("Artem B. Bityutskiy");
MODULE_LICENSE("GPL");

Re: [Bug? Report] kref problem

[Greg KH]

On Thu, Mar 16, 2006 at 08:10:04PM +0300, Artem B. Bityutskiy wrote:
> On Thu, 2006-03-16 at 20:07 +0300, Artem B. Bityutskiy wrote:
> > On Thu, 2006-03-16 at 08:53 -0800, Greg KH wrote: 
> > > > static void a_release(struct kobject *kobj)
> > > > {
> > > > 	struct my_obj_a *a;
> > > > 	
> > > > 	printk("%s\n", __FUNCTION__);
> > > > 	a = container_of(kobj, struct my_obj_a, kobj);
> > > > 	sysfs_remove_dir(&a->kobj);
> > > 
> > > Woah, don't do that here, the kobject core already does this.  A release
> > > function is for you to release the memory you have created with this
> > > kobject, not to mess with sysfs.
> > So do you mean this (attached) ? Anyway I end up with -1 kref.
> > 
> Pardon, forgot to attach.

> #include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/module.h>
> #include <linux/kobject.h>
> #include <linux/stat.h>
> 
> static void a_release(struct kobject *kobj);
> 
> static struct kobj_type a_ktype = {
>         .release   = a_release
> };
> 
> static void b_release(struct kobject *kobj);
> 
> static struct kobj_type b_ktype = {
>         .release   = b_release
> };
> 
> struct my_obj_a
> {
> 	struct kobject kobj;
> } a;
> 
> struct my_obj_b
> {
> 	struct kobject kobj;
> } b;
> 
> static __init int test_init(void)
> {
> 	int err;
> 
> 	kobject_init(&a.kobj);
> 	a.kobj.ktype = &a_ktype;
> 	err = kobject_set_name(&a.kobj, "A");
> 	if (err)
> 		return err;
> 	printk("a inited, kref %d\n", atomic_read(&a.kobj.kref.refcount));
> 	
> 	kobject_init(&b.kobj);
> 	b.kobj.ktype = &b_ktype;
> 	b.kobj.parent = &a.kobj;
> 	err = kobject_set_name(&b.kobj, "B");
> 	if (err)
> 		goto out_a;
> 	printk("b inited, kref %d\n", atomic_read(&b.kobj.kref.refcount));
> 
> 	err = sysfs_create_dir(&a.kobj);

Again, why are you trying to call the sysfs raw functions?  You are not
registering the kobject with the kobject core, so bad things are
happening.  Why not call kobject_register() or kobject_add(), like it is
documented to do so?

Because of this, you are seeing odd things happen.

thanks,

greg k-h

Re: [Bug? Report] kref problem

[Artem B. Bityutskiy]

On Thu, 2006-03-16 at 09:20 -0800, Greg KH wrote:
> Again, why are you trying to call the sysfs raw functions?  You are not
> registering the kobject with the kobject core, so bad things are
> happening.  Why not call kobject_register() or kobject_add(), like it is
> documented to do so?

Well, we were discussing this with you some time ago, and you pointed me
to these raw functions. My stuff just does not fit device/driver/bu
modes and you said I have to create whatever sysfs hierarchy I want with
the raw functions.

kobject_register()/kobject_del() instead of
sysfs_create_dir()/sysfs_remove_dir() solved my problem, thanks. Just to
refine this, I'm still going to use
sysfs_create_file()/sysfs_remove_file() to create whatever attributes I
want, is this right?

I've just noticed similarity in naming: sysfs_remove_file() creates a
file, so the symmetrical sysfs_create_dir() creates a directory. So just
started using it. From the names it was not obvious that I could
not. :-)

Thanks.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [Bug? Report] kref problem

[Greg KH]

On Thu, Mar 16, 2006 at 09:00:19PM +0300, Artem B. Bityutskiy wrote:
> On Thu, 2006-03-16 at 09:20 -0800, Greg KH wrote:
> > Again, why are you trying to call the sysfs raw functions?  You are not
> > registering the kobject with the kobject core, so bad things are
> > happening.  Why not call kobject_register() or kobject_add(), like it is
> > documented to do so?
> 
> Well, we were discussing this with you some time ago, and you pointed me
> to these raw functions. My stuff just does not fit device/driver/bu
> modes and you said I have to create whatever sysfs hierarchy I want with
> the raw functions.

Yes, but you still need to register your kobject :)

> kobject_register()/kobject_del() instead of
> sysfs_create_dir()/sysfs_remove_dir() solved my problem, thanks. Just to
> refine this, I'm still going to use
> sysfs_create_file()/sysfs_remove_file() to create whatever attributes I
> want, is this right?

Yes, that should be fine.

> I've just noticed similarity in naming: sysfs_remove_file() creates a
> file, so the symmetrical sysfs_create_dir() creates a directory. So just
> started using it. From the names it was not obvious that I could
> not. :-)

Hm, perhaps I'll just remove the export for that function as the only
thing that should use that is the kobject core, and that can't be built
as a module.

thanks,

greg k-h

Re: [Bug? Report] kref problem

[Greg KH]

On Thu, Mar 16, 2006 at 08:07:57PM +0300, Artem B. Bityutskiy wrote:
> On Thu, 2006-03-16 at 08:53 -0800, Greg KH wrote: 
> > > static void a_release(struct kobject *kobj)
> > > {
> > > 	struct my_obj_a *a;
> > > 	
> > > 	printk("%s\n", __FUNCTION__);
> > > 	a = container_of(kobj, struct my_obj_a, kobj);
> > > 	sysfs_remove_dir(&a->kobj);
> > 
> > Woah, don't do that here, the kobject core already does this.  A release
> > function is for you to release the memory you have created with this
> > kobject, not to mess with sysfs.
> So do you mean this (attached) ? Anyway I end up with -1 kref.

No file was attached :(

Care to try again?

> My real task is: I have sysfs directory /sys/A which corresponds to my
> module, to my subsystem. There I want to create subdirectories
> like /sys/A/B/ and delete them from time to time.

What kind of subsystem are you creating that you are using raw kobjects?

> So the problem is that whenver I remove B I end up with A's kref
> decremented.

And does A go away?  Or is it still there in sysfs?

> The attached test
> demonstrates this. P;ease, look at its output:
> 
> a inited, kref 1
> b inited, kref 1
> dir A created, A kref 1, B kref 1
> dir B created, A kref 1, B kref 1
> dir B removed, A kref 1, B kref 1
> b_release
> a_release       <--- What is this? I removed B, not A ???
> kobj B put, A kref 0, B kref 0
> dir A removed, A kref 0, B kref 0
> kobj A put, A kref -1, B kref 0

-ENOFILE :(

thanks,

greg k-h

Re: [Bug? Report] kref problem

[Dave Hansen]

On Thu, 2006-03-16 at 08:53 -0800, Greg KH wrote:
> On Thu, Mar 16, 2006 at 02:41:19PM +0300, Artem B. Bityutskiy wrote:
> > struct my_obj_a
> > {
> > 	struct kobject kobj;
> > } a;
> > 
> > struct my_obj_b
> > {
> > 	struct kobject kobj;
> > } b;
> 
> Don't statically create kobjects, it's not nice.  But the real problem
> is below...

This seems to be one of those top ten sysfs snafus.  Could we take the
definitions from include/asm-generic/sections.h, and make a kobject
verification function to put in the critical generic kernel functions
that deal with kobjects,  like kobject_init()?

Somthing like...

void verify_dynamic_kobject_allocation(struct kobject *kobj)
{
	if (kobj >= &_data && kobj < &_edata)
		goto warn;
	if (kobj >= &_bss_start && kobj < &_bss_end)
		goto warn;
	...
	return;
warn:
	printk(KERN_WARN "statically allocated kobject, you suck...\n");
}

I'm not sure that all of the architectures fill in all of the values,
but we could at least support the warnings for the ones that do.  That
includes at least i386, so it could be a relatively effective tool.

I'll cook up a real patch in a bit.

-- Dave

Re: [Bug? Report] kref problem

[Greg KH]

On Thu, Mar 16, 2006 at 09:31:18AM -0800, Dave Hansen wrote:
> On Thu, 2006-03-16 at 08:53 -0800, Greg KH wrote:
> > On Thu, Mar 16, 2006 at 02:41:19PM +0300, Artem B. Bityutskiy wrote:
> > > struct my_obj_a
> > > {
> > > 	struct kobject kobj;
> > > } a;
> > > 
> > > struct my_obj_b
> > > {
> > > 	struct kobject kobj;
> > > } b;
> > 
> > Don't statically create kobjects, it's not nice.  But the real problem
> > is below...
> 
> This seems to be one of those top ten sysfs snafus.  Could we take the
> definitions from include/asm-generic/sections.h, and make a kobject
> verification function to put in the critical generic kernel functions
> that deal with kobjects,  like kobject_init()?

I wish.  The main offender of this is the kernel core code itself, with
the decl_subsys and struct bus stuff.  If you provide some nice fuctions
to fix those up to be dynamic, then I would have no problem with the
function you have below.

> Somthing like...
> 
> void verify_dynamic_kobject_allocation(struct kobject *kobj)
> {
> 	if (kobj >= &_data && kobj < &_edata)
> 		goto warn;
> 	if (kobj >= &_bss_start && kobj < &_bss_end)
> 		goto warn;
> 	...
> 	return;
> warn:
> 	printk(KERN_WARN "statically allocated kobject, you suck...\n");
> }
> 
> I'm not sure that all of the architectures fill in all of the values,
> but we could at least support the warnings for the ones that do.  That
> includes at least i386, so it could be a relatively effective tool.
> 
> I'll cook up a real patch in a bit.

That would be fun to play with, I'd appreciate it.  If nothing else,
I'll add it to my tree for future use.

thanks,

greg k-h

Re: [Bug? Report] kref problem

[Artem B. Bityutskiy]

Greg KH wrote:
> Don't statically create kobjects, it's not nice.  But the real problem
> is below...

Well, that was just an example...

But in real life I do use a static kobject in one case, so I'm very 
interested what should I do instead. I have a subsystem, and I want it 
to put all its stuff in a /sys/A directory. So I just define a static 
kobject for A and assign a dummy release function to it. Why is this bad?

And what should I do instead? kmalloc(sizeof(struct kobject), 
GFP_KERNEL) ? I do not have a dynamic structure corresponding to my 
module. I have many data structures corresponding to entities my object 
handles and I have one static array which refers them. All is simple. I 
do not want to introduce a dynamic data structure corresponding to the 
subsystem as a whole just in order to not use static kobjects.

Comments?

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: [Bug? Report] kref problem

[Greg KH]

On Thu, Mar 16, 2006 at 08:45:10PM +0300, Artem B. Bityutskiy wrote:
> 
> Greg KH wrote:
> >Don't statically create kobjects, it's not nice.  But the real problem
> >is below...
> 
> Well, that was just an example...
> 
> But in real life I do use a static kobject in one case, so I'm very 
> interested what should I do instead. I have a subsystem, and I want it 
> to put all its stuff in a /sys/A directory. So I just define a static 
> kobject for A and assign a dummy release function to it. Why is this bad?

If you use decl_subsys(), you should be fine for this.  Use that instead
of trying to roll your own subsystem kobjects please.  That
infrastructure was written for a reason...

> And what should I do instead? kmalloc(sizeof(struct kobject), 
> GFP_KERNEL) ? I do not have a dynamic structure corresponding to my 
> module. I have many data structures corresponding to entities my object 
> handles and I have one static array which refers them. All is simple. I 
> do not want to introduce a dynamic data structure corresponding to the 
> subsystem as a whole just in order to not use static kobjects.

Data (kobjects) have a different lifespan than code (modules).
Seperating them is a good idea, and if not, your reference counting
issues can be quite nasty.  See the recent EDAC fiasco for a good
example of how easy it is to mess things up in this manner.

thanks,

greg k-h

Re: [Bug? Report] kref problem

[Artem B. Bityutskiy]

Greg KH wrote:
> If you use decl_subsys(), you should be fine for this.  Use that instead
> of trying to roll your own subsystem kobjects please.  That
> infrastructure was written for a reason...
Ok, I see, thanks. I just thought that this subsystem stuff will oblige 
me to use the device/driver/bus model which does not suit me.

> Data (kobjects) have a different lifespan than code (modules).
> Seperating them is a good idea, and if not, your reference counting
> issues can be quite nasty.  See the recent EDAC fiasco for a good
> example of how easy it is to mess things up in this manner.

My logic was that the lifetime of that kobject = lifetime of my module 
because I cannot remove the module because every it's user increments 
the module's refcount. So, if refcount of my module is zero then the 
kobject's refcount is zero. Why this doesn't this work?

Note, I do not object, I agree that in general you're right, I'm just 
wonering.

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: [Bug? Report] kref problem

[Greg KH]

On Thu, Mar 16, 2006 at 09:08:56PM +0300, Artem B. Bityutskiy wrote:
> Greg KH wrote:
> >If you use decl_subsys(), you should be fine for this.  Use that instead
> >of trying to roll your own subsystem kobjects please.  That
> >infrastructure was written for a reason...
> Ok, I see, thanks. I just thought that this subsystem stuff will oblige 
> me to use the device/driver/bus model which does not suit me.

decl_subsys() is in the sysfs.h header file, not the device.h file.
Just stay away from anything in there if you hate the driver core so
much :)

> >Data (kobjects) have a different lifespan than code (modules).
> >Seperating them is a good idea, and if not, your reference counting
> >issues can be quite nasty.  See the recent EDAC fiasco for a good
> >example of how easy it is to mess things up in this manner.
> 
> My logic was that the lifetime of that kobject = lifetime of my module 
> because I cannot remove the module because every it's user increments 
> the module's refcount. So, if refcount of my module is zero then the 
> kobject's refcount is zero. Why this doesn't this work?

It "should" work in theory, but in real-life, the odds of getting all of
that lifetime logic correct...  :)

You can do it, but in general, it's easier to think about kobjects as
being dynamic devices, as that is what they are.  So making them dynamic
for real is a better thing to do to reinforce that principle.

Again, what are you doing that you can't use the driver core for?

thanks,

greg k-h

Re: [Bug? Report] kref problem

[Artem B. Bityutskiy]

Greg KH wrote:
> decl_subsys() is in the sysfs.h header file, not the device.h file.
> Just stay away from anything in there if you hate the driver core so
> much :)
Ok, I see, thank you. I do not hate it, it is just not appropriate for 
me. I do not have any bus. My device is a virtual device, not a real one.

-- 
Best regards, Artem B. Bityutskiy
Oktet Labs (St. Petersburg), Software Engineer.
+7 812 4286709 (office) +7 911 2449030 (mobile)
E-mail: dedekind@oktetlabs.ru, Web: www.oktetlabs.ru

Re: [Bug? Report] kref problem

[Greg KH]

On Fri, Mar 17, 2006 at 12:30:07PM +0300, Artem B. Bityutskiy wrote:
> Greg KH wrote:
> >decl_subsys() is in the sysfs.h header file, not the device.h file.
> >Just stay away from anything in there if you hate the driver core so
> >much :)
> Ok, I see, thank you. I do not hate it, it is just not appropriate for 
> me. I do not have any bus. My device is a virtual device, not a real one.

So, you should still use the driver core for virtual devices (we have
lots of virtual devices in the driver model today.)

Why are you not using it?  What kind of device do you have?  Why does it
not fit into any existing device model (platform, system, etc)?

thanks,

greg k-h

Re: [Bug? Report] kref problem

[Artem B. Bityutskiy]

Greg KH wrote:
> So, you should still use the driver core for virtual devices (we have
> lots of virtual devices in the driver model today.)
> 
> Why are you not using it?  What kind of device do you have?  Why does it
> not fit into any existing device model (platform, system, etc)?
> 
Actually I tried to use this model first. I hit on problems with and you 
said that I should avoid using the device model.

Unfortunately I cannot so far say what I'm writing. The below is a cite 
from my old mail which illustrates the sort of my device:

"In connection with this, I have a question. There is a whole bunch of
drivers which do not directly relate to hardware devices, but which
still want to expose their parameters via sysfs. For example, this could
be a filesystem, LVM, a compression layer on top of a file system of a
block device, whatever. These are "virtual" devices and they are not
physically connected to any bus. How should they deal with sysfs?"

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.