svn commit: r287780 - in head: share/man/man9 sys/kern sys/sys

[Randall Stewart]

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’t think
you should be changing the nature of the callout and its lock.  

Overall I really doubt this will work well since the callout that you start will call to the user
function that says “I am done with the callout memory.. which is usually what you want this async-drain for” but
using the very memory that you want to free for the callout.

I wonder how tested this code is?  

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 “I am done”.. not this interesting recursive use
of the callout itself.

R
On Sep 14, 2015, at 3:52 AM, Hans Petter Selasky <hselasky at freebsd.org> wrote:

> Author: hselasky
> Date: Mon Sep 14 10:52:26 2015
> New Revision: 287780
> URL: https://svnweb.freebsd.org/changeset/base/287780
> 
> Log:
>  Implement callout_drain_async(), inspired by the projects/hps_head
>  branch.
> 
>  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.
> 
>  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.
> 
>  Differential Revision:	https://reviews.freebsd.org/D3521
>  Reviewed by:		wblock
>  MFC after:		1 month
> 
> 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
> 
> Modified: head/share/man/man9/Makefile
> ==============================================================================
> --- 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+=timeout.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 \
> 
> Modified: head/share/man/man9/timeout.9
> ==============================================================================
> --- 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
> 
> Modified: head/sys/kern/kern_timeout.c
> ==============================================================================
> --- 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 
> }
> 
> 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 = callout_lock(c);
> +	if (c->c_iflags & CALLOUT_DIRECT) {
> +		direct = 1;
> +	} else {
> +		direct = 0;
> +	}
> +	retval = (cc_exec_curr(cc, direct) == c);
> +
> +	/* drop locks, if any */
> +	if (retval && c->c_lock != NULL &&
> +	    c->c_lock != &Giant.lock_object) {
> +		/* ensure we are properly locked */
> +		class = LOCK_CLASS(c->c_lock);
> +		class->lc_assert(c->c_lock, LA_XLOCKED);
> +		/* the final callback should not be called locked */
> +		c->c_lock = NULL;
> +		c->c_iflags |= 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;
> 
> Modified: head/sys/sys/_callout.h
> ==============================================================================
> --- 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);
> 
> +typedef void callout_func_t(void *);
> +
> struct callout {
> 	union {
> 		LIST_ENTRY(callout) le;
> 
> Modified: head/sys/sys/callout.h
> ==============================================================================
> --- 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 &= ~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)					\
> 

--------
Randall Stewart
rrs at netflix.com
803-317-4952