Date: Wed, 16 Sep 2015 13:54:47 -0700 From: Randall Stewart <rrs@netflix.com> To: Hans Petter Selasky <hselasky@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r287780 - in head: share/man/man9 sys/kern sys/sys Message-ID: <C6E2C380-AF00-4CAD-BBD4-8F73BCEB89A1@netflix.com> In-Reply-To: <201509141052.t8EAqRWf008293@repo.freebsd.org> References: <201509141052.t8EAqRWf008293@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hans: By outside prompting, I have finally had a chance to look at this: What it appears you do here is: a) stop the callout, not paying attention to if it stopped or not. b) Then get the callout systems lock and check if your callout is up and = running, storing that in your retval. Where 1 is it was running, and 0 is no it was not running. c) Assuming that your callout is running. 1) assert the user has the lock (if its a mtx associated lock), which = means the callout is blocked on you finishing this. 2) change the nature of the callout so that it will return-unlocked = (no matter what the caller thought he set in his callout). 3) Start a timeout using this same callout that calls the function = (async drain function) that the user supplied *instead* of the original callout. Now this is *not* how I would have done it and I question c.2 = especially. I don=92t think you should be changing the nature of the callout and its lock. =20 Overall I really doubt this will work well since the callout that you = start will call to the user function that says =93I am done with the callout memory.. which is = usually what you want this async-drain for=94 but using the very memory that you want to free for the callout. I wonder how tested this code is? =20 I would think one would be better off having a way to set a callback if = you are trying to stop and drain-async that the actual callout code itself calls to say =93I am done=94.. not = this interesting recursive use of the callout itself. R On Sep 14, 2015, at 3:52 AM, Hans Petter Selasky <hselasky@freebsd.org> = wrote: > Author: hselasky > Date: Mon Sep 14 10:52:26 2015 > New Revision: 287780 > URL: https://svnweb.freebsd.org/changeset/base/287780 >=20 > Log: > Implement callout_drain_async(), inspired by the projects/hps_head > branch. >=20 > This function is used to drain a callout via a callback instead of > blocking the caller until the drain is complete. Refer to the > callout_drain_async() manual page for a detailed description. >=20 > Limitation: If a lock is used with the callout, the callout can only > be drained asynchronously one time unless the callout_init_mtx() > function is called again. This limitation is not present in > projects/hps_head and will require more invasive changes to the > timeout code, which was not in the scope of this patch. >=20 > Differential Revision: https://reviews.freebsd.org/D3521 > Reviewed by: wblock > MFC after: 1 month >=20 > Modified: > head/share/man/man9/Makefile > head/share/man/man9/timeout.9 > head/sys/kern/kern_timeout.c > head/sys/sys/_callout.h > head/sys/sys/callout.h >=20 > Modified: head/share/man/man9/Makefile > = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=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/Makefile Mon Sep 14 10:28:47 2015 = (r287779) > +++ head/share/man/man9/Makefile Mon Sep 14 10:52:26 2015 = (r287780) > @@ -1641,6 +1641,7 @@ MLINKS+=3Dtimeout.9 callout.9 \ > timeout.9 callout_active.9 \ > timeout.9 callout_deactivate.9 \ > timeout.9 callout_drain.9 \ > + timeout.9 callout_drain_async.9 \ > timeout.9 callout_handle_init.9 \ > timeout.9 callout_init.9 \ > timeout.9 callout_init_mtx.9 \ >=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 Mon Sep 14 10:28:47 2015 = (r287779) > +++ head/share/man/man9/timeout.9 Mon Sep 14 10:52:26 2015 = (r287780) > @@ -36,6 +36,7 @@ > .Nm callout_active , > .Nm callout_deactivate , > .Nm callout_drain , > +.Nm callout_drain_async , > .Nm callout_handle_init , > .Nm callout_init , > .Nm callout_init_mtx , > @@ -70,6 +71,8 @@ typedef void timeout_t (void *); > .Fn callout_deactivate "struct callout *c" > .Ft int > .Fn callout_drain "struct callout *c" > +.Ft int > +.Fn callout_drain_async "struct callout *c" "callout_func_t *fn" = "void *arg" > .Ft void > .Fn callout_handle_init "struct callout_handle *handle" > .Bd -literal > @@ -264,6 +267,24 @@ fully stopped before > .Fn callout_drain > returns. > .Pp > +The function > +.Fn callout_drain_async > +is non-blocking and works the same as the > +.Fn callout_stop > +function. > +When this function returns non-zero, do not call it again until the = callback function given by > +.Fa fn > +has been called with argument > +.Fa arg . > +Only one of > +.Fn callout_drain > +or > +.Fn callout_drain_async > +should be called at a time to drain a callout. > +If this function returns zero, it is safe to free the callout = structure pointed to by the > +.Fa c > +argument immediately. > +.Pp > The > .Fn callout_reset > and >=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 Mon Sep 14 10:28:47 2015 = (r287779) > +++ head/sys/kern/kern_timeout.c Mon Sep 14 10:52:26 2015 = (r287780) > @@ -1145,6 +1145,45 @@ callout_schedule(struct callout *c, int=20 > } >=20 > int > +callout_drain_async(struct callout *c, callout_func_t *func, void = *arg) > +{ > + struct callout_cpu *cc; > + struct lock_class *class; > + int retval; > + int direct; > + > + /* stop callout */ > + callout_stop(c); > + > + /* check if callback is being called */ > + cc =3D callout_lock(c); > + if (c->c_iflags & CALLOUT_DIRECT) { > + direct =3D 1; > + } else { > + direct =3D 0; > + } > + retval =3D (cc_exec_curr(cc, direct) =3D=3D c); > + > + /* drop locks, if any */ > + if (retval && c->c_lock !=3D NULL && > + c->c_lock !=3D &Giant.lock_object) { > + /* ensure we are properly locked */ > + class =3D LOCK_CLASS(c->c_lock); > + class->lc_assert(c->c_lock, LA_XLOCKED); > + /* the final callback should not be called locked */ > + c->c_lock =3D NULL; > + c->c_iflags |=3D CALLOUT_RETURNUNLOCKED; > + } > + CC_UNLOCK(cc); > + > + /* check if we should queue final callback */ > + if (retval) > + callout_reset(c, 1, func, arg); > + > + return (retval); > +} > + > +int > _callout_stop_safe(struct callout *c, int safe) > { > struct callout_cpu *cc, *old_cc; >=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 Mon Sep 14 10:28:47 2015 = (r287779) > +++ head/sys/sys/_callout.h Mon Sep 14 10:52:26 2015 = (r287780) > @@ -46,6 +46,8 @@ LIST_HEAD(callout_list, callout); > SLIST_HEAD(callout_slist, callout); > TAILQ_HEAD(callout_tailq, callout); >=20 > +typedef void callout_func_t(void *); > + > struct callout { > union { > LIST_ENTRY(callout) le; >=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 Mon Sep 14 10:28:47 2015 = (r287779) > +++ head/sys/sys/callout.h Mon Sep 14 10:52:26 2015 = (r287780) > @@ -82,6 +82,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) > +int callout_drain_async(struct callout *, callout_func_t *, void *); > void callout_init(struct callout *, int); > void _callout_init_lock(struct callout *, struct lock_object *, int); > #define callout_init_mtx(c, mtx, flags) = \ >=20 -------- Randall Stewart rrs@netflix.com 803-317-4952
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?C6E2C380-AF00-4CAD-BBD4-8F73BCEB89A1>