Re: svn commit: r286880 - head/sys/kern
Hi Konstantin, On 27/08/15 19:19, Konstantin Belousov wrote: On Thu, Aug 27, 2015 at 06:28:03PM +0200, Julien Charbon wrote: On 27/08/15 12:49, Konstantin Belousov wrote: On Wed, Aug 26, 2015 at 08:14:15PM +0200, Julien Charbon wrote: As I said, I am not opposed to back out this change, callout(9) API in mpsafe mode is a already complex/subtle API, it won't change too much the current complexity. Let say that if nobody screams until Friday 8/28, I will put back r284245 and revert this change _and_ I will make this case clear in the man page. [Replying to a random message in the whole set of conversations] There is one more case, besides TCP timers, which is equially, of not more, critical and sensitive WRT to the callout_stop(). Look at the sys/kern/subr_sleepqueue.c:sleepq_check_timeout(). Stray return of the false result from callout_stop() causes creation of the non-killable processes: the callout fired, we missed the wakeup and went to sleep by manually doing the context switch. Such thread cannot be woken up. I suspect that your fix is a better approach than my attempt to look at something similar at PR 200992 (may be not). This change (r286880) won't improve the PR 200992: r286880 only addresses a case where callout_stop() returns 1 instead of 0. Thus the only thing that can do r286880 to PR 200992: - Don't change anything the issues - Worsen the issue Sorry to kill your hope of a simple and elegant fix for PR 200992. Well, not that I am frustrated much. Thank you for taking a look. Did you read the patch attached to the PR ? Right, I read it from here: https://bugs.freebsd.org/bugzilla/attachment.cgi?id=157915action=diff And I am not confident enough to say if it is the right way go. I became knowledgeable on callout calls from TCP timers (D2079 and D2763), but that's it. I would propose rrs (as he introduced the 'not_on_a_list' logic) and jhb for reviewing your patch. And unlike me (D3078), don't underestimate the callout complexity in mpsafe mode. :) -- Julien signature.asc Description: OpenPGP digital signature
Re: svn commit: r286880 - head/sys/kern
On Thu, Aug 27, 2015 at 06:28:03PM +0200, Julien Charbon wrote: Hi Konstantin, On 27/08/15 12:49, Konstantin Belousov wrote: On Wed, Aug 26, 2015 at 08:14:15PM +0200, Julien Charbon wrote: As I said, I am not opposed to back out this change, callout(9) API in mpsafe mode is a already complex/subtle API, it won't change too much the current complexity. Let say that if nobody screams until Friday 8/28, I will put back r284245 and revert this change _and_ I will make this case clear in the man page. [Replying to a random message in the whole set of conversations] There is one more case, besides TCP timers, which is equially, of not more, critical and sensitive WRT to the callout_stop(). Look at the sys/kern/subr_sleepqueue.c:sleepq_check_timeout(). Stray return of the false result from callout_stop() causes creation of the non-killable processes: the callout fired, we missed the wakeup and went to sleep by manually doing the context switch. Such thread cannot be woken up. I suspect that your fix is a better approach than my attempt to look at something similar at PR 200992 (may be not). This change (r286880) won't improve the PR 200992: r286880 only addresses a case where callout_stop() returns 1 instead of 0. Thus the only thing that can do r286880 to PR 200992: - Don't change anything the issues - Worsen the issue Sorry to kill your hope of a simple and elegant fix for PR 200992. Well, not that I am frustrated much. Thank you for taking a look. Did you read the patch attached to the PR ? ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r286880 - head/sys/kern
Hi Konstantin, On 27/08/15 12:49, Konstantin Belousov wrote: On Wed, Aug 26, 2015 at 08:14:15PM +0200, Julien Charbon wrote: As I said, I am not opposed to back out this change, callout(9) API in mpsafe mode is a already complex/subtle API, it won't change too much the current complexity. Let say that if nobody screams until Friday 8/28, I will put back r284245 and revert this change _and_ I will make this case clear in the man page. [Replying to a random message in the whole set of conversations] There is one more case, besides TCP timers, which is equially, of not more, critical and sensitive WRT to the callout_stop(). Look at the sys/kern/subr_sleepqueue.c:sleepq_check_timeout(). Stray return of the false result from callout_stop() causes creation of the non-killable processes: the callout fired, we missed the wakeup and went to sleep by manually doing the context switch. Such thread cannot be woken up. I suspect that your fix is a better approach than my attempt to look at something similar at PR 200992 (may be not). This change (r286880) won't improve the PR 200992: r286880 only addresses a case where callout_stop() returns 1 instead of 0. Thus the only thing that can do r286880 to PR 200992: - Don't change anything the issues - Worsen the issue Sorry to kill your hope of a simple and elegant fix for PR 200992. -- Julien signature.asc Description: OpenPGP digital signature
Re: svn commit: r286880 - head/sys/kern
On Wed, Aug 26, 2015 at 08:14:15PM +0200, Julien Charbon wrote: As I said, I am not opposed to back out this change, callout(9) API in mpsafe mode is a already complex/subtle API, it won't change too much the current complexity. Let say that if nobody screams until Friday 8/28, I will put back r284245 and revert this change _and_ I will make this case clear in the man page. [Replying to a random message in the whole set of conversations] There is one more case, besides TCP timers, which is equially, of not more, critical and sensitive WRT to the callout_stop(). Look at the sys/kern/subr_sleepqueue.c:sleepq_check_timeout(). Stray return of the false result from callout_stop() causes creation of the non-killable processes: the callout fired, we missed the wakeup and went to sleep by manually doing the context switch. Such thread cannot be woken up. I suspect that your fix is a better approach than my attempt to look at something similar at PR 200992 (may be not). ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r286880 - head/sys/kern
Hi Hans, On 26/08/15 18:31, Julien Charbon wrote: On 26/08/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.cTue Aug 18 10:07:03 2015(r286879) +++ head/sys/kern/kern_timeout.cTue 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 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; Hum, interesting let me double check that. After checking the non-mpsafe callout cases, you are completely right, this test makes sense only in mpsafe callout case. Fixed in r287196: https://svnweb.freebsd.org/base?view=revisionrevision=287196 Thanks a lot for your time. -- Julien signature.asc Description: OpenPGP digital signature
Re: svn commit: r286880 - head/sys/kern
Hi Julien, On 08/27/15 10:17, Julien Charbon wrote: Hi Hans, After checking the non-mpsafe callout cases, you are completely right, this test makes sense only in mpsafe callout case. Fixed in r287196: https://svnweb.freebsd.org/base?view=revisionrevision=287196 Thanks a lot for your time. Possibly you'll need to add an: else not_running = 1; To restore the old behaviour for non-MPSAFE callouts, because the variable is not initialized at top of the function. --HPS ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r286880 - head/sys/kern
Hi Hans, On 26/08/15 20:31, Hans Petter Selasky wrote: On 08/26/15 20:14, Julien Charbon wrote: Let say that if nobody screams until Friday 8/28, I will put back r284245 and revert this change_and_ I will make this case clear in the man page. If you can update the manual page about this special case for MPSAFE callouts only I presume, then its totally fine. Good idea, I am going to update the callout(9) man page to make clear that in mpsafe case if a callout is pending and currently being serviced callout_stop() returns 0 (fail). Then I can update my projects/hps_head to follow that new change, which now is a bit broken. You might also want to check existing MPSAFE consumers in the kernel, if this API change makes any difference. I checked all the mpsafe callouts that check callout_stop() return value (actually only a few are testing callout_stop() return value), and none relies on the old behavior (i.e. before r286880). I was thinking if user-space TCP might be affected by this. CC'ing Adrian. Interesting. -- Julien signature.asc Description: OpenPGP digital signature
Re: svn commit: r286880 - head/sys/kern
On 08/26/15 20:14, Julien Charbon wrote: Let say that if nobody screams until Friday 8/28, I will put back r284245 and revert this change_and_ I will make this case clear in the man page. Hi, If you can update the manual page about this special case for MPSAFE callouts only I presume, then its totally fine. Then I can update my projects/hps_head to follow that new change, which now is a bit broken. You might also want to check existing MPSAFE consumers in the kernel, if this API change makes any difference. I was thinking if user-space TCP might be affected by this. CC'ing Adrian. --HPS ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r286880 - head/sys/kern
Hi Hans,
On 26/08/15 10:12, Hans Petter Selasky wrote:
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.cTue Aug 18 10:07:03 2015
(r286879)
+++ head/sys/kern/kern_timeout.cTue 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
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 */
First thank for your time. I indeed agree with your analysis and I am
not opposed to back out this change. The border between bug or feature
can indeed be thin; below why I was more on bug side:
o Pro It is a feature:
- This behavior is here since the beginning and nobody ever complains
(Big one)
o Pro It is a bug:
- Having callout_stop() returning 1 (success) while having the callout
currently being serviced is counter intuitive.
- You cannot call callout_stop() twice to address this case:
callout_reset();
callout_stop(); /* cancel _new_ callout invocation */
callout_stop(); /* check for any pending callout invokation */
Because the second callout_stop() will always return 0 (Fail). In
details: If the callout is currently being serviced (without r286880
i.e. the classical behavior):
callout_reset() returns 0 (Fail)(PENDING flag set)
callout_stop() returns 1 (Success) (PENDING flag removed)
callout_stop() returns 0 (Always fail because not PENDING flag)
In mpsafe mode, the only way a callout_stop() can return 1 (Success) is
with the PENDING flags set. The way I found to address this case is with:
fail_reset = !callout_reset();
fail_stop = !callout_stop();
if (fail_reset || fail_stop) {
/* Callout not stopped */
}
This is what I did in r284245:
Re: svn commit: r286880 - head/sys/kern
Hi Hans, On 26/08/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.cTue Aug 18 10:07:03 2015(r286879) +++ head/sys/kern/kern_timeout.cTue 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 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; Hum, interesting let me double check that. -- Julien signature.asc Description: OpenPGP digital signature
Re: svn commit: r286880 - head/sys/kern
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.cTue Aug 18 10:07:03 2015(r286879)
+++ head/sys/kern/kern_timeout.cTue 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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r286880 - head/sys/kern
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.cTue Aug 18 10:07:03 2015
(r286879)
+++ head/sys/kern/kern_timeout.cTue 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
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
svn commit: r286880 - head/sys/kern
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.cTue Aug 18 10:07:03 2015 (r286879) +++ head/sys/kern/kern_timeout.cTue 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 ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r286880 - head/sys/kern
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.cTue Aug 18 10:07:03 2015 (r286879) +++ head/sys/kern/kern_timeout.cTue 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 Should this be MFC'ed to 10? --HPS ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
