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>