From owner-svn-src-head@freebsd.org Fri Nov 13 22:45:01 2015 Return-Path: Delivered-To: svn-src-head@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 D434CA2E277 for ; Fri, 13 Nov 2015 22:45:01 +0000 (UTC) (envelope-from rrs@netflix.com) Received: from mail-pa0-x236.google.com (mail-pa0-x236.google.com [IPv6:2607:f8b0:400e:c03::236]) (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 991181C0D for ; Fri, 13 Nov 2015 22:45:01 +0000 (UTC) (envelope-from rrs@netflix.com) Received: by pacej9 with SMTP id ej9so6081342pac.2 for ; Fri, 13 Nov 2015 14:45:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netflix.com; s=google; h=content-type:mime-version:subject:from:in-reply-to:date:cc :message-id:references:to; bh=hRBB8GXiDTu4By9Bnhhcpd2VtzUTmJRskEYxkKwShFY=; b=T8af0Ms1ATsFZxBlafUhqbFZ5/RYkqzBJbuGqWg5oqyy3LDCl2L9sOWB6zjsJN6V65 PGcU8riQ6LpbioMcIwaYON/PNEqZXGu6O+2WXowCC5ER4HehCqDboV58e3DqP90YD4gk 6Txjb7OIx7zbmBCIDB7gjitjx4hvmVKcJz+pw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:content-type:mime-version:subject:from :in-reply-to:date:cc:message-id:references:to; bh=hRBB8GXiDTu4By9Bnhhcpd2VtzUTmJRskEYxkKwShFY=; b=aKcZrL8l9A6bYKZlPPKc7vjkhVGInudxsoOiBZe3NQ+bZKmz7IShvwpMLQ/QL/q2zb pEGT+J/jU9/oxOdQ3Pm+nltZkCDOXzJSV7cwuckuNAlwDmsev9Ww9nVexsEemzcen/cz rtAAs07KBQLU7WDcTm4/XK8zYOfp1Lt9ZGpofx2Z10dsDxPEwCtzghNji/iiQi/rsiak 5+/DvDUE1TBh7qYIYmKQM6MnuNzTgB1yefaZ0npv2oqUEyeO4ymo69aKfzq9WJPjZiku tJ3qZdwLQ60inIxKq4t3Lx9UQFURld/jsCVeat0GUEo+Zpnl1aJhuV1n53xW+AbyoWNx YcIA== X-Gm-Message-State: ALoCoQk4QkEWSycdj40CgmcYWjuWbLwd2T8Re83Oc9vilBB75Vtk3edMkLZmGs5ZaGUboEwsJZJH X-Received: by 10.66.155.226 with SMTP id vz2mr8180435pab.18.1447454701005; Fri, 13 Nov 2015 14:45:01 -0800 (PST) Received: from ip-192-168-70-10.us-west-2.compute.internal ([69.53.245.107]) by smtp.gmail.com with ESMTPSA id ea1sm22311044pbb.76.2015.11.13.14.44.55 (version=TLS1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 13 Nov 2015 14:44:59 -0800 (PST) Mime-Version: 1.0 (Mac OS X Mail 7.3 \(1878.6\)) Subject: Re: svn commit: r290664 - in head: share/man/man9 sys/kern sys/sys From: Randall Stewart In-Reply-To: <94281447451500@web13o.yandex.ru> Date: Fri, 13 Nov 2015 17:44:59 -0500 Cc: "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" , Adrian Chadd , "imp@freebsd.org" Message-Id: <320E56BB-64E8-41BF-BFF3-003F6610BC11@netflix.com> References: null <201511101449.tAAEnXIi065747@repo.freebsd.org> <1660421447413365@web19h.yandex.ru> <758BC83D-E327-4B08-9103-C31EC4DDB193@netflix.com> <81431447450321@web13o.yandex.ru> <19FCC2F9-0986-4106-9559-75E65184704C@netflix.com> <94281447451500@web13o.yandex.ru> To: "Alexander V. Chernikov" X-Mailer: Apple Mail (2.1878.6) Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.20 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Nov 2015 22:45:01 -0000 let me fix that R On Nov 13, 2015, at 4:51 PM, Alexander V. Chernikov = wrote: > 14.11.2015, 00:41, "Randall Stewart" : >> Hmm >> =20 >> callout_reset() returns either 0 or 1=20 >> =20 >> It returns no other values and did not change.. maybe ti should say = positive or one in the manual=85 >> =20 >> I can add that to the fix patch. > It would be great :) > Also (just nitpicking) "return values" section of callout(9) still = states that callout_stop() returns non-zero value if the callout is = pending. >> =20 >> =20 >> R >> On Nov 13, 2015, at 4:32 PM, Alexander V. Chernikov = wrote: >> =20 >>>=20 >>> One small note on lltable: change in nd6_llinfo_settimer_locked() = assumes that callout_reset() returns >0 for stopped callout. The man = page still says "non-zero value" for that case. >>> (And that was the reason of writing comment to D4076 on inconsistent = callout_reset() return value). >>> =20 >>> =20 >>> =20 >>> 14.11.2015, 00:19, "Randall Stewart" : >>>> So looking deeper something like the following (with Bryan=92s = patch) is in order. >>>> =20 >>>> Though there is one place in the task code that looks funny since = it was >>>> =20 >>>> pending =3D !!callout_stop() >>>> =20 >>>> I changed it to >>>> =20 >>>> pending =3D !! (callout_stop > 0) >>>> =20 >>>> But I wonder about the double !! that seems rather convoluted.. >>>> =20 >>>> Unless someone objects I will commit this shortly >>>> R >>>> =20 >>>> =20 >>>> =20 >>>> =20 >>>> , >>>> =20 >>>> On Nov 13, 2015, at 6:16 AM, Alexander V. Chernikov = wrote: >>>> =20 >>>>>=20 >>>>> 10.11.2015, 17:49, "Randall Stewart" : >>>>>>=20 >>>>>> Author: rrs >>>>>> Date: Tue Nov 10 14:49:32 2015 >>>>>> New Revision: 290664 >>>>>> URL: https://svnweb.freebsd.org/changeset/base/290664 >>>>>>=20 >>>>>> Log: >>>>>> Add new async_drain to the callout system. This is so-far not = used but >>>>>> should be used by TCP for sure in its cleanup of the IN-PCB = (will be coming shortly). >>>>>=20 >>>>> Randall, this commit introduced change in callout_stop() which was = not mentioned in commit message. >>>>> This change has broken lltable arp/nd handling: deleting interface = address causes immediate panic. >>>>> I also see other other code/subsystems relying on callout_stop() = return value (netgraph, pfsync, iscsi). >>>>> I was not able to find any discussion/analysis/testing for these = in D4076 so this change does not look like being properly tested prior = commiting.. >>>>>=20 >>>>>=20 >>>>> =20 >>>>>>=20 >>>>>> Sponsored by: Netflix Inc. >>>>>> Differential Revision: https://reviews.freebsd.org/D4076 >>>>>>=20 >>>>>> Modified: >>>>>> head/share/man/man9/timeout.9 >>>>>> head/sys/kern/kern_timeout.c >>>>>> head/sys/sys/callout.h >>>>>>=20 >>>>>> Modified: head/share/man/man9/timeout.9 >>>>>> = =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/share/man/man9/timeout.9 Tue Nov 10 14:14:41 2015 = (r290663) >>>>>> +++ head/share/man/man9/timeout.9 Tue Nov 10 14:49:32 2015 = (r290664) >>>>>> @@ -35,6 +35,7 @@ >>>>>> .Sh NAME >>>>>> .Nm callout_active , >>>>>> .Nm callout_deactivate , >>>>>> +.Nm callout_async_drain , >>>>>> .Nm callout_drain , >>>>>> .Nm callout_handle_init , >>>>>> .Nm callout_init , >>>>>> @@ -69,6 +70,8 @@ typedef void timeout_t (void *); >>>>>> .Ft void >>>>>> .Fn callout_deactivate "struct callout *c" >>>>>> .Ft int >>>>>> +.Fn callout_async_drain "struct callout *c" "timeout_t *drain" >>>>>> +.Ft int >>>>>> .Fn callout_drain "struct callout *c" >>>>>> .Ft void >>>>>> .Fn callout_handle_init "struct callout_handle *handle" >>>>>> @@ -236,17 +239,42 @@ The function >>>>>> cancels a callout >>>>>> .Fa c >>>>>> if it is currently pending. >>>>>> -If the callout is pending, then >>>>>> +If the callout is pending and successfuly stopped, then >>>>>> .Fn callout_stop >>>>>> -returns a non-zero value. >>>>>> -If the callout is not set, >>>>>> -has already been serviced, >>>>>> -or is currently being serviced, >>>>>> +returns a value of one. >>>>>> +If the callout is not set, or >>>>>> +has already been serviced, then >>>>>> +negative one is returned. >>>>>> +If the callout is currently being serviced and cannot be = stopped, >>>>>> then zero will be returned. >>>>>> If the callout has an associated lock, >>>>>> then that lock must be held when this function is called. >>>>>> .Pp >>>>>> The function >>>>>> +.Fn callout_async_drain >>>>>> +is identical to >>>>>> +.Fn callout_stop >>>>>> +with one difference. >>>>>> +When >>>>>> +.Fn callout_async_drain >>>>>> +returns zero it will arrange for the function >>>>>> +.Fa drain >>>>>> +to be called using the same argument given to the >>>>>> +.Fn callout_reset >>>>>> +function. >>>>>> +.Fn callout_async_drain >>>>>> +If the callout has an associated lock, >>>>>> +then that lock must be held when this function is called. >>>>>> +Note that when stopping multiple callouts that use the same lock = it is possible >>>>>> +to get multiple return's of zero and multiple calls to the >>>>>> +.Fa drain >>>>>> +function, depending upon which CPU's the callouts are running. = The >>>>>> +.Fa drain >>>>>> +function itself is called from the context of the completing = callout >>>>>> +i.e. softclock or hardclock, just like a callout itself. >>>>>> +p >>>>>> +.Pp >>>>>> +The function >>>>>> .Fn callout_drain >>>>>> is identical to >>>>>> .Fn callout_stop >>>>>>=20 >>>>>> 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 Nov 10 14:14:41 2015 = (r290663) >>>>>> +++ head/sys/kern/kern_timeout.c Tue Nov 10 14:49:32 2015 = (r290664) >>>>>> @@ -136,6 +136,7 @@ u_int callwheelsize, callwheelmask; >>>>>> */ >>>>>> struct cc_exec { >>>>>> struct callout *cc_curr; >>>>>> + void (*cc_drain)(void *); >>>>>> #ifdef SMP >>>>>> void (*ce_migration_func)(void *); >>>>>> void *ce_migration_arg; >>>>>> @@ -170,6 +171,7 @@ struct callout_cpu { >>>>>> #define callout_migrating(c) ((c)->c_iflags & = CALLOUT_DFRMIGRATION) >>>>>>=20 >>>>>> #define cc_exec_curr(cc, dir) cc->cc_exec_entity[dir].cc_curr >>>>>> +#define cc_exec_drain(cc, dir) cc->cc_exec_entity[dir].cc_drain >>>>>> #define cc_exec_next(cc) cc->cc_next >>>>>> #define cc_exec_cancel(cc, dir) = cc->cc_exec_entity[dir].cc_cancel >>>>>> #define cc_exec_waiting(cc, dir) = cc->cc_exec_entity[dir].cc_waiting >>>>>> @@ -679,6 +681,7 @@ softclock_call_cc(struct callout *c, str >>>>>>=20 >>>>>> cc_exec_curr(cc, direct) =3D c; >>>>>> cc_exec_cancel(cc, direct) =3D false; >>>>>> + cc_exec_drain(cc, direct) =3D NULL; >>>>>> CC_UNLOCK(cc); >>>>>> if (c_lock !=3D NULL) { >>>>>> class->lc_lock(c_lock, lock_status); >>>>>> @@ -744,6 +747,15 @@ skip: >>>>>> CC_LOCK(cc); >>>>>> KASSERT(cc_exec_curr(cc, direct) =3D=3D c, ("mishandled = cc_curr")); >>>>>> cc_exec_curr(cc, direct) =3D NULL; >>>>>> + if (cc_exec_drain(cc, direct)) { >>>>>> + void (*drain)(void *); >>>>>> + >>>>>> + drain =3D cc_exec_drain(cc, direct); >>>>>> + cc_exec_drain(cc, direct) =3D NULL; >>>>>> + CC_UNLOCK(cc); >>>>>> + drain(c_arg); >>>>>> + CC_LOCK(cc); >>>>>> + } >>>>>> if (cc_exec_waiting(cc, direct)) { >>>>>> /* >>>>>> * There is someone waiting for the >>>>>> @@ -1145,7 +1157,7 @@ callout_schedule(struct callout *c, int >>>>>> } >>>>>>=20 >>>>>> int >>>>>> -_callout_stop_safe(struct callout *c, int safe) >>>>>> +_callout_stop_safe(struct callout *c, int safe, void = (*drain)(void *)) >>>>>> { >>>>>> struct callout_cpu *cc, *old_cc; >>>>>> struct lock_class *class; >>>>>> @@ -1225,19 +1237,22 @@ again: >>>>>> * stop it by other means however. >>>>>> */ >>>>>> if (!(c->c_iflags & CALLOUT_PENDING)) { >>>>>> - c->c_flags &=3D ~CALLOUT_ACTIVE; >>>>>> - >>>>>> /* >>>>>> * If it wasn't on the queue and it isn't the = current >>>>>> * callout, then we can't stop it, so just bail. >>>>>> + * It probably has already been run (if locking >>>>>> + * is properly done). You could get here if the caller >>>>>> + * calls stop twice in a row for example. The second >>>>>> + * call would fall here without CALLOUT_ACTIVE set. >>>>>> */ >>>>>> + c->c_flags &=3D ~CALLOUT_ACTIVE; >>>>>> if (cc_exec_curr(cc, direct) !=3D c) { >>>>>> CTR3(KTR_CALLOUT, "failed to stop %p = func %p arg %p", >>>>>> c, c->c_func, c->c_arg); >>>>>> CC_UNLOCK(cc); >>>>>> if (sq_locked) >>>>>> = sleepq_release(&cc_exec_waiting(cc, direct)); >>>>>> - return (0); >>>>>> + return (-1); >>>>>> } >>>>>>=20 >>>>>> if (safe) { >>>>>> @@ -1298,14 +1313,16 @@ again: >>>>>> CC_LOCK(cc); >>>>>> } >>>>>> } else if (use_lock && >>>>>> - !cc_exec_cancel(cc, direct)) { >>>>>> + !cc_exec_cancel(cc, direct) && (drain =3D=3D NULL)) { >>>>>>=20 >>>>>> /* >>>>>> * The current callout is waiting for = its >>>>>> * lock which we hold. Cancel the = callout >>>>>> * and return. After our caller drops = the >>>>>> * lock, the callout will be skipped in >>>>>> - * softclock(). >>>>>> + * softclock(). This *only* works with a >>>>>> + * callout_stop() *not* callout_drain() or >>>>>> + * callout_async_drain(). >>>>>> */ >>>>>> cc_exec_cancel(cc, direct) =3D true; >>>>>> CTR3(KTR_CALLOUT, "cancelled %p func %p = arg %p", >>>>>> @@ -1351,11 +1368,17 @@ again: >>>>>> #endif >>>>>> CTR3(KTR_CALLOUT, "postponing stop %p = func %p arg %p", >>>>>> c, c->c_func, c->c_arg); >>>>>> + if (drain) { >>>>>> + cc_exec_drain(cc, direct) =3D drain; >>>>>> + } >>>>>> CC_UNLOCK(cc); >>>>>> return (0); >>>>>> } >>>>>> CTR3(KTR_CALLOUT, "failed to stop %p func %p arg = %p", >>>>>> c, c->c_func, c->c_arg); >>>>>> + if (drain) { >>>>>> + cc_exec_drain(cc, direct) =3D drain; >>>>>> + } >>>>>> CC_UNLOCK(cc); >>>>>> KASSERT(!sq_locked, ("sleepqueue chain still = locked")); >>>>>> return (0); >>>>>>=20 >>>>>> Modified: head/sys/sys/callout.h >>>>>> = =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/sys/callout.h Tue Nov 10 14:14:41 2015 (r290663) >>>>>> +++ head/sys/sys/callout.h Tue Nov 10 14:49:32 2015 (r290664) >>>>>> @@ -81,7 +81,7 @@ struct callout_handle { >>>>>> */ >>>>>> #define callout_active(c) ((c)->c_flags & CALLOUT_ACTIVE) >>>>>> #define callout_deactivate(c) ((c)->c_flags &=3D = ~CALLOUT_ACTIVE) >>>>>> -#define callout_drain(c) _callout_stop_safe(c, 1) >>>>>> +#define callout_drain(c) _callout_stop_safe(c, 1, NULL) >>>>>> void callout_init(struct callout *, int); >>>>>> void _callout_init_lock(struct callout *, struct lock_object *, = int); >>>>>> #define callout_init_mtx(c, mtx, flags) \ >>>>>> @@ -119,10 +119,11 @@ int callout_schedule(struct callout *, i >>>>>> int callout_schedule_on(struct callout *, int, int); >>>>>> #define callout_schedule_curcpu(c, on_tick) \ >>>>>> callout_schedule_on((c), (on_tick), PCPU_GET(cpuid)) >>>>>> -#define callout_stop(c) _callout_stop_safe(c, 0) >>>>>> -int _callout_stop_safe(struct callout *, int); >>>>>> +#define callout_stop(c) _callout_stop_safe(c, 0, NULL) >>>>>> +int _callout_stop_safe(struct callout *, int, void (*)(void *)); >>>>>> void callout_process(sbintime_t now); >>>>>> - >>>>>> +#define callout_async_drain(c, d) \ >>>>>> + _callout_stop_safe(c, 0, d) >>>>>> #endif >>>>>>=20 >>>>>> #endif /* _SYS_CALLOUT_H_ */ >>>> =20 >>>> -------- >>>> Randall Stewart >>>> rrs@netflix.com >>>> 803-317-4952 >>>> =20 >>>>=20 >>>> =20 >> =20 >> -------- >> Randall Stewart >> rrs@netflix.com >> 803-317-4952 >> =20 >>=20 >> =20 -------- Randall Stewart rrs@netflix.com 803-317-4952