lkml.org 
[lkml]   [2006]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [take12 3/3] kevent: Timer notifications.
    On Mon, Aug 21, 2006 at 02:19:49PM +0400, Evgeniy Polyakov wrote:
    >
    >
    > Timer notifications.
    >
    > Timer notifications can be used for fine grained per-process time
    > management, since interval timers are very inconvenient to use,
    > and they are limited.

    Shouldn't this at leat use a hrtimer?

    > new file mode 100644
    > index 0000000..5217cd1
    > --- /dev/null
    > +++ b/kernel/kevent/kevent_timer.c
    > @@ -0,0 +1,107 @@
    > +/*
    > + * kevent_timer.c

    You still include those sill filename ontop of file comments..

    > +static struct lock_class_key kevent_timer_key;
    > +
    > +static int kevent_timer_enqueue(struct kevent *k)
    > +{
    > + int err;
    > + struct kevent_timer *t;
    > +
    > + t = kmalloc(sizeof(struct kevent_timer), GFP_KERNEL);
    > + if (!t)
    > + return -ENOMEM;
    > +
    > + setup_timer(&t->ktimer, &kevent_timer_func, (unsigned long)k);
    > +
    > + err = kevent_storage_init(&t->ktimer, &t->ktimer_storage);
    > + if (err)
    > + goto err_out_free;
    > + lockdep_set_class(&t->ktimer_storage.lock, &kevent_timer_key);

    When looking at the kevent_storage_init callers most need to do
    those lockdep_set_class class. Shouldn't kevent_storage_init just
    get a "struct lock_class_key *" argument?

    > +static int kevent_timer_callback(struct kevent *k)
    > +{
    > + k->event.ret_data[0] = (__u32)jiffies;

    This is returned to userspace, isn't it? raw jiffies should never be
    user-visible. Please convert this to an unit that actually makes sense
    for userspace (probably nanoseconds)

    > +static int __init kevent_init_timer(void)
    > +{
    > + struct kevent_callbacks tc = {
    > + .callback = &kevent_timer_callback,
    > + .enqueue = &kevent_timer_enqueue,
    > + .dequeue = &kevent_timer_dequeue};

    I think this should be static, and the normal style to write it would be:

    static struct kevent_callbacks tc = {
    .callback = kevent_timer_callback,
    .enqueue = kevent_timer_enqueue,
    .dequeue = kevent_timer_dequeue,
    };

    also please consider makring all the kevent_callbacks structs const
    to avoid false cacheline sharing and accidental modification, similar
    to what we did to various other operation vectors.

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-08-21 13:15    [W:0.024 / U:4.476 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site