Re: Let callout_reset return if it reschedules a pending callout

2018-02-28 Thread Ryota Ozaki 
On Thu, Mar 1, 2018 at 5:45 AM, Joerg Sonnenberger  wrote:
> On Thu, Mar 01, 2018 at 01:58:29AM +0900, Ryota Ozaki wrote:
>> On Wed, Feb 28, 2018 at 10:11 PM, Joerg Sonnenberger  wrote:
>> > On Wed, Feb 28, 2018 at 05:47:13PM +0900, Ryota Ozaki wrote:
>> >> The feature is useful when you have a reference to an object that is
>> >> passed to a callout. In this case you need to take care of a
>> >> reference leak on callout_reset (and other APIs); it silently
>> >> reschedules (IOW cancels) a pending callout and leaks a reference.
>> >> Unfortunately callout_reset doesn't tell us the reschedule.
>> >
>> > Can we look at this part first before discussing any API changes?
>> > Why can't you store the reference next to callout instance itself?
>>
>> Oh, my explanation was quite confusing... I meant that the object
>> has a reference counter and the counter is incremented to indicate
>> that it is referred by a callout. obj_ref and obj_unref can be
>> thought as obj->refcnt++ and obj->refcnt-- respectively.
>
> Just consider the object reference owned by the callout struct. You
> change the reference count when the callout is destroyed or if the
> callout is reassigned, not because it is executed.

Oh, there is a limitation I didn't mention yet; we can't use callout_halt
to stop the callout when we try to destroy an obj because of the issue of
softnet_lock vs. callout_halt(*).

(*) http://mail-index.netbsd.org/tech-net/2018/01/12/msg006612.html

We can use only callout_stop that doesn't ensure if a callout stops
or not. It requires that we track the activity of the callout, which
is what a refcount does.

  ozaki-r

Re: Let callout_reset return if it reschedules a pending callout

2018-02-28 Thread Joerg Sonnenberger 
On Thu, Mar 01, 2018 at 01:58:29AM +0900, Ryota Ozaki wrote:
> On Wed, Feb 28, 2018 at 10:11 PM, Joerg Sonnenberger  wrote:
> > On Wed, Feb 28, 2018 at 05:47:13PM +0900, Ryota Ozaki wrote:
> >> The feature is useful when you have a reference to an object that is
> >> passed to a callout. In this case you need to take care of a
> >> reference leak on callout_reset (and other APIs); it silently
> >> reschedules (IOW cancels) a pending callout and leaks a reference.
> >> Unfortunately callout_reset doesn't tell us the reschedule.
> >
> > Can we look at this part first before discussing any API changes?
> > Why can't you store the reference next to callout instance itself?
> 
> Oh, my explanation was quite confusing... I meant that the object
> has a reference counter and the counter is incremented to indicate
> that it is referred by a callout. obj_ref and obj_unref can be
> thought as obj->refcnt++ and obj->refcnt-- respectively.

Just consider the object reference owned by the callout struct. You
change the reference count when the callout is destroyed or if the
callout is reassigned, not because it is executed.

Joerg

Re: Let callout_reset return if it reschedules a pending callout

2018-02-28 Thread Ryota Ozaki 
On Wed, Feb 28, 2018 at 10:11 PM, Joerg Sonnenberger  wrote:
> On Wed, Feb 28, 2018 at 05:47:13PM +0900, Ryota Ozaki wrote:
>> The feature is useful when you have a reference to an object that is
>> passed to a callout. In this case you need to take care of a
>> reference leak on callout_reset (and other APIs); it silently
>> reschedules (IOW cancels) a pending callout and leaks a reference.
>> Unfortunately callout_reset doesn't tell us the reschedule.
>
> Can we look at this part first before discussing any API changes?
> Why can't you store the reference next to callout instance itself?

Oh, my explanation was quite confusing... I meant that the object
has a reference counter and the counter is incremented to indicate
that it is referred by a callout. obj_ref and obj_unref can be
thought as obj->refcnt++ and obj->refcnt-- respectively.

  ozaki-r

Re: Let callout_reset return if it reschedules a pending callout

2018-02-28 Thread Joerg Sonnenberger 
On Wed, Feb 28, 2018 at 05:47:13PM +0900, Ryota Ozaki wrote:
> The feature is useful when you have a reference to an object that is
> passed to a callout. In this case you need to take care of a
> reference leak on callout_reset (and other APIs); it silently
> reschedules (IOW cancels) a pending callout and leaks a reference.
> Unfortunately callout_reset doesn't tell us the reschedule.

Can we look at this part first before discussing any API changes?
Why can't you store the reference next to callout instance itself?

Joerg

Let callout_reset return if it reschedules a pending callout

2018-02-28 Thread Ryota Ozaki 
Hi,

This proposal lets callout_reset and callout_schedule return true if
it reschedules a pending callout and return false otherwise.

The feature is useful when you have a reference to an object that is
passed to a callout. In this case you need to take care of a
reference leak on callout_reset (and other APIs); it silently
reschedules (IOW cancels) a pending callout and leaks a reference.
Unfortunately callout_reset doesn't tell us the reschedule.

To avoid the leak with the current APIs of callout, we have to do
something like this:

  if (callout_pending(>ch)) {
expired = callout_stop(>ch);
if (!expired)
  obj_unref(obj);
  }
  obj_ref(obj);
  callout_reset(>ch);

callout_pending is required to exclude the case that the callout has
never been scheduled because callout_stop just checks CALLOUT_FIRED.

The proposal makes the code simple like this:

  obj_ref(obj);
  canceled = callout_reset(>ch);
  if (canceled)
obj_unref(obj);

This is a patch (quite simple):
  http://www.netbsd.org/~ozaki-r/callout_reset.diff

Note that this proposal changes API/ABI however we don't need to
tweak existing callers of callout_reset/callout_schedule because
it doesn't break the original usage.

Any comments?

Thanks,
  ozaki-r