Date: Wed, 2 May 2018 11:20:24 -0400 From: Mark Johnston <markj@FreeBSD.org> To: freebsd-arch@FreeBSD.org Subject: callout_stop() return value Message-ID: <20180502152024.GA24397@raichu>
next in thread | raw e-mail | index | archive | help
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
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180502152024.GA24397>