From owner-svn-src-head@freebsd.org Fri Nov 13 11:16:21 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 6E898A2EF5A; Fri, 13 Nov 2015 11:16:21 +0000 (UTC) (envelope-from melifaro@ipfw.ru) Received: from forward12h.cmail.yandex.net (forward12h.cmail.yandex.net [IPv6:2a02:6b8:0:f35::9d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "forwards.mail.yandex.net", Issuer "Certum Level IV CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 07BFD1C33; Fri, 13 Nov 2015 11:16:20 +0000 (UTC) (envelope-from melifaro@ipfw.ru) Received: from web19h.yandex.ru (web19h.yandex.ru [IPv6:2a02:6b8:0:f05::29]) by forward12h.cmail.yandex.net (Yandex) with ESMTP id 3A38B20F72; Fri, 13 Nov 2015 14:16:07 +0300 (MSK) Received: from 127.0.0.1 (localhost [127.0.0.1]) by web19h.yandex.ru (Yandex) with ESMTP id EA44F2E00E1F; Fri, 13 Nov 2015 14:16:05 +0300 (MSK) Received: by web19h.yandex.ru with HTTP; Fri, 13 Nov 2015 14:16:05 +0300 From: Alexander V. Chernikov Envelope-From: melifaro@ipfw.ru To: Randall Stewart , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" , Adrian Chadd , imp@freebsd.org In-Reply-To: <201511101449.tAAEnXIi065747@repo.freebsd.org> References: null <201511101449.tAAEnXIi065747@repo.freebsd.org> Subject: Re: svn commit: r290664 - in head: share/man/man9 sys/kern sys/sys MIME-Version: 1.0 Message-Id: <1660421447413365@web19h.yandex.ru> X-Mailer: Yamail [ http://yandex.ru ] 5.0 Date: Fri, 13 Nov 2015 14:16:05 +0300 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=koi8-r 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 11:16:21 -0000 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 > > 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). 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.. > > ššSponsored by: Netflix Inc. > ššDifferential Revision: https://reviews.freebsd.org/D4076 > > Modified: > ššhead/share/man/man9/timeout.9 > ššhead/sys/kern/kern_timeout.c > ššhead/sys/sys/callout.h > > Modified: head/share/man/man9/timeout.9 > ============================================================================== > --- 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 > > Modified: head/sys/kern/kern_timeout.c > ============================================================================== > --- 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) > > š#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 > > šššššššššcc_exec_curr(cc, direct) = c; > šššššššššcc_exec_cancel(cc, direct) = false; > + cc_exec_drain(cc, direct) = NULL; > šššššššššCC_UNLOCK(cc); > šššššššššif (c_lock != NULL) { > šššššššššššššššššclass->lc_lock(c_lock, lock_status); > @@ -744,6 +747,15 @@ skip: > šššššššššCC_LOCK(cc); > šššššššššKASSERT(cc_exec_curr(cc, direct) == c, ("mishandled cc_curr")); > šššššššššcc_exec_curr(cc, direct) = NULL; > + if (cc_exec_drain(cc, direct)) { > + void (*drain)(void *); > + > + drain = cc_exec_drain(cc, direct); > + cc_exec_drain(cc, direct) = 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 > š} > > š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 &= ~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 &= ~CALLOUT_ACTIVE; > šššššššššššššššššif (cc_exec_curr(cc, direct) != 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); > ššššššššššššššššš} > > ššššššššššššššššš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 == NULL)) { > > ššššššššššššššššššššššššš/* > šššššššššššššššššššššššššš* 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) = 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) = 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) = drain; > + } > šššššššššššššššššCC_UNLOCK(cc); > šššššššššššššššššKASSERT(!sq_locked, ("sleepqueue chain still locked")); > šššššššššššššššššreturn (0); > > Modified: head/sys/sys/callout.h > ============================================================================== > --- 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 &= ~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 > > š#endif /* _SYS_CALLOUT_H_ */