From owner-svn-src-all@FreeBSD.ORG Sat Mar 23 16:45:30 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id EFDC421FE for ; Sat, 23 Mar 2013 16:45:30 +0000 (UTC) (envelope-from will@firepipe.net) Received: from mail-ia0-x234.google.com (mail-ia0-x234.google.com [IPv6:2607:f8b0:4001:c02::234]) by mx1.freebsd.org (Postfix) with ESMTP id C1E88800 for ; Sat, 23 Mar 2013 16:45:30 +0000 (UTC) Received: by mail-ia0-f180.google.com with SMTP id f27so4338064iae.25 for ; Sat, 23 Mar 2013 09:45:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type:x-gm-message-state; bh=luCZdXh3ldj3Dizu46SUxWDRbUIxl+G7u4O8uLHiAVU=; b=TdAlLJAH1j80ZjmVpODeUEVpMJTiZJkAXe2fiE0frQTt0OTLgVx0qtnrK8YWI7MBM4 KT3TKYs0Ufo7JSDUqyZoAWriLFHbfxcvSAt55epccHi1RPO1UtMuuiEKZfg6B8ykTX26 I+rUaVkxjGi3pZNp3cHoNlYha7XDTiGanQStf0JMpqbc1V+qDYiUlLKlKcQT0x2KVTA3 VG1Of3iPpTM4L1i1NE8mN0uOONO534UzP/XjS/6z4AbiXKFyYs+hmoNYSpgLDSq/Xrjm dwslIx9r7rjNeNfAZBGFxYMKcFCYY1VmpY/UT8Th1T0eFClL7qpvdsmBB5BDwy6zTDvR S+ww== MIME-Version: 1.0 X-Received: by 10.50.124.103 with SMTP id mh7mr288903igb.103.1364057130350; Sat, 23 Mar 2013 09:45:30 -0700 (PDT) Received: by 10.231.247.74 with HTTP; Sat, 23 Mar 2013 09:45:30 -0700 (PDT) In-Reply-To: References: <201303231511.r2NFBrrr074289@svn.freebsd.org> Date: Sat, 23 Mar 2013 10:45:30 -0600 Message-ID: Subject: Re: svn commit: r248649 - in head: share/man/man9 sys/kern sys/sys From: Will Andrews To: Matthew Fleming X-Gm-Message-State: ALoCoQl9/tAocIdH8ugp82f6EMwhPjO57+oUNy2fBbBkc/YA+ZYBMAh4ICHwynRJaNwyk1KNs+va Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.14 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Will Andrews X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 23 Mar 2013 16:45:31 -0000 I agree about the name length, it is a bit obnoxious. However, it is also descriptive. TQCB strikes me as perhaps a bit too far in the other direction. How about TQ_CALLBACK_? Is there an existing (pseudo) convention for callback names? Thanks, --Will. On Sat, Mar 23, 2013 at 10:20 AM, wrote: > On Sat, Mar 23, 2013 at 8:11 AM, Will Andrews wrote: > > Author: will > > Date: Sat Mar 23 15:11:53 2013 > > New Revision: 248649 > > URL: http://svnweb.freebsd.org/changeset/base/248649 > > > > Log: > > Extend taskqueue(9) to enable per-taskqueue callbacks. > > > > The scope of these callbacks is primarily to support actions that > affect the > > taskqueue's thread environments. They are entirely optional, and > > consequently are introduced as a new API: taskqueue_set_callback(). > > > > This interface allows the caller to specify that a taskqueue requires a > > callback and optional context pointer for a given callback type. > > > > The callback types included in this commit can be used to register a > > constructor and destructor for thread-local storage using osd(9). This > > allows a particular taskqueue to define that its threads require a > specific > > type of TLS, without the need for a specially-orchestrated task-based > > mechanism for startup and shutdown in order to accomplish it. > > > > Two callback types are supported at this point: > > > > - TASKQUEUE_CALLBACK_TYPE_INIT, called by every thread when it starts, > prior > > to processing any tasks. > > - TASKQUEUE_CALLBACK_TYPE_SHUTDOWN, called by every thread when it > exits, > > after it has processed its last task but before the taskqueue is > > reclaimed. > > > > While I'm here: > > > > - Add two new macros, TQ_ASSERT_LOCKED and TQ_ASSERT_UNLOCKED, and use > them > > in appropriate locations. > > - Fix taskqueue.9 to mention taskqueue_start_threads(), which is a > required > > interface for all consumers of taskqueue(9). > > > > Reviewed by: kib (all), eadler (taskqueue.9), brd (taskqueue.9) > > Approved by: ken (mentor) > > Sponsored by: Spectra Logic > > MFC after: 1 month > > > > Modified: > > head/share/man/man9/taskqueue.9 > > head/sys/kern/subr_taskqueue.c > > head/sys/sys/taskqueue.h > > > > Modified: head/share/man/man9/taskqueue.9 > > > ============================================================================== > > --- head/share/man/man9/taskqueue.9 Sat Mar 23 14:52:31 2013 > (r248648) > > +++ head/share/man/man9/taskqueue.9 Sat Mar 23 15:11:53 2013 > (r248649) > > @@ -53,12 +53,23 @@ struct task { > > void *ta_context; /* argument for handler > */ > > }; > > > > +enum taskqueue_callback_type { > > + TASKQUEUE_CALLBACK_TYPE_INIT, > > + TASKQUEUE_CALLBACK_TYPE_SHUTDOWN, > > +}; > > + > > +typedef void (*taskqueue_callback_fn)(void *context); > > + > > struct timeout_task; > > .Ed > > .Ft struct taskqueue * > > .Fn taskqueue_create "const char *name" "int mflags" > "taskqueue_enqueue_fn enqueue" "void *context" > > .Ft struct taskqueue * > > .Fn taskqueue_create_fast "const char *name" "int mflags" > "taskqueue_enqueue_fn enqueue" "void *context" > > +.Ft int > > +.Fn taskqueue_start_threads "struct taskqueue **tqp" "int count" "int > pri" "const char *name" "..." > > +.Ft void > > +.Fn taskqueue_set_callback "struct taskqueue *queue" "enum > taskqueue_callback_type cb_type" "taskqueue_callback_fn callback" "void > *context" > > .Ft void > > .Fn taskqueue_free "struct taskqueue *queue" > > .Ft int > > @@ -127,6 +138,23 @@ should be used to free the memory used b > > Any tasks that are on the queue will be executed at this time after > > which the thread servicing the queue will be signaled that it should > exit. > > .Pp > > +Once a taskqueue has been created, its threads should be started using > > +.Fn taskqueue_start_threads . > > +Callbacks may optionally be registered using > > +.Fn taskqueue_set_callback . > > +Currently, callbacks may be registered for the following purposes: > > +.Bl -tag -width TASKQUEUE_CALLBACK_TYPE_SHUTDOWN > > +.It Dv TASKQUEUE_CALLBACK_TYPE_INIT > > +This callback is called by every thread in the taskqueue, before it > executes > > +any tasks. > > +This callback must be set before the taskqueue's threads are started. > > +.It Dv TASKQUEUE_CALLBACK_TYPE_SHUTDOWN > > +This callback is called by every thread in the taskqueue, after it > executes > > +its last task. > > +This callback will always be called before the taskqueue structure is > > +reclaimed. > > +.El > > +.Pp > > To add a task to the list of tasks queued on a taskqueue, call > > .Fn taskqueue_enqueue > > with pointers to the queue and task. > > > > Modified: head/sys/kern/subr_taskqueue.c > > > ============================================================================== > > --- head/sys/kern/subr_taskqueue.c Sat Mar 23 14:52:31 2013 > (r248648) > > +++ head/sys/kern/subr_taskqueue.c Sat Mar 23 15:11:53 2013 > (r248649) > > @@ -63,6 +63,8 @@ struct taskqueue { > > int tq_spin; > > int tq_flags; > > int tq_callouts; > > + taskqueue_callback_fn tq_callbacks[TASKQUEUE_NUM_CALLBACKS]; > > + void *tq_cb_contexts[TASKQUEUE_NUM_CALLBACKS]; > > }; > > > > #define TQ_FLAGS_ACTIVE (1 << 0) > > @@ -78,6 +80,7 @@ struct taskqueue { > > else \ > > mtx_lock(&(tq)->tq_mutex); \ > > } while (0) > > +#define TQ_ASSERT_LOCKED(tq) mtx_assert(&(tq)->tq_mutex, > MA_OWNED) > > > > #define TQ_UNLOCK(tq) > \ > > do { \ > > @@ -86,6 +89,7 @@ struct taskqueue { > > else \ > > mtx_unlock(&(tq)->tq_mutex); \ > > } while (0) > > +#define TQ_ASSERT_UNLOCKED(tq) mtx_assert(&(tq)->tq_mutex, > MA_NOTOWNED) > > > > void > > _timeout_task_init(struct taskqueue *queue, struct timeout_task > *timeout_task, > > @@ -137,6 +141,23 @@ taskqueue_create(const char *name, int m > > MTX_DEF, "taskqueue"); > > } > > > > +void > > +taskqueue_set_callback(struct taskqueue *queue, > > + enum taskqueue_callback_type cb_type, taskqueue_callback_fn > callback, > > + void *context) > > +{ > > + > > + KASSERT(((cb_type >= TASKQUEUE_CALLBACK_TYPE_MIN) && > > + (cb_type <= TASKQUEUE_CALLBACK_TYPE_MAX)), > > + ("Callback type %d not valid, must be %d-%d", cb_type, > > + TASKQUEUE_CALLBACK_TYPE_MIN, TASKQUEUE_CALLBACK_TYPE_MAX)); > > + KASSERT((queue->tq_callbacks[cb_type] == NULL), > > + ("Re-initialization of taskqueue callback?")); > > + > > + queue->tq_callbacks[cb_type] = callback; > > + queue->tq_cb_contexts[cb_type] = context; > > +} > > + > > /* > > * Signal a taskqueue thread to terminate. > > */ > > @@ -293,7 +314,7 @@ taskqueue_run_locked(struct taskqueue *q > > struct task *task; > > int pending; > > > > - mtx_assert(&queue->tq_mutex, MA_OWNED); > > + TQ_ASSERT_LOCKED(queue); > > tb.tb_running = NULL; > > TAILQ_INSERT_TAIL(&queue->tq_active, &tb, tb_link); > > > > @@ -332,7 +353,7 @@ task_is_running(struct taskqueue *queue, > > { > > struct taskqueue_busy *tb; > > > > - mtx_assert(&queue->tq_mutex, MA_OWNED); > > + TQ_ASSERT_LOCKED(queue); > > TAILQ_FOREACH(tb, &queue->tq_active, tb_link) { > > if (tb->tb_running == task) > > return (1); > > @@ -489,6 +510,18 @@ taskqueue_start_threads(struct taskqueue > > return (0); > > } > > > > +static inline void > > +taskqueue_run_callback(struct taskqueue *tq, > > + enum taskqueue_callback_type cb_type) > > +{ > > + taskqueue_callback_fn tq_callback; > > + > > + TQ_ASSERT_UNLOCKED(tq); > > + tq_callback = tq->tq_callbacks[cb_type]; > > + if (tq_callback != NULL) > > + tq_callback(tq->tq_cb_contexts[cb_type]); > > +} > > + > > void > > taskqueue_thread_loop(void *arg) > > { > > @@ -496,6 +529,7 @@ taskqueue_thread_loop(void *arg) > > > > tqp = arg; > > tq = *tqp; > > + taskqueue_run_callback(tq, TASKQUEUE_CALLBACK_TYPE_INIT); > > TQ_LOCK(tq); > > while ((tq->tq_flags & TQ_FLAGS_ACTIVE) != 0) { > > taskqueue_run_locked(tq); > > @@ -510,6 +544,15 @@ taskqueue_thread_loop(void *arg) > > } > > taskqueue_run_locked(tq); > > > > + /* > > + * This thread is on its way out, so just drop the lock > temporarily > > + * in order to call the shutdown callback. This allows the > callback > > + * to look at the taskqueue, even just before it dies. > > + */ > > + TQ_UNLOCK(tq); > > + taskqueue_run_callback(tq, TASKQUEUE_CALLBACK_TYPE_SHUTDOWN); > > + TQ_LOCK(tq); > > + > > /* rendezvous with thread that asked us to terminate */ > > tq->tq_tcount--; > > wakeup_one(tq->tq_threads); > > @@ -525,7 +568,7 @@ taskqueue_thread_enqueue(void *context) > > tqp = context; > > tq = *tqp; > > > > - mtx_assert(&tq->tq_mutex, MA_OWNED); > > + TQ_ASSERT_LOCKED(tq); > > wakeup_one(tq); > > } > > > > > > Modified: head/sys/sys/taskqueue.h > > > ============================================================================== > > --- head/sys/sys/taskqueue.h Sat Mar 23 14:52:31 2013 (r248648) > > +++ head/sys/sys/taskqueue.h Sat Mar 23 15:11:53 2013 (r248649) > > @@ -47,6 +47,16 @@ struct timeout_task { > > int f; > > }; > > > > +enum taskqueue_callback_type { > > + TASKQUEUE_CALLBACK_TYPE_INIT, > > + TASKQUEUE_CALLBACK_TYPE_SHUTDOWN, > > +}; > > +#define TASKQUEUE_CALLBACK_TYPE_MIN > TASKQUEUE_CALLBACK_TYPE_INIT > > +#define TASKQUEUE_CALLBACK_TYPE_MAX > TASKQUEUE_CALLBACK_TYPE_SHUTDOWN > > +#define TASKQUEUE_NUM_CALLBACKS > TASKQUEUE_CALLBACK_TYPE_MAX + 1 > > This need parentheses to defensively guard against unexpected > precedence of operators. > > While here, TASKQUEUE_CALLBACK_ is a very long prefix (19 characters!) > Can this be named TQCB_ instead so it's not so verbose? > > Thanks, > matthew >