On Wed, Feb 12, 2014 at 06:38:21PM +0200, Michael S. Tsirkin wrote:> It is sometimes useful to get the value of the reference count after > decrement. > For example, vhost wants to execute some periodic cleanup operations > once number of references drops below a specific value, before it > reaches zero (for efficiency).You should never care about what the value of the kref is, if you are using it correctly :) So I really don't want to add this function, as I'm sure people will use it incorrectly. You should only care if the reference drops to 0, if not, then your usage doesn't really fit into the "kref" model, and so, just use an atomic variable. I really want to know why it matters for "efficiency" that you know this number. How does that help anything, as the number could then go up later on, and the work you did at a "lower" number is obsolete, right? thanks, greg k-h
On Wed, Feb 12, 2014 at 08:56:30AM -0800, Greg Kroah-Hartman wrote:> On Wed, Feb 12, 2014 at 06:38:21PM +0200, Michael S. Tsirkin wrote: > > It is sometimes useful to get the value of the reference count after > > decrement. > > For example, vhost wants to execute some periodic cleanup operations > > once number of references drops below a specific value, before it > > reaches zero (for efficiency). > > You should never care about what the value of the kref is, if you are > using it correctly :) > > So I really don't want to add this function, as I'm sure people will use > it incorrectly. You should only care if the reference drops to 0, if > not, then your usage doesn't really fit into the "kref" model, and so, > just use an atomic variable.This happens when you have code that keeps reference itself implicitly or explicitly. foo(struct kref *k, int bar) { sub = kref_sub(k) if (sub == 1) FOO(k, bar) /* Here I am the only one with a reference */ } kref_get(k) foo(k, bar); .... kref_put(k) Why not do FOO in destructor you ask? Absolutely but this will be called much later. Maybe you will reconsider if I document this as the only legal use?> > I really want to know why it matters for "efficiency" that you know this > number. How does that help anything, as the number could then go up > later on, and the work you did at a "lower" number is obsolete, right? > > thanks, > > greg k-hThe issue is that if number dropped to 1, this means we must do the cleanup work since there are no outstanding buffers, (last user is ourselves) if we do not cleanup, guest will hang waiting for us. But it never drops to 0 since we have our own reference in the device. If it goes up again this means we didn't have to do cleanup, but an alternative is doing it all the time and that is slow. Yes I can rework vhost to open-code this kref use, it's no big deal. Alternatively since most of the use does match kref model, maybe __kref_sub_return with disclaimers that you must know what you are doing? Please let me know. Thanks! -- MST
On Wed, Feb 12, 2014 at 07:35:24PM +0200, Michael S. Tsirkin wrote:> On Wed, Feb 12, 2014 at 08:56:30AM -0800, Greg Kroah-Hartman wrote: > > On Wed, Feb 12, 2014 at 06:38:21PM +0200, Michael S. Tsirkin wrote: > > > It is sometimes useful to get the value of the reference count after > > > decrement. > > > For example, vhost wants to execute some periodic cleanup operations > > > once number of references drops below a specific value, before it > > > reaches zero (for efficiency). > > > > You should never care about what the value of the kref is, if you are > > using it correctly :) > > > > So I really don't want to add this function, as I'm sure people will use > > it incorrectly. You should only care if the reference drops to 0, if > > not, then your usage doesn't really fit into the "kref" model, and so, > > just use an atomic variable. > > This happens when you have code that keeps > reference itself implicitly or explicitly. > > foo(struct kref *k, int bar) { > > sub = kref_sub(k) > > if (sub == 1) > FOO(k, bar) /* Here I am the only one > with a reference */Why do you care if you are the only one with a reference? If you do, then just don't grab that reference and do the work in the cleanup callback :)> } > > kref_get(k) > foo(k, bar); > .... > kref_put(k) > > Why not do FOO in destructor you ask? > Absolutely but this will be called much later. > > Maybe you will reconsider if I document this > as the only legal use?No one reads documentation :(> > I really want to know why it matters for "efficiency" that you know this > > number. How does that help anything, as the number could then go up > > later on, and the work you did at a "lower" number is obsolete, right? > > > > thanks, > > > > greg k-h > > The issue is that if number dropped to 1, this means > we must do the cleanup work since there are > no outstanding buffers, (last user is ourselves) > if we do not cleanup, > guest will hang waiting for us.This doesn't make sense, nor does it sound like a use for a kref (or you are using it wrong.)> But it never drops to 0 since we have our own reference > in the device.Then don't do that.> If it goes up again this means we didn't have > to do cleanup, but an alternative is doing > it all the time and that is slow.Then just cleanup when it hits 0, like the rest of the world does.> Yes I can rework vhost to open-code this kref use, it's > no big deal. > Alternatively since most of the use does match kref > model, maybe __kref_sub_return with disclaimers > that you must know what you are doing?No, no one reads documentation, sorry. Either fix your use of kref (i.e. don't care about the count), or do something else, as you don't want a kref, but rather, an atomic count of what is going on and "1" means something "special" to you. sorry, greg k-h
Hi On Wed, Feb 12, 2014 at 9:35 AM, Michael S. Tsirkin <mst at redhat.com> wrote:> On Wed, Feb 12, 2014 at 08:56:30AM -0800, Greg Kroah-Hartman wrote: >> On Wed, Feb 12, 2014 at 06:38:21PM +0200, Michael S. Tsirkin wrote: >> > It is sometimes useful to get the value of the reference count after >> > decrement. >> > For example, vhost wants to execute some periodic cleanup operations >> > once number of references drops below a specific value, before it >> > reaches zero (for efficiency). >> >> You should never care about what the value of the kref is, if you are >> using it correctly :) >> >> So I really don't want to add this function, as I'm sure people will use >> it incorrectly. You should only care if the reference drops to 0, if >> not, then your usage doesn't really fit into the "kref" model, and so, >> just use an atomic variable. > > This happens when you have code that keeps > reference itself implicitly or explicitly. > > foo(struct kref *k, int bar) { > > sub = kref_sub(k) > > if (sub == 1)At this moment you cannot be sure that refcount is 1. It might be changed between kref_sub call and this point. The refcount might be 0, 1, ... or any other value. The only value that you can be sure is zero. Once refcount becomes zero there is no way to increase it to positive value as there are no alive pointers to the object.> FOO(k, bar) /* Here I am the only one > with a reference */ > > } > > kref_get(k) > foo(k, bar); > .... > kref_put(k) > > Why not do FOO in destructor you ask? > Absolutely but this will be called much later. > > Maybe you will reconsider if I document this > as the only legal use? > >> >> I really want to know why it matters for "efficiency" that you know this >> number. How does that help anything, as the number could then go up >> later on, and the work you did at a "lower" number is obsolete, right? >> >> thanks, >> >> greg k-h > > The issue is that if number dropped to 1, this means > we must do the cleanup work since there are > no outstanding buffers, (last user is ourselves) > if we do not cleanup, > guest will hang waiting for us. > > But it never drops to 0 since we have our own reference > in the device. > If it goes up again this means we didn't have > to do cleanup, but an alternative is doing > it all the time and that is slow. > > Yes I can rework vhost to open-code this kref use, it's > no big deal. > Alternatively since most of the use does match kref > model, maybe __kref_sub_return with disclaimers > that you must know what you are doing? > Please let me know. > > Thanks! > > -- > MST
From: Greg Kroah-Hartman <gregkh at linuxfoundation.org> Date: Wed, 12 Feb 2014 08:56:30 -0800> On Wed, Feb 12, 2014 at 06:38:21PM +0200, Michael S. Tsirkin wrote: >> It is sometimes useful to get the value of the reference count after >> decrement. >> For example, vhost wants to execute some periodic cleanup operations >> once number of references drops below a specific value, before it >> reaches zero (for efficiency). > > You should never care about what the value of the kref is, if you are > using it correctly :)It isn't being used to determine when to destroy things. They use it to as a heuristic of when to trigger polling. Each ubuf attached gets a kref to the higher level virtio_net buffer holding object, they want to trigger polling when that reference drops to 1 or lower. Right now they are reading the atomic refcount directly, which I think is much worse than this helper.
On Wed, 12 February 2014 19:06:37 -0500, David Miller wrote:> > It isn't being used to determine when to destroy things. > > They use it to as a heuristic of when to trigger polling. > > Each ubuf attached gets a kref to the higher level virtio_net buffer > holding object, they want to trigger polling when that reference drops > to 1 or lower. > > Right now they are reading the atomic refcount directly, which > I think is much worse than this helper.I disagree. Reading the refcount directly is leaving noone under any illusion that this might be a good idea. J?rn -- Victory in war is not repetitious. -- Sun Tzu
On Wed, Feb 12, 2014 at 07:06:37PM -0500, David Miller wrote:> From: Greg Kroah-Hartman <gregkh at linuxfoundation.org> > Date: Wed, 12 Feb 2014 08:56:30 -0800 > > > On Wed, Feb 12, 2014 at 06:38:21PM +0200, Michael S. Tsirkin wrote: > >> It is sometimes useful to get the value of the reference count after > >> decrement. > >> For example, vhost wants to execute some periodic cleanup operations > >> once number of references drops below a specific value, before it > >> reaches zero (for efficiency). > > > > You should never care about what the value of the kref is, if you are > > using it correctly :) > > It isn't being used to determine when to destroy things. > > They use it to as a heuristic of when to trigger polling. > > Each ubuf attached gets a kref to the higher level virtio_net buffer > holding object, they want to trigger polling when that reference drops > to 1 or lower. > > Right now they are reading the atomic refcount directly, which > I think is much worse than this helper.Yes, that's horrible as well, but as was already pointed out in this thread, you can't rely on that value to really be "1" after reading it due to the way krefs work, what happened if someone else just grabbed it? If all they want is a "count" for when to start polling, then use a separate atomic count, but don't abuse the kref interface for this, I don't think that will work properly at all. thanks, greg k-h