On 08/26/15 09:25, Hans Petter Selasky wrote:
On 08/18/15 12:15, Julien Charbon wrote:
Author: jch
Date: Tue Aug 18 10:15:09 2015
New Revision: 286880
URL: https://svnweb.freebsd.org/changeset/base/286880
Log:
callout_stop() should return 0 (fail) when the callout is currently
being serviced and indeed unstoppable.
A scenario to reproduce this case is:
- the callout is being serviced and at same time,
- callout_reset() is called on this callout that sets
the CALLOUT_PENDING flag and at same time,
- callout_stop() is called on this callout and returns 1 (success)
even if the callout is indeed currently running and unstoppable.
This issue was caught up while making r284245 (D2763) workaround, and
was discussed at BSDCan 2015. Once applied the r284245 workaround
is not needed anymore and will be reverted.
Differential Revision: https://reviews.freebsd.org/D3078
Reviewed by: jhb
Sponsored by: Verisign, Inc.
Modified:
head/sys/kern/kern_timeout.c
Modified: head/sys/kern/kern_timeout.c
==============================================================================
--- head/sys/kern/kern_timeout.c Tue Aug 18 10:07:03 2015 (r286879)
+++ head/sys/kern/kern_timeout.c Tue Aug 18 10:15:09 2015 (r286880)
@@ -1150,7 +1150,7 @@ _callout_stop_safe(struct callout *c, in
struct callout_cpu *cc, *old_cc;
struct lock_class *class;
int direct, sq_locked, use_lock;
- int not_on_a_list;
+ int not_on_a_list, not_running;
if (safe)
WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, c->c_lock,
@@ -1378,8 +1378,15 @@ again:
}
}
callout_cc_del(c, cc);
+
+ /*
+ * If we are asked to stop a callout which is currently in progress
+ * and indeed impossible to stop then return 0.
+ */
+ not_running = !(cc_exec_curr(cc, direct) == c);
+
CC_UNLOCK(cc);
- return (1);
+ return (not_running);
}
void
Hi,
I think this patch is incomplete and can break the return value for
non-MPSAFE callouts. I think the correct statement should check the
value of "use_lock" too:
not_running = !(cc_exec_curr(cc, direct) == c && use_lock == 0);
Because if the callback process waits for lock "c_lock" in the callback
process then we have above "cc_exec_curr(cc, direct) == c" is satisfied
too, and non-MPSAFE callouts are always cancelable, via
cc_exec_cancel(cc, direct) = true;
class->lc_lock(c_lock, lock_status);
/*
* The callout may have been cancelled
* while we switched locks.
*/
if (cc_exec_cancel(cc, direct)) {
class->lc_unlock(c_lock);
goto skip;
}
/* The callout cannot be stopped now. */
cc_exec_cancel(cc, direct) = true;
--HPS
Hi,
I'm sorry to say, but I think this change should be backed out as it
changes the behaviour of the callout API. The old code was correct. The
issues should be fixed in the TCP stack instead, like suggested by D1563
and also r284245.
This patch introduces new behaviour to the callout_stop() function,
which is contradicting "man 9 callout" and _not_ documented anywhere!
Reading "man 9 callout" in 11-current:
The callout_reset() and callout_schedule() function families schedule a
future function invocation for callout c. If c already has a pending
callout, it is cancelled before the new invocation is scheduled.
callout_reset() causes a _new_ callout invocation and it is logical that
the return value of callout_stop() reflect operations on the _new_
callout invocation and not the previous one.
The function callout_stop() cancels a callout c if it is currently pend-
ing. If the callout is pending, then callout_stop() returns a non-zero
value. If the callout is not set, has already been serviced, or is cur-
rently being serviced, then zero will be returned. If the callout has an
associated lock, then that lock must be held when this function is
called.
If there are two callout invocations that callout_stop() needs to
handle, it should be called twice.
The correct way is:
callout_reset();
callout_stop();
callout_drain();
For the TCP stack's matter, it should be:
callout_reset();
callout_stop(); /* cancel _new_ callout invocation */
callout_stop(); /* check for any pending callout invokation */
Remember there are other OS'es out there using our TCP/IP stack which do
not have an identical callout subsystem.
--HPS
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
