From owner-svn-src-head@FreeBSD.ORG Sun Mar 24 17:35:20 2013 Return-Path: Delivered-To: svn-src-head@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 C33342E7 for ; Sun, 24 Mar 2013 17:35:20 +0000 (UTC) (envelope-from will@firepipe.net) Received: from mail-ie0-x22e.google.com (mail-ie0-x22e.google.com [IPv6:2607:f8b0:4001:c03::22e]) by mx1.freebsd.org (Postfix) with ESMTP id 8EB7A878 for ; Sun, 24 Mar 2013 17:35:20 +0000 (UTC) Received: by mail-ie0-f174.google.com with SMTP id aq17so3525122iec.19 for ; Sun, 24 Mar 2013 10:35:20 -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=cR2eJdUpItpMzpOw4PT/Sp56ZOb73hJ07SZ6lEmdCYg=; b=nXSQI8f3gAvwc1lW+I+HEMA05TVLJt1FFOEsh5y3YWaVhbThwAT5yaz3bRPH1moE0s K5Qnd7BKYhnW67mC3uJ0NffM5eVRvifruHYvpNPJs/SSVSFm8ZJ6AYx5YctTcwKnPUKt exJbtiSyvdVIUzh2zA8blKNR8tJtzdZ03MfxMWuhAk+48SGRA/VMDSavL/q0UVkpgbUW fje2qEJoZk5NZdHPR5j5tdQHZ0y7Xxc1+hOEsxGkcUje0DjR4OqezCnBO29BBGIOqKcC wS9YXmlNZDwVkV5mUd9XAeo4xezIdqyKSTsP7PHRJCZK5p+JABiblULPmZw6ShQf0YIH SQwA== MIME-Version: 1.0 X-Received: by 10.50.119.102 with SMTP id kt6mr6004314igb.12.1364146520123; Sun, 24 Mar 2013 10:35:20 -0700 (PDT) Received: by 10.231.247.74 with HTTP; Sun, 24 Mar 2013 10:35:20 -0700 (PDT) In-Reply-To: References: <201303231511.r2NFBrrr074289@svn.freebsd.org> Date: Sun, 24 Mar 2013 11:35:20 -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: ALoCoQmA80z/SKEwE/nM9OofL37/4/2a/NdcdVM2XMujHOvYkjZcYVYwJajScvjNRAWO8XDUsRTW 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-head@freebsd.org X-Mailman-Version: 2.1.14 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: Sun, 24 Mar 2013 17:35:20 -0000 BTW, the TQ_ prefix is only used in subr_taskqueue.c; it is not used in taskqueue.h. It could appear unrelated when viewed in taskqueue(9) consumers. There are also other very long names in taskqueue.h, so I'm not sure it's really worth renaming the callback types. Thanks, --Will. On Sat, Mar 23, 2013 at 12:36 PM, wrote: > On Sat, Mar 23, 2013 at 9:45 AM, Will Andrews wrote: > > 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? > > I'm not sure FreeBSD has anything standard, but at $WORK we use CB or > cb frequently as an abbreviation for callback. TASKQUEUE -> TQ still > removes 7 letters, which is a start. :-) > > Thanks, > matthew > > > > 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 > > > > >