Date: Wed, 02 May 2018 09:34:57 -0600 From: Ian Lepore <ian@freebsd.org> To: Mark Johnston <markj@FreeBSD.org>, freebsd-arch@FreeBSD.org Subject: Re: callout_stop() return value Message-ID: <1525275297.57768.202.camel@freebsd.org> In-Reply-To: <20180502152024.GA24397@raichu> References: <20180502152024.GA24397@raichu>
next in thread | previous in thread | raw e-mail | index | archive | help
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
> /*
> _______________________________________________
> freebsd-arch@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.or
> g"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1525275297.57768.202.camel>