Date: Wed, 26 Aug 2015 20:14:15 +0200 From: Julien Charbon <jch@freebsd.org> To: Hans Petter Selasky <hp@selasky.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, John Baldwin <jhb@freebsd.org> Subject: Re: svn commit: r286880 - head/sys/kern Message-ID: <55DE01F7.8040508@freebsd.org> In-Reply-To: <55DD74EB.30601@selasky.org> References: <201508181015.t7IAFAex055889@repo.freebsd.org> <55DD69E5.4090904@selasky.org> <55DD74EB.30601@selasky.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --TItrp25ijAVjbKfgsMecOF3872ljK8LVM Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 currentl= y >>> 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 >>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D >>> >>> >>> --- head/sys/kern/kern_timeout.c Tue Aug 18 10:07:03 2015 =20 >>> (r286879) >>> +++ head/sys/kern/kern_timeout.c Tue Aug 18 10:15:09 2015 =20 >>> (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 progr= ess >>> + * and indeed impossible to stop then return 0. >>> + */ >>> + not_running =3D !(cc_exec_curr(cc, direct) =3D=3D c); >>> + >>> CC_UNLOCK(cc); >>> - return (1); >>> + return (not_running); >>> } >>> >>> void >=20 > 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 D156= 3 > and also r284245. >=20 > This patch introduces new behaviour to the callout_stop() function, > which is contradicting "man 9 callout" and _not_ documented anywhere! >=20 > Reading "man 9 callout" in 11-current: >=20 >> 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. >=20 > callout_reset() causes a _new_ callout invocation and it is logical tha= t > the return value of callout_stop() reflect operations on the _new_ > callout invocation and not the previous one. >=20 >> 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 i= s >> called. >=20 > If there are two callout invocations that callout_stop() needs to > handle, it should be called twice. >=20 > The correct way is: >=20 > callout_reset(); > callout_stop(); > callout_drain(); >=20 > For the TCP stack's matter, it should be: >=20 > 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 =3D !callout_reset(); fail_stop =3D !callout_stop(); if (fail_reset || fail_stop) { /* Callout not stopped */ } This is what I did in r284245: https://svnweb.freebsd.org/base/head/sys/netinet/tcp_timer.c?r1=3D284245&= r2=3D284244&pathrev=3D284245 And it felt wrong to me because callout_reset() and callout_stop() calls can be far away. o Pro "it is 'neither'" (i.e. API unclear by design): In this case the same callout is indeed _both_ pending and currently being serviced. According to man page callout(9): (As you already notic= ed) If the callout is pending, then callout_stop() returns a non-zero ^^^^^^^^^^^^^^^^^^ value. Conclusion: It is a feature! And just after: If the callout is not set, has already been serviced, or is currently being serviced, then zero will be returned. ^^^^^^^^^^^^^^^^^^^^^^^^ Conclusion: It is a bug! Obviously, no indication about the case where the callout is indeed _both_ pending and currently being serviced. > Remember there are other OS'es out there using our TCP/IP stack which > do not have an identical callout subsystem. I was not aware of that. 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. -- Julien --TItrp25ijAVjbKfgsMecOF3872ljK8LVM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Comment: GPGTools - https://gpgtools.org iQEcBAEBCgAGBQJV3gH9AAoJEKVlQ5Je6dhxEGsIAOargp57PbbzfYCq9JoawFSR 4Pdzxan0KQNibvxIRoy6g3taTh4+g8AsXrJkdCuafsXJ31vnHjWRje03CpMHx6O1 ynjwF8h1QgWvnbRqzfHP0dWVAVAbOtSAROEutGzHWc3T7a0lLbPpznOkh81w6OMH 7BLY/laGZXKU55UsHnRBxoWS2/FRwJdjVuBaJ4Y4n0fXmtLPrtDLC4BVGIfMI9Zz h8XTNipNLjx/7NEDAGvLQ1IgHcUpgBMhPrJ5MnxcYxdJONZZb02QWnHODsWgm5/8 RHsO9lFyrnxjZJsBtRxjyQdH3FqGS5+LQVOkiI/JnF9gB9xgnI42uS0gk6+f6b8= =uzsn -----END PGP SIGNATURE----- --TItrp25ijAVjbKfgsMecOF3872ljK8LVM--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?55DE01F7.8040508>