From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zimbra.linbit.com (zimbra.linbit.com [212.69.161.123]) by mail09.linbit.com (LINBIT Mail Daemon) with ESMTP id 089A310003C9 for ; Thu, 24 May 2012 18:16:12 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by zimbra.linbit.com (Postfix) with ESMTP id EFAAD1B4262 for ; Thu, 24 May 2012 18:16:11 +0200 (CEST) Received: from zimbra.linbit.com ([127.0.0.1]) by localhost (zimbra.linbit.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qbwOCfsBEHKO for ; Thu, 24 May 2012 18:16:11 +0200 (CEST) Received: from soda.linbit (tuerlsteher.linbit.com [86.59.100.100]) by zimbra.linbit.com (Postfix) with ESMTP id D261C1B40FA for ; Thu, 24 May 2012 18:16:11 +0200 (CEST) Resent-Message-ID: <20120524161611.GW5734@soda.linbit> Received: from labridge.com (perches-mx.perches.com [206.117.179.246]) (using SSLv3 with cipher DES-CBC3-SHA (168/168 bits)) (No client certificate requested) by mail09.linbit.com (LINBIT Mail Daemon) with ESMTPS id 5D453106DCC0 for ; Thu, 24 May 2012 03:47:28 +0200 (CEST) Message-ID: <1337820443.1747.24.camel@joe2Laptop> From: Joe Perches To: Kent Overstreet Date: Wed, 23 May 2012 17:47:23 -0700 In-Reply-To: <1337817771-25038-13-git-send-email-koverstreet@google.com> References: <1337817771-25038-1-git-send-email-koverstreet@google.com> <1337817771-25038-13-git-send-email-koverstreet@google.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Cc: axboe@kernel.dk, yehuda@hq.newdream.net, dm-devel@redhat.com, neilb@suse.de, linux-kernel@vger.kernel.org, tj@kernel.org, linux-bcache@vger.kernel.org, mpatocka@redhat.com, vgoyal@redhat.com, bharrosh@panasas.com, linux-fsdevel@vger.kernel.org, sage@newdream.net, agk@redhat.com, drbd-dev@lists.linbit.com Subject: Re: [Drbd-dev] [PATCH v2 12/14] Closures List-Id: Coordination of development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Wed, 2012-05-23 at 17:02 -0700, Kent Overstreet wrote: > Asynchronous refcounty thingies; Hi again Kent. Highly descriptive word choice you've employed there. > they embed a refcount and a work > struct. Extensive documentation follows in include/linux/closure.h All trivia: > diff --git a/include/linux/closure.h b/include/linux/closure.h [] > +#define CLOSURE_REMAINING_MASK (~(~0 << 20)) More commonly used is ((1 << 20) - 1) > +#define CLOSURE_GUARD_MASK \ > + ((1 << 20)|(1 << 22)|(1 << 24)|(1 << 26)|(1 << 28)|(1 << 30)) Perhaps a poor choice of layout. Perhaps it'd be more sensible to define CLOSURE__GUARDs along with CLOSURE_ It might make more sense to use another macro with the somewhat magic number of 20 more explicitly defined. > + > + /* > + * CLOSURE_RUNNING: Set when a closure is running (i.e. by > + * closure_init() and when closure_put() runs then next function), and > + * must be cleared before remaining hits 0. Primarily to help guard > + * against incorrect usage and accidently transferring references. accidentally [] > + * CLOSURE_TIMER: Analagous to CLOSURE_WAITING, indicates that a closure analogous [] > +#define TYPE_closure 0U > +#define TYPE_closure_with_waitlist 1U > +#define TYPE_closure_with_timer 2U > +#define TYPE_closure_with_waitlist_and_timer 3U UPPER_lower is generally frowned on. It'd be better to use CLOSURE_TYPE as all uses are obscured by macros. > +#define MAX_CLOSURE_TYPE 3U > + unsigned type; > + > +#ifdef CONFIG_DEBUG_CLOSURES > +#define CLOSURE_MAGIC_DEAD 0xc054dead > +#define CLOSURE_MAGIC_ALIVE 0xc054a11e > + > + unsigned magic; > + struct list_head all; > + unsigned long ip; > + unsigned long waiting_on; > +#endif > +}; > + > +struct closure_with_waitlist { > + struct closure cl; > + closure_list_t wait; > +}; > + > +struct closure_with_timer { > + struct closure cl; > + struct timer_list timer; > +}; > + > +struct closure_with_waitlist_and_timer { > + struct closure cl; > + closure_list_t wait; > + struct timer_list timer; > +}; > + > +extern unsigned invalid_closure_type(void); > + > +#define __CL_TYPE(cl, _t) \ > + __builtin_types_compatible_p(typeof(cl), struct _t) \ > + ? TYPE_ ## _t : \ Might as well use __CLOSURE_TYPE > + > +#define __closure_type(cl) \ > +( \ > + __CL_TYPE(cl, closure) \ > + __CL_TYPE(cl, closure_with_waitlist) \ > + __CL_TYPE(cl, closure_with_timer) \ > + __CL_TYPE(cl, closure_with_waitlist_and_timer) \ > + invalid_closure_type() \ > +) outstandingly obscure. props. > diff --git a/lib/closure.c b/lib/closure.c [] > +#define CL_FIELD(type, field) \ > + case TYPE_ ## type: \ > + return &container_of(cl, struct type, cl)->field > + > +static closure_list_t *closure_waitlist(struct closure *cl) > +{ > + switch (cl->type) { > + CL_FIELD(closure_with_waitlist, wait); > + CL_FIELD(closure_with_waitlist_and_timer, wait); > + } > + return NULL; > +} > + > +static struct timer_list *closure_timer(struct closure *cl) > +{ > + switch (cl->type) { > + CL_FIELD(closure_with_timer, timer); > + CL_FIELD(closure_with_waitlist_and_timer, timer); > + } > + return NULL; > +} > + Another paragon of intelligibility. I think you should lose CL_FIELD and write normal switch/cases. [] > +static int debug_seq_show(struct seq_file *f, void *data) > +{ > + struct closure *cl; > + spin_lock_irq(&closure_list_lock); > + > + list_for_each_entry(cl, &closure_list, all) { > + int r = atomic_read(&cl->remaining); > + > + seq_printf(f, "%p: %pF -> %pf p %p r %i ", > + cl, (void *) cl->ip, cl->fn, cl->parent, > + r & CLOSURE_REMAINING_MASK); > + > + if (test_bit(WORK_STRUCT_PENDING, work_data_bits(&cl->work))) > + seq_printf(f, "Q"); seq_putc or seq_puts > + > + if (r & CLOSURE_RUNNING) > + seq_printf(f, "R"); > + > + if (r & CLOSURE_BLOCKING) > + seq_printf(f, "B"); > + > + if (r & CLOSURE_STACK) > + seq_printf(f, "S"); > + > + if (r & CLOSURE_SLEEPING) > + seq_printf(f, "Sl"); > + > + if (r & CLOSURE_TIMER) > + seq_printf(f, "T"); > + > + if (r & CLOSURE_WAITING) > + seq_printf(f, " W %pF\n", > + (void *) cl->waiting_on); > + > + seq_printf(f, "\n"); or maybe just bundle all this in a single seq_printf