Re: [PATCH] kref: add function for reading kref value

Re: [PATCH] kref: add function for reading kref value

[Daniel Baluta]

On Mon, Dec 12, 2011 at 5:02 PM, Greg KH <greg@kroah.com> wrote:
> On Mon, Dec 12, 2011 at 02:44:52PM +0200, Daniel Baluta wrote:
>> We can easily get kref refcount value by accesing
>> kref->refcount but it is better to have a function
>> for this.
>
> No, you should NEVER need the value of the kref, if you do, you are
> doing something wrong.
>
> What code needs this patch?

I plan to replace raw refcount patterns from net/ with kref, and
I have noticed that there are some parts of code who need access
to kref->refcount value.

e.g:

core/neighbour.c:152:			if (atomic_read(&n->refcnt) == 1 &&
core/neighbour.c:801:			if (atomic_read(&n->refcnt) == 1 &&
sched/cls_u32.c:459:	if (ht->refcnt == 1) {


thanks,
Daniel.

Re: [PATCH] kref: add function for reading kref value

[Eric Dumazet]

Le lundi 12 décembre 2011 à 17:21 +0200, Daniel Baluta a écrit :
> On Mon, Dec 12, 2011 at 5:02 PM, Greg KH <greg@kroah.com> wrote:
> > On Mon, Dec 12, 2011 at 02:44:52PM +0200, Daniel Baluta wrote:
> >> We can easily get kref refcount value by accesing
> >> kref->refcount but it is better to have a function
> >> for this.
> >
> > No, you should NEVER need the value of the kref, if you do, you are
> > doing something wrong.
> >
> > What code needs this patch?
> 
> I plan to replace raw refcount patterns from net/ with kref, and
> I have noticed that there are some parts of code who need access
> to kref->refcount value.
> 
> e.g:
> 
> core/neighbour.c:152:			if (atomic_read(&n->refcnt) == 1 &&
> core/neighbour.c:801:			if (atomic_read(&n->refcnt) == 1 &&
> sched/cls_u32.c:459:	if (ht->refcnt == 1) {
> 

Well, its not a good idea.

kref adds nothing here.

Re: [PATCH] kref: add function for reading kref value

[Daniel Baluta]

On Mon, Dec 12, 2011 at 5:32 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 12 décembre 2011 à 17:21 +0200, Daniel Baluta a écrit :
>> On Mon, Dec 12, 2011 at 5:02 PM, Greg KH <greg@kroah.com> wrote:
>> > On Mon, Dec 12, 2011 at 02:44:52PM +0200, Daniel Baluta wrote:
>> >> We can easily get kref refcount value by accesing
>> >> kref->refcount but it is better to have a function
>> >> for this.
>> >
>> > No, you should NEVER need the value of the kref, if you do, you are
>> > doing something wrong.
>> >
>> > What code needs this patch?
>>
>> I plan to replace raw refcount patterns from net/ with kref, and
>> I have noticed that there are some parts of code who need access
>> to kref->refcount value.
>>
>> e.g:
>>
>> core/neighbour.c:152:                 if (atomic_read(&n->refcnt) == 1 &&
>> core/neighbour.c:801:                 if (atomic_read(&n->refcnt) == 1 &&
>> sched/cls_u32.c:459:  if (ht->refcnt == 1) {
>>
>
> Well, its not a good idea.
>
> kref adds nothing here.
>

Ok, for the moment it seems to be a bad idea. But my intention is
to integrate kref to networking code, and then to write a general
debugging tool for refs.

Tracking down reference count problems is hard, and this tool can
really help everyone.

thanks,
Daniel.

Re: [PATCH] kref: add function for reading kref value

[Eric Dumazet]

Le lundi 12 décembre 2011 à 17:39 +0200, Daniel Baluta a écrit :

> Ok, for the moment it seems to be a bad idea. But my intention is
> to integrate kref to networking code, and then to write a general
> debugging tool for refs.
> 
> Tracking down reference count problems is hard, and this tool can
> really help everyone.

I dont think kref will help you that much, because its used everywhere.

Adding a general debugging tool will provide too much noise.

Instead, we (network dev) add debugging points where we want.

Re: [PATCH] kref: add function for reading kref value

[Daniel Baluta]

On Mon, Dec 12, 2011 at 5:41 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 12 décembre 2011 à 17:39 +0200, Daniel Baluta a écrit :
>
>> Ok, for the moment it seems to be a bad idea. But my intention is
>> to integrate kref to networking code, and then to write a general
>> debugging tool for refs.
>>
>> Tracking down reference count problems is hard, and this tool can
>> really help everyone.
>
> I dont think kref will help you that much, because its used everywhere.
>
> Adding a general debugging tool will provide too much noise.
>
> Instead, we (network dev) add debugging points where we want.

Yes, but you have to do this each time you start debugging, for a
particular referenced counted object.

We must find a clever solution to avoid the noise. (e.g use
/proc, /sysfs, /debugfs options to trigger dumping info for
some/all objects with a certain state).

Usecase:

At some point we need to know all objects with refcount X,
and their history of get/put operations.

For us it would have been very useful when debugging dev
refcnts problems. With the current implementation kernel
only dumped info at dev.c:5429:

printk(KERN_EMERG "unregister_netdevice: "
       waiting for %s to become free. Usage "
       count = %d\n",
       dev->name, atomic_read(&dev->refcnt));

Daniel.

Re: [PATCH] kref: add function for reading kref value

[Greg KH]

On Mon, Dec 12, 2011 at 06:42:59PM +0200, Daniel Baluta wrote:
> On Mon, Dec 12, 2011 at 5:41 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Le lundi 12 décembre 2011 à 17:39 +0200, Daniel Baluta a écrit :
> >
> >> Ok, for the moment it seems to be a bad idea. But my intention is
> >> to integrate kref to networking code, and then to write a general
> >> debugging tool for refs.
> >>
> >> Tracking down reference count problems is hard, and this tool can
> >> really help everyone.
> >
> > I dont think kref will help you that much, because its used everywhere.
> >
> > Adding a general debugging tool will provide too much noise.
> >
> > Instead, we (network dev) add debugging points where we want.
> 
> Yes, but you have to do this each time you start debugging, for a
> particular referenced counted object.
> 
> We must find a clever solution to avoid the noise. (e.g use
> /proc, /sysfs, /debugfs options to trigger dumping info for
> some/all objects with a certain state).

Then use the dynamic debug infrastructure, which is there to help you
try to handle this type of debugging on-the-fly.

But again, it's not a kref issue, sorry.

greg k-h

Re: [PATCH] kref: add function for reading kref value

[Daniel Baluta]

On Mon, Dec 12, 2011 at 7:33 PM, Greg KH <greg@kroah.com> wrote:
> On Mon, Dec 12, 2011 at 06:42:59PM +0200, Daniel Baluta wrote:
>> On Mon, Dec 12, 2011 at 5:41 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > Le lundi 12 décembre 2011 ŕ 17:39 +0200, Daniel Baluta a écrit :
>> >
>> >> Ok, for the moment it seems to be a bad idea. But my intention is
>> >> to integrate kref to networking code, and then to write a general
>> >> debugging tool for refs.
>> >>
>> >> Tracking down reference count problems is hard, and this tool can
>> >> really help everyone.
>> >
>> > I dont think kref will help you that much, because its used everywhere.
>> >
>> > Adding a general debugging tool will provide too much noise.
>> >
>> > Instead, we (network dev) add debugging points where we want.
>>
>> Yes, but you have to do this each time you start debugging, for a
>> particular referenced counted object.
>>
>> We must find a clever solution to avoid the noise. (e.g use
>> /proc, /sysfs, /debugfs options to trigger dumping info for
>> some/all objects with a certain state).
>
> Then use the dynamic debug infrastructure, which is there to help you
> try to handle this type of debugging on-the-fly.
>
> But again, it's not a kref issue, sorry.

I see. Thanks.

One last remark: Should we encourage people
to use kref implementation, instead of making
their own ?

What are the chances of accepting changes with
respect to this?

Just a few examples:

neighbour.h
295:#define neigh_hold(n)	atomic_inc(&(n)->refcnt)

addrconf.h
219:static inline void in6_dev_hold(struct inet6_dev *idev)

Daniel.

Re: [PATCH] kref: add function for reading kref value

[Eric Dumazet]

Le lundi 12 décembre 2011 à 19:46 +0200, Daniel Baluta a écrit :
> On Mon, Dec 12, 2011 at 7:33 PM, Greg KH <greg@kroah.com> wrote:
> > On Mon, Dec 12, 2011 at 06:42:59PM +0200, Daniel Baluta wrote:
> >> On Mon, Dec 12, 2011 at 5:41 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> > Le lundi 12 décembre 2011 ŕ 17:39 +0200, Daniel Baluta a écrit :
> >> >
> >> >> Ok, for the moment it seems to be a bad idea. But my intention is
> >> >> to integrate kref to networking code, and then to write a general
> >> >> debugging tool for refs.
> >> >>
> >> >> Tracking down reference count problems is hard, and this tool can
> >> >> really help everyone.
> >> >
> >> > I dont think kref will help you that much, because its used everywhere.
> >> >
> >> > Adding a general debugging tool will provide too much noise.
> >> >
> >> > Instead, we (network dev) add debugging points where we want.
> >>
> >> Yes, but you have to do this each time you start debugging, for a
> >> particular referenced counted object.
> >>
> >> We must find a clever solution to avoid the noise. (e.g use
> >> /proc, /sysfs, /debugfs options to trigger dumping info for
> >> some/all objects with a certain state).
> >
> > Then use the dynamic debug infrastructure, which is there to help you
> > try to handle this type of debugging on-the-fly.
> >
> > But again, it's not a kref issue, sorry.
> 
> I see. Thanks.
> 
> One last remark: Should we encourage people
> to use kref implementation, instead of making
> their own ?
> 
> What are the chances of accepting changes with
> respect to this?
> 
> Just a few examples:
> 
> neighbour.h
> 295:#define neigh_hold(n)	atomic_inc(&(n)->refcnt)
> 
> addrconf.h
> 219:static inline void in6_dev_hold(struct inet6_dev *idev)
> 

I dont know how to say this Daniel.

I dont think kref added layer is helping this. Just say no.

Since we convert about all network stack to RCU, we need much better api
than kref is offering.

(atomic_..._not_zero for example)

Re: [PATCH] kref: add function for reading kref value

[Greg KH]

On Mon, Dec 12, 2011 at 07:46:33PM +0200, Daniel Baluta wrote:
> On Mon, Dec 12, 2011 at 7:33 PM, Greg KH <greg@kroah.com> wrote:
> > On Mon, Dec 12, 2011 at 06:42:59PM +0200, Daniel Baluta wrote:
> >> On Mon, Dec 12, 2011 at 5:41 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> > Le lundi 12 décembre 2011 ŕ 17:39 +0200, Daniel Baluta a écrit :
> >> >
> >> >> Ok, for the moment it seems to be a bad idea. But my intention is
> >> >> to integrate kref to networking code, and then to write a general
> >> >> debugging tool for refs.
> >> >>
> >> >> Tracking down reference count problems is hard, and this tool can
> >> >> really help everyone.
> >> >
> >> > I dont think kref will help you that much, because its used everywhere.
> >> >
> >> > Adding a general debugging tool will provide too much noise.
> >> >
> >> > Instead, we (network dev) add debugging points where we want.
> >>
> >> Yes, but you have to do this each time you start debugging, for a
> >> particular referenced counted object.
> >>
> >> We must find a clever solution to avoid the noise. (e.g use
> >> /proc, /sysfs, /debugfs options to trigger dumping info for
> >> some/all objects with a certain state).
> >
> > Then use the dynamic debug infrastructure, which is there to help you
> > try to handle this type of debugging on-the-fly.
> >
> > But again, it's not a kref issue, sorry.
> 
> I see. Thanks.
> 
> One last remark: Should we encourage people
> to use kref implementation, instead of making
> their own ?

Yes, where possible.

But, as the network developers keep pointing out, this is not a place
for a kref.

thanks,

greg k-h

Re: [PATCH] kref: add function for reading kref value

[Eric Dumazet]

Le lundi 12 décembre 2011 à 18:42 +0200, Daniel Baluta a écrit :

> Yes, but you have to do this each time you start debugging, for a
> particular referenced counted object.
> 

What a big deal.

> We must find a clever solution to avoid the noise. (e.g use
> /proc, /sysfs, /debugfs options to trigger dumping info for
> some/all objects with a certain state).
> 
> Usecase:
> 
> At some point we need to know all objects with refcount X,
> and their history of get/put operations.
> 
> For us it would have been very useful when debugging dev
> refcnts problems. With the current implementation kernel
> only dumped info at dev.c:5429:
> 

current on your git repo maybe, not on ours :(

> printk(KERN_EMERG "unregister_netdevice: "
>        waiting for %s to become free. Usage "
>        count = %d\n",
>        dev->name, atomic_read(&dev->refcnt));
> 

This is a very good example where you _cannot_ use kref, since we now
use a percpu refcnt fot netdevice.