lkml.org 
[lkml]   [2012]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 12/14] Closures
    From
    Date
    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_<FOO>_GUARDs
    along with CLOSURE_<FOO>

    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





    \
     
     \ /
      Last update: 2012-05-24 03:21    [W:0.042 / U:31.680 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site