[PATCH] kref: add function for reading kref value

[PATCH] kref: add function for reading kref value

[Daniel Baluta]

We can easily get kref refcount value by accesing
kref->refcount but it is better to have a function
for this.

Signed-off-by: Daniel Baluta <dbaluta@ixiacom.com>
---
 include/linux/kref.h |    1 +
 lib/kref.c           |    9 +++++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/linux/kref.h b/include/linux/kref.h
index d4a62ab..cd1b04a 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -22,6 +22,7 @@ struct kref {
 };
 
 void kref_init(struct kref *kref);
+atomic_t kref_read(struct kref *kref);
 void kref_get(struct kref *kref);
 int kref_put(struct kref *kref, void (*release) (struct kref *kref));
 int kref_sub(struct kref *kref, unsigned int count,
diff --git a/lib/kref.c b/lib/kref.c
index 3efb882..48aaf2a 100644
--- a/lib/kref.c
+++ b/lib/kref.c
@@ -25,6 +25,15 @@ void kref_init(struct kref *kref)
 	smp_mb();
 }
 
+/**
+ * kref_read - read refcount for object
+ * @kref: object.
+ */
+void kref_read(struct kref *kref)
+{
+	return atomic_read(&kref->refcount);
+}
+
 /**
  * kref_get - increment refcount for object.
  * @kref: object.
-- 
1.7.1

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

[Belisko Marek]

On Mon, Dec 12, 2011 at 1:44 PM, Daniel Baluta <dbaluta@ixiacom.com> wrote:
> We can easily get kref refcount value by accesing
> kref->refcount but it is better to have a function
> for this.
>
> Signed-off-by: Daniel Baluta <dbaluta@ixiacom.com>
> ---
>  include/linux/kref.h |    1 +
>  lib/kref.c           |    9 +++++++++
>  2 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/kref.h b/include/linux/kref.h
> index d4a62ab..cd1b04a 100644
> --- a/include/linux/kref.h
> +++ b/include/linux/kref.h
> @@ -22,6 +22,7 @@ struct kref {
>  };
>
>  void kref_init(struct kref *kref);
> +atomic_t kref_read(struct kref *kref);
>  void kref_get(struct kref *kref);
>  int kref_put(struct kref *kref, void (*release) (struct kref *kref));
>  int kref_sub(struct kref *kref, unsigned int count,
> diff --git a/lib/kref.c b/lib/kref.c
> index 3efb882..48aaf2a 100644
> --- a/lib/kref.c
> +++ b/lib/kref.c
> @@ -25,6 +25,15 @@ void kref_init(struct kref *kref)
>        smp_mb();
>  }
>
> +/**
> + * kref_read - read refcount for object
> + * @kref: object.
> + */
> +void kref_read(struct kref *kref)
> +{
> +       return atomic_read(&kref->refcount);
> +}
Return void? Shouldn't be: atomic_t. Did you compile your code before
submission???
> +
>  /**
>  * kref_get - increment refcount for object.
>  * @kref: object.
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

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

[Daniel Baluta]

On Mon, Dec 12, 2011 at 2:51 PM, Belisko Marek <marek.belisko@gmail.com> wrote:
> On Mon, Dec 12, 2011 at 1:44 PM, Daniel Baluta <dbaluta@ixiacom.com> wrote:
>> We can easily get kref refcount value by accesing
>> kref->refcount but it is better to have a function
>> for this.
>>
>> Signed-off-by: Daniel Baluta <dbaluta@ixiacom.com>
>> ---
>>  include/linux/kref.h |    1 +
>>  lib/kref.c           |    9 +++++++++
>>  2 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/kref.h b/include/linux/kref.h
>> index d4a62ab..cd1b04a 100644
>> --- a/include/linux/kref.h
>> +++ b/include/linux/kref.h
>> @@ -22,6 +22,7 @@ struct kref {
>>  };
>>
>>  void kref_init(struct kref *kref);
>> +atomic_t kref_read(struct kref *kref);
>>  void kref_get(struct kref *kref);
>>  int kref_put(struct kref *kref, void (*release) (struct kref *kref));
>>  int kref_sub(struct kref *kref, unsigned int count,
>> diff --git a/lib/kref.c b/lib/kref.c
>> index 3efb882..48aaf2a 100644
>> --- a/lib/kref.c
>> +++ b/lib/kref.c
>> @@ -25,6 +25,15 @@ void kref_init(struct kref *kref)
>>        smp_mb();
>>  }
>>
>> +/**
>> + * kref_read - read refcount for object
>> + * @kref: object.
>> + */
>> +void kref_read(struct kref *kref)
>> +{
>> +       return atomic_read(&kref->refcount);
>> +}
> Return void? Shouldn't be: atomic_t. Did you compile your code before
> submission???

You are right. I'll send again with the correct version.

thanks,
Daniel.

[PATCH] kref: add function for reading kref value

[Daniel Baluta]

We can easily get kref refcount value by accesing
kref->refcount but it is better to have a function
for this.

Signed-off-by: Daniel Baluta <dbaluta@ixiacom.com>
---
 include/linux/kref.h |    1 +
 lib/kref.c           |    9 +++++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/linux/kref.h b/include/linux/kref.h
index d4a62ab..f2e69c1 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -22,6 +22,7 @@ struct kref {
 };
 
 void kref_init(struct kref *kref);
+int kref_read(struct kref *kref);
 void kref_get(struct kref *kref);
 int kref_put(struct kref *kref, void (*release) (struct kref *kref));
 int kref_sub(struct kref *kref, unsigned int count,
diff --git a/lib/kref.c b/lib/kref.c
index 3efb882..fc7be37 100644
--- a/lib/kref.c
+++ b/lib/kref.c
@@ -26,6 +26,15 @@ void kref_init(struct kref *kref)
 }
 
 /**
+ * kref_read - read refcount for object
+ * @kref: object.
+ */
+int kref_read(struct kref *kref)
+{
+	return atomic_read(&kref->refcount);
+}
+
+/**
  * kref_get - increment refcount for object.
  * @kref: object.
  */
-- 
1.7.1

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

[Alexey Dobriyan]

On 12/12/11, Daniel Baluta <dbaluta@ixiacom.com> wrote:
> We can easily get kref refcount value by accesing
> kref->refcount but it is better to have a function
> for this.

You're missing a point. Refcount value as-is is not interesting.
You do GET/PUT operations and you don't care.

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

[Greg KH]

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?

greg k-h

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.

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

[Peter Zijlstra]

On Mon, 2011-12-12 at 07:02 -0800, Greg KH 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 very much agree with Greg, those sites that know wth they're doing are
way better off open-coding it like they do now. (I only looked at the
neihgbour.c one, which is creative but not evidently wrong).

Providing a kref_read() function however, suggests its actually a sane
thing to do, its not.