callout_stop() return value

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

callout_stop() return value

Mark Johnston-2
Hi,

We have a few pieces of code that follow a pattern: a thread allocates a
resource and schedules a callout. The callout handler releases the
resource and never reschedules itself. In the common case, a thread
will cancel the callout before it executes. To know whether it must
release the resource associated with the callout, this thread must know
whether the pending callout was cancelled.

Before r302350, our code happened to work; it could use the return value
of callout_stop() to correctly determine whether to release the
resource. Now, however, it cannot: the return value does not contain
sufficient information. In particular, if the return value is 0, we do
not know whether a future invocation of the callout was cancelled.

The resource's lifetime is effectively tied to the scheduling of the
callout, so I think it's preferable to address this by extending the
callout API rather than by adding some extra state to the structure
containing the callout. Does anyone have any opinions on adding a new
flag to _callout_stop_safe(), somewhat akin to CS_EXECUTING? When this
flag is specified, _callout_stop_safe() will return 1 if a future
invocation was cancelled, even if the callout was running at the time of
the call.

The patch below implements this suggestion, but I haven't yet tested
it and was wondering if anyone had opinions on how to handle this
scenario. If I end up going ahead with this approach, I'll be sure to
update the callout man page with a description of CS_EXECUTING and the
new flag. Thanks in advance.

diff --git a/sys/kern/kern_timeout.c b/sys/kern/kern_timeout.c
index 81b4a14ecf08..23c8c7dc3675 100644
--- a/sys/kern/kern_timeout.c
+++ b/sys/kern/kern_timeout.c
@@ -1183,6 +1183,8 @@ _callout_stop_safe(struct callout *c, int flags, void (*drain)(void *))
  int direct, sq_locked, use_lock;
  int cancelled, not_on_a_list;
 
+ KASSERT((flags & (CS_EXECUTING | CS_DESCHEDULED)) !=
+    (CS_EXECUTING | CS_DESCHEDULED), ("invalid flags %#x", flags));
  if ((flags & CS_DRAIN) != 0)
  WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, c->c_lock,
     "calling %s", __func__);
@@ -1391,7 +1393,7 @@ _callout_stop_safe(struct callout *c, int flags, void (*drain)(void *))
  KASSERT(!sq_locked, ("sleepqueue chain still locked"));
  cancelled = ((flags & CS_EXECUTING) != 0);
  } else
- cancelled = 1;
+ cancelled = ((flags & CS_DESCHEDULED) == 0);
 
  if (sq_locked)
  sleepq_release(&cc_exec_waiting(cc, direct));
@@ -1422,6 +1424,8 @@ _callout_stop_safe(struct callout *c, int flags, void (*drain)(void *))
  } else {
  TAILQ_REMOVE(&cc->cc_expireq, c, c_links.tqe);
  }
+ if ((flags & CS_DESCHEDULED) != 0)
+ cancelled = 1;
  }
  callout_cc_del(c, cc);
  CC_UNLOCK(cc);
diff --git a/sys/sys/callout.h b/sys/sys/callout.h
index e4d91c69da3f..5142b3255122 100644
--- a/sys/sys/callout.h
+++ b/sys/sys/callout.h
@@ -70,6 +70,9 @@ struct callout_handle {
 #define CS_DRAIN 0x0001 /* callout_drain(), wait allowed */
 #define CS_EXECUTING 0x0002 /* Positive return value indicates that
   the callout was executing */
+#define CS_DESCHEDULED 0x0004 /* Positive return value indicates that
+  a future invocation of the callout was
+  cancelled. */
 
 #ifdef _KERNEL
 /*
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: callout_stop() return value

Ian Lepore-3
On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote:

> Hi,
>
> We have a few pieces of code that follow a pattern: a thread
> allocates a
> resource and schedules a callout. The callout handler releases the
> resource and never reschedules itself. In the common case, a thread
> will cancel the callout before it executes. To know whether it must
> release the resource associated with the callout, this thread must
> know
> whether the pending callout was cancelled.
>

It seems to me a better solution would be to track the state / lifetime
of the resource separately rather than trying to infer the state of the
resource from the state of the callout as viewed through a semi-opaque
interface.

If the original thread that schedules the callout keeps enough
information about the resource to free it after cancelling, then it is
already maintaining some kind of sideband info about the resource, even
if it's just a pointer to be freed. Couldn't the callout routine
manipulate this resource tracking info (null out the pointer or set a
bool or whatever), then after cancelling you don't really care whether
the callout ran or not, you just examine the pointer/bool/whatever and
free or not based on that.

-- Ian


> Before r302350, our code happened to work; it could use the return
> value
> of callout_stop() to correctly determine whether to release the
> resource. Now, however, it cannot: the return value does not contain
> sufficient information. In particular, if the return value is 0, we
> do
> not know whether a future invocation of the callout was cancelled.
>
> The resource's lifetime is effectively tied to the scheduling of the
> callout, so I think it's preferable to address this by extending the
> callout API rather than by adding some extra state to the structure
> containing the callout. Does anyone have any opinions on adding a new
> flag to _callout_stop_safe(), somewhat akin to CS_EXECUTING? When
> this
> flag is specified, _callout_stop_safe() will return 1 if a future
> invocation was cancelled, even if the callout was running at the time
> of
> the call.
>
> The patch below implements this suggestion, but I haven't yet tested
> it and was wondering if anyone had opinions on how to handle this
> scenario. If I end up going ahead with this approach, I'll be sure to
> update the callout man page with a description of CS_EXECUTING and
> the
> new flag. Thanks in advance.
>
> diff --git a/sys/kern/kern_timeout.c b/sys/kern/kern_timeout.c
> index 81b4a14ecf08..23c8c7dc3675 100644
> --- a/sys/kern/kern_timeout.c
> +++ b/sys/kern/kern_timeout.c
> @@ -1183,6 +1183,8 @@ _callout_stop_safe(struct callout *c, int
> flags, void (*drain)(void *))
>   int direct, sq_locked, use_lock;
>   int cancelled, not_on_a_list;
>  
> + KASSERT((flags & (CS_EXECUTING | CS_DESCHEDULED)) !=
> +     (CS_EXECUTING | CS_DESCHEDULED), ("invalid flags %#x",
> flags));
>   if ((flags & CS_DRAIN) != 0)
>   WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, c->c_lock,
>       "calling %s", __func__);
> @@ -1391,7 +1393,7 @@ _callout_stop_safe(struct callout *c, int
> flags, void (*drain)(void *))
>   KASSERT(!sq_locked, ("sleepqueue chain still
> locked"));
>   cancelled = ((flags & CS_EXECUTING) != 0);
>   } else
> - cancelled = 1;
> + cancelled = ((flags & CS_DESCHEDULED) == 0);
>  
>   if (sq_locked)
>   sleepq_release(&cc_exec_waiting(cc, direct));
> @@ -1422,6 +1424,8 @@ _callout_stop_safe(struct callout *c, int
> flags, void (*drain)(void *))
>   } else {
>   TAILQ_REMOVE(&cc->cc_expireq, c,
> c_links.tqe);
>   }
> + if ((flags & CS_DESCHEDULED) != 0)
> + cancelled = 1;
>   }
>   callout_cc_del(c, cc);
>   CC_UNLOCK(cc);
> diff --git a/sys/sys/callout.h b/sys/sys/callout.h
> index e4d91c69da3f..5142b3255122 100644
> --- a/sys/sys/callout.h
> +++ b/sys/sys/callout.h
> @@ -70,6 +70,9 @@ struct callout_handle {
>  #define CS_DRAIN 0x0001 /* callout_drain(),
> wait allowed */
>  #define CS_EXECUTING 0x0002 /* Positive return
> value indicates that
>     the callout was executing
> */
> +#define CS_DESCHEDULED 0x0004 /* Positive
> return value indicates that
> +   a future invocation of the
> callout was
> +   cancelled. */
>  
>  #ifdef _KERNEL
>  /* 
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "[hidden email]
> g"
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: callout_stop() return value

Mark Johnston-2
On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote:

> On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote:
> > Hi,
> >
> > We have a few pieces of code that follow a pattern: a thread
> > allocates a
> > resource and schedules a callout. The callout handler releases the
> > resource and never reschedules itself. In the common case, a thread
> > will cancel the callout before it executes. To know whether it must
> > release the resource associated with the callout, this thread must
> > know
> > whether the pending callout was cancelled.
> >
>
> It seems to me a better solution would be to track the state / lifetime
> of the resource separately rather than trying to infer the state of the
> resource from the state of the callout as viewed through a semi-opaque
> interface.
>
> If the original thread that schedules the callout keeps enough
> information about the resource to free it after cancelling, then it is
> already maintaining some kind of sideband info about the resource, even
> if it's just a pointer to be freed. Couldn't the callout routine
> manipulate this resource tracking info (null out the pointer or set a
> bool or whatever), then after cancelling you don't really care whether
> the callout ran or not, you just examine the pointer/bool/whatever and
> free or not based on that.

I'd considered that. It's not quite as elegant a solution as you
suggest, since in my case the resource is embedded in an opaque
structure, so I need to add an extra field beside the callout to track
state that's already tracked by the callout subsystem. That plus the
fact that we have multiple instances of this bug make me want to fix it
in a general way, though I recognize that the callout API is already
overly complicated.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: callout_stop() return value

Konstantin Belousov
On Wed, May 02, 2018 at 11:42:37AM -0400, Mark Johnston wrote:

> On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote:
> > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote:
> > > Hi,
> > >
> > > We have a few pieces of code that follow a pattern: a thread
> > > allocates a
> > > resource and schedules a callout. The callout handler releases the
> > > resource and never reschedules itself. In the common case, a thread
> > > will cancel the callout before it executes. To know whether it must
> > > release the resource associated with the callout, this thread must
> > > know
> > > whether the pending callout was cancelled.
> > >
> >
> > It seems to me a better solution would be to track the state / lifetime
> > of the resource separately rather than trying to infer the state of the
> > resource from the state of the callout as viewed through a semi-opaque
> > interface.
> >
> > If the original thread that schedules the callout keeps enough
> > information about the resource to free it after cancelling, then it is
> > already maintaining some kind of sideband info about the resource, even
> > if it's just a pointer to be freed. Couldn't the callout routine
> > manipulate this resource tracking info (null out the pointer or set a
> > bool or whatever), then after cancelling you don't really care whether
> > the callout ran or not, you just examine the pointer/bool/whatever and
> > free or not based on that.
>
> I'd considered that. It's not quite as elegant a solution as you
> suggest, since in my case the resource is embedded in an opaque
> structure, so I need to add an extra field beside the callout to track
> state that's already tracked by the callout subsystem. That plus the
> fact that we have multiple instances of this bug make me want to fix it
> in a general way, though I recognize that the callout API is already
> overly complicated.

I gave up on trying to get this fixed.  r302350 was not the first time
the return value was broken.

I had to do r303426 exactly for this reason.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: callout_stop() return value

Mark Johnston-2
On Wed, May 02, 2018 at 07:24:12PM +0300, Konstantin Belousov wrote:

> On Wed, May 02, 2018 at 11:42:37AM -0400, Mark Johnston wrote:
> > On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote:
> > > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote:
> > > > Hi,
> > > >
> > > > We have a few pieces of code that follow a pattern: a thread
> > > > allocates a
> > > > resource and schedules a callout. The callout handler releases the
> > > > resource and never reschedules itself. In the common case, a thread
> > > > will cancel the callout before it executes. To know whether it must
> > > > release the resource associated with the callout, this thread must
> > > > know
> > > > whether the pending callout was cancelled.
> > > >
> > >
> > > It seems to me a better solution would be to track the state / lifetime
> > > of the resource separately rather than trying to infer the state of the
> > > resource from the state of the callout as viewed through a semi-opaque
> > > interface.
> > >
> > > If the original thread that schedules the callout keeps enough
> > > information about the resource to free it after cancelling, then it is
> > > already maintaining some kind of sideband info about the resource, even
> > > if it's just a pointer to be freed. Couldn't the callout routine
> > > manipulate this resource tracking info (null out the pointer or set a
> > > bool or whatever), then after cancelling you don't really care whether
> > > the callout ran or not, you just examine the pointer/bool/whatever and
> > > free or not based on that.
> >
> > I'd considered that. It's not quite as elegant a solution as you
> > suggest, since in my case the resource is embedded in an opaque
> > structure, so I need to add an extra field beside the callout to track
> > state that's already tracked by the callout subsystem. That plus the
> > fact that we have multiple instances of this bug make me want to fix it
> > in a general way, though I recognize that the callout API is already
> > overly complicated.

I forgot to add that in some cases, extra synchronization would be
needed to maintain this hypothetical flag. Specifically, some of these
callouts do not have an associated lock, so the callout handler doesn't
have an easy way to mark the resource as consumed.

> I gave up on trying to get this fixed.  r302350 was not the first time
> the return value was broken.
>
> I had to do r303426 exactly for this reason.

What kind of fix did you envision? The problem seems to be that
callout_stop()'s return value simply does not contain enough information
for certain use cases.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: callout_stop() return value

Konstantin Belousov
On Wed, May 02, 2018 at 12:40:02PM -0400, Mark Johnston wrote:

> On Wed, May 02, 2018 at 07:24:12PM +0300, Konstantin Belousov wrote:
> > On Wed, May 02, 2018 at 11:42:37AM -0400, Mark Johnston wrote:
> > > On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote:
> > > > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote:
> > > > > Hi,
> > > > >
> > > > > We have a few pieces of code that follow a pattern: a thread
> > > > > allocates a
> > > > > resource and schedules a callout. The callout handler releases the
> > > > > resource and never reschedules itself. In the common case, a thread
> > > > > will cancel the callout before it executes. To know whether it must
> > > > > release the resource associated with the callout, this thread must
> > > > > know
> > > > > whether the pending callout was cancelled.
> > > > >
> > > >
> > > > It seems to me a better solution would be to track the state / lifetime
> > > > of the resource separately rather than trying to infer the state of the
> > > > resource from the state of the callout as viewed through a semi-opaque
> > > > interface.
> > > >
> > > > If the original thread that schedules the callout keeps enough
> > > > information about the resource to free it after cancelling, then it is
> > > > already maintaining some kind of sideband info about the resource, even
> > > > if it's just a pointer to be freed. Couldn't the callout routine
> > > > manipulate this resource tracking info (null out the pointer or set a
> > > > bool or whatever), then after cancelling you don't really care whether
> > > > the callout ran or not, you just examine the pointer/bool/whatever and
> > > > free or not based on that.
> > >
> > > I'd considered that. It's not quite as elegant a solution as you
> > > suggest, since in my case the resource is embedded in an opaque
> > > structure, so I need to add an extra field beside the callout to track
> > > state that's already tracked by the callout subsystem. That plus the
> > > fact that we have multiple instances of this bug make me want to fix it
> > > in a general way, though I recognize that the callout API is already
> > > overly complicated.
>
> I forgot to add that in some cases, extra synchronization would be
> needed to maintain this hypothetical flag. Specifically, some of these
> callouts do not have an associated lock, so the callout handler doesn't
> have an easy way to mark the resource as consumed.
>
> > I gave up on trying to get this fixed.  r302350 was not the first time
> > the return value was broken.
> >
> > I had to do r303426 exactly for this reason.
>
> What kind of fix did you envision? The problem seems to be that
> callout_stop()'s return value simply does not contain enough information
> for certain use cases.

The fix is really social, to make people who change the callout_stop() KPI
to care about non-tcp stack uses.

I tried to add a flag to _callout_stop_safe() to make return values useful
for sleepqueues, see r296320.  But as I said, it was broken again and I
gave up.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: callout_stop() return value

Ian Lepore-3
In reply to this post by Mark Johnston-2
On Wed, 2018-05-02 at 12:40 -0400, Mark Johnston wrote:

> On Wed, May 02, 2018 at 07:24:12PM +0300, Konstantin Belousov wrote:
> >
> > On Wed, May 02, 2018 at 11:42:37AM -0400, Mark Johnston wrote:
> > >
> > > On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote:
> > > >
> > > > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > We have a few pieces of code that follow a pattern: a thread
> > > > > allocates a
> > > > > resource and schedules a callout. The callout handler releases the
> > > > > resource and never reschedules itself. In the common case, a thread
> > > > > will cancel the callout before it executes. To know whether it must
> > > > > release the resource associated with the callout, this thread must
> > > > > know
> > > > > whether the pending callout was cancelled.
> > > > >
> > > > It seems to me a better solution would be to track the state / lifetime
> > > > of the resource separately rather than trying to infer the state of the
> > > > resource from the state of the callout as viewed through a semi-opaque
> > > > interface.
> > > >
> > > > If the original thread that schedules the callout keeps enough
> > > > information about the resource to free it after cancelling, then it is
> > > > already maintaining some kind of sideband info about the resource, even
> > > > if it's just a pointer to be freed. Couldn't the callout routine
> > > > manipulate this resource tracking info (null out the pointer or set a
> > > > bool or whatever), then after cancelling you don't really care whether
> > > > the callout ran or not, you just examine the pointer/bool/whatever and
> > > > free or not based on that.
> > > I'd considered that. It's not quite as elegant a solution as you
> > > suggest, since in my case the resource is embedded in an opaque
> > > structure, so I need to add an extra field beside the callout to track
> > > state that's already tracked by the callout subsystem. That plus the
> > > fact that we have multiple instances of this bug make me want to fix it
> > > in a general way, though I recognize that the callout API is already
> > > overly complicated.
> I forgot to add that in some cases, extra synchronization would be
> needed to maintain this hypothetical flag. Specifically, some of these
> callouts do not have an associated lock, so the callout handler doesn't
> have an easy way to mark the resource as consumed.
>

Wouldn't there be implied temporal synchronization? The callout will
free the resource and manipulate the sideband info to say so, knowing
that nothing else will modify it or even examine it at the same time
because the callout would be cancelled and drained before the resource
is manipulated by someone else. Conversely, the someone-else doesn't
examine or modify the resource or sideband info until the callout is
drained (whether cancelled or it just runs to normal completion).

Only if it were impossible to reliably cancel the callout and reach a
point where you know it's not running would there be any chance of a
race that needs explicit locking.

-- Ian

> >
> > I gave up on trying to get this fixed.  r302350 was not the first time
> > the return value was broken.
> >
> > I had to do r303426 exactly for this reason.
> What kind of fix did you envision? The problem seems to be that
> callout_stop()'s return value simply does not contain enough information
> for certain use cases.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: callout_stop() return value

Mark Johnston-2
On Wed, May 02, 2018 at 11:02:09AM -0600, Ian Lepore wrote:

> On Wed, 2018-05-02 at 12:40 -0400, Mark Johnston wrote:
> > On Wed, May 02, 2018 at 07:24:12PM +0300, Konstantin Belousov wrote:
> > >
> > > On Wed, May 02, 2018 at 11:42:37AM -0400, Mark Johnston wrote:
> > > >
> > > > On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote:
> > > > >
> > > > > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > We have a few pieces of code that follow a pattern: a thread
> > > > > > allocates a
> > > > > > resource and schedules a callout. The callout handler releases the
> > > > > > resource and never reschedules itself. In the common case, a thread
> > > > > > will cancel the callout before it executes. To know whether it must
> > > > > > release the resource associated with the callout, this thread must
> > > > > > know
> > > > > > whether the pending callout was cancelled.
> > > > > >
> > > > > It seems to me a better solution would be to track the state / lifetime
> > > > > of the resource separately rather than trying to infer the state of the
> > > > > resource from the state of the callout as viewed through a semi-opaque
> > > > > interface.
> > > > >
> > > > > If the original thread that schedules the callout keeps enough
> > > > > information about the resource to free it after cancelling, then it is
> > > > > already maintaining some kind of sideband info about the resource, even
> > > > > if it's just a pointer to be freed. Couldn't the callout routine
> > > > > manipulate this resource tracking info (null out the pointer or set a
> > > > > bool or whatever), then after cancelling you don't really care whether
> > > > > the callout ran or not, you just examine the pointer/bool/whatever and
> > > > > free or not based on that.
> > > > I'd considered that. It's not quite as elegant a solution as you
> > > > suggest, since in my case the resource is embedded in an opaque
> > > > structure, so I need to add an extra field beside the callout to track
> > > > state that's already tracked by the callout subsystem. That plus the
> > > > fact that we have multiple instances of this bug make me want to fix it
> > > > in a general way, though I recognize that the callout API is already
> > > > overly complicated.
> > I forgot to add that in some cases, extra synchronization would be
> > needed to maintain this hypothetical flag. Specifically, some of these
> > callouts do not have an associated lock, so the callout handler doesn't
> > have an easy way to mark the resource as consumed.
> >
>
> Wouldn't there be implied temporal synchronization? The callout will
> free the resource and manipulate the sideband info to say so, knowing
> that nothing else will modify it or even examine it at the same time
> because the callout would be cancelled and drained before the resource
> is manipulated by someone else. Conversely, the someone-else doesn't
> examine or modify the resource or sideband info until the callout is
> drained (whether cancelled or it just runs to normal completion).

In one case, the callout isn't drained but simply stopped. Only if
callout_stop() returns 1 (i.e., we cancelled a future invocation AND the
callout wasn't running at the time of the call) do we release the
resource. However, if the call returns 0 we don't know whether to
release the resource.

In this case though, I think it's sufficient to check whether
callout_pending() is true before calling callout_stop(). I will spend
some time trying to fix these issues locally before trying to push this
topic further.

> Only if it were impossible to reliably cancel the callout and reach a
> point where you know it's not running would there be any chance of a
> race that needs explicit locking.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: callout_stop() return value

Hans Petter Selasky-6
In reply to this post by Mark Johnston-2
On 05/02/18 17:20, Mark Johnston wrote:

> Hi,
>
> We have a few pieces of code that follow a pattern: a thread allocates a
> resource and schedules a callout. The callout handler releases the
> resource and never reschedules itself. In the common case, a thread
> will cancel the callout before it executes. To know whether it must
> release the resource associated with the callout, this thread must know
> whether the pending callout was cancelled.
>
> Before r302350, our code happened to work; it could use the return value
> of callout_stop() to correctly determine whether to release the
> resource. Now, however, it cannot: the return value does not contain
> sufficient information. In particular, if the return value is 0, we do
> not know whether a future invocation of the callout was cancelled.
>
> The resource's lifetime is effectively tied to the scheduling of the
> callout, so I think it's preferable to address this by extending the
> callout API rather than by adding some extra state to the structure
> containing the callout. Does anyone have any opinions on adding a new
> flag to _callout_stop_safe(), somewhat akin to CS_EXECUTING? When this
> flag is specified, _callout_stop_safe() will return 1 if a future
> invocation was cancelled, even if the callout was running at the time of
> the call.
>
> The patch below implements this suggestion, but I haven't yet tested
> it and was wondering if anyone had opinions on how to handle this
> scenario. If I end up going ahead with this approach, I'll be sure to
> update the callout man page with a description of CS_EXECUTING and the
> new flag. Thanks in advance.
>

Hi,

You cannot add nor change the current callout_xxx() return values!

There is a lot of code in the kernel (TCP stack for example) which
simply compares this value with < > != and ==   !  Be warned !

To force all callers to re-evaluate the return value, I suggest to
return the callout state as a bitmap structure. See:

https://svnweb.freebsd.org/base/projects/hps_head

> 55 /* return value for all callout_xxx() functions */
> 56 typedef union callout_ret {
> 57        struct {
> 58                unsigned cancelled : 1;
> 59                unsigned draining : 1;
> 60                unsigned reserved : 30;
> 61        } bit;
> 62        unsigned value;
> 63 } callout_ret_t;

The clang compiler treats this structure as an integer.

Example:

>  if (callout_async_drain(t_callout, tcp_timer_discard).bit.draining) {

--HPS
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"