From owner-svn-src-all@freebsd.org Wed Aug 26 18:14:26 2015 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 5C8639C276A for ; Wed, 26 Aug 2015 18:14:26 +0000 (UTC) (envelope-from julien@jch.io) Received: from mail-wi0-f175.google.com (mail-wi0-f175.google.com [209.85.212.175]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id E8AC99D for ; Wed, 26 Aug 2015 18:14:25 +0000 (UTC) (envelope-from julien@jch.io) Received: by wicne3 with SMTP id ne3so52705862wic.0 for ; Wed, 26 Aug 2015 11:14:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-type; bh=3M9wN0G1mHbEmAr6jIOJEqlFUkfeHTzKK64zPat+QeA=; b=R08lYMOIgPIG/uxIWIGamvVCOIy1qXS1PZrBKZ1jjh2bE9Wqay/Zd+maA4iyb0EdCL 6WTdSR43a12WidPckmxOmZ5WAbPVv/0aVh92NqGhbX1QCMKavhuTUOZ2QCTHBhRcyn6s JTo+sCyFGHNcsKdVWcUs7nbiIQoushyYGLVvJhKXWGEYjgA827w3VHjOIY8TQsiD8C9c QZWL1vHQvXudbfWzAIjOQ01hoYwjMvP7Sy+QAZmtCnEGBYH8WEvqkLBDXnPgM0lFJi9f 3b6q3lGuFp3yStX9Mmym6CB9ZPYrXeGYJAgzhjqoFsJPvIzmFYZHt8R4dd7CAbFNiYtP uElw== X-Gm-Message-State: ALoCoQl1jiJNrjT/N2im6tszEiY2vcf/lG68dpyj9anxhQZ/ZBfCyJ2/4OYdxuO9cETy7wBOi77k X-Received: by 10.194.93.166 with SMTP id cv6mr61239741wjb.63.1440612863765; Wed, 26 Aug 2015 11:14:23 -0700 (PDT) Received: from fri2pmaresca-l1.vcorp.ad.vrsn.com ([217.30.88.7]) by smtp.googlemail.com with ESMTPSA id gv1sm8769765wib.15.2015.08.26.11.14.22 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 26 Aug 2015 11:14:23 -0700 (PDT) Subject: Re: svn commit: r286880 - head/sys/kern To: Hans Petter Selasky , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, John Baldwin References: <201508181015.t7IAFAex055889@repo.freebsd.org> <55DD69E5.4090904@selasky.org> <55DD74EB.30601@selasky.org> From: Julien Charbon X-Enigmail-Draft-Status: N1110 Message-ID: <55DE01F7.8040508@freebsd.org> Date: Wed, 26 Aug 2015 20:14:15 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <55DD74EB.30601@selasky.org> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="TItrp25ijAVjbKfgsMecOF3872ljK8LVM" X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Aug 2015 18:14:26 -0000 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--