From owner-svn-src-head@freebsd.org Fri Nov 13 20:59:07 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 A9FDBA2E8B1 for ; Fri, 13 Nov 2015 20:59:07 +0000 (UTC) (envelope-from rrs@netflix.com) Received: from mail-pa0-x22c.google.com (mail-pa0-x22c.google.com [IPv6:2607:f8b0:400e:c03::22c]) (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 6FFAE1218 for ; Fri, 13 Nov 2015 20:59:07 +0000 (UTC) (envelope-from rrs@netflix.com) Received: by pabfh17 with SMTP id fh17so110862638pab.0 for ; Fri, 13 Nov 2015 12:59:06 -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=vYlLOUw7K0zEBQ10q4TOdipphUfLwRdJSh59DaS4bvg=; b=K2XDTUeA+Po2o5Zv+bXdDFrbjJlpWnIe4lHQjZAZ3WsoCEqHQj7+wnYnODe+rq+0FC hAChif1t5wi6VzVv82WpuaLgdqWmf0CGXmT8g0yqp+A5RVUtSBdc8mIKvUtiCAww2Fbe VL2usbgqoLrnm999KQ1UQsDMUdN8qNLqagtV0= 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=vYlLOUw7K0zEBQ10q4TOdipphUfLwRdJSh59DaS4bvg=; b=Fn5J8jbnDCWZZ7m7XDoY7nZEEMAQBq6nKOGxBnoVltthrn7fX3q1ni02r58wo9SMOr +P6OrLr2+lSuKyQ9tOXab/ZF8l0By2H8c2CXM1+Kb63AsbvbXSu7L+kFTtPqqU9ozmsE O5th35vnV0sV4OYuKfVkQCRpOM7Pp8tCKqM6rNAc4s6/kBG8fwJ4igIZQ7+OQdXySWo6 IJ1izJH02ukSSsSssZBOSAmn8a8gPstvpQ8BXRi0W+L0XysEOMVChFdL7wxbxVc/iVcb EZICY27R/BoE2C3w1yOju+4/Kphfk68zO9tjz76UTRomYmQpbBLB84rh1CowDeILlSDl UVLA== X-Gm-Message-State: ALoCoQnMTLAVdeni0W+5yhxZ002C+m6aHgC6QsnNVJJMst67HdIpxzq3AfcSyUSln5OsMM699O+z X-Received: by 10.66.119.174 with SMTP id kv14mr35239492pab.143.1447448346712; Fri, 13 Nov 2015 12:59:06 -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 qk7sm22059504pbb.80.2015.11.13.12.59.03 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 13 Nov 2015 12:59:05 -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: <1660421447413365@web19h.yandex.ru> Date: Fri, 13 Nov 2015 15:59:07 -0500 Cc: "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" , Adrian Chadd , imp@freebsd.org Message-Id: References: null <201511101449.tAAEnXIi065747@repo.freebsd.org> <1660421447413365@web19h.yandex.ru> To: "Alexander V. Chernikov" X-Mailer: Apple Mail (2.1878.6) Content-Type: text/plain; charset=windows-1251 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 20:59:07 -0000 Strange I went looking through all calls to callout stop with cscope and saw no one paying attention to the return value=85 (which I thought was not = good). And yes I am running this in a lot of systems. R On Nov 13, 2015, at 6:16 AM, Alexander V. Chernikov = wrote: > 10.11.2015, 17:49, "Randall Stewart" : >> 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 >> 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_ */ -------- Randall Stewart rrs@netflix.com 803-317-4952