lkml.org 
[lkml]   [2007]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] Add not_critical_when_idle timer
    On Fri, 16 Mar 2007 15:07:35 -0700 Venkatesh Pallipadi <venkatesh.pallipadi@intel.com> wrote:

    >
    > Introduce a new kind of timers - not_critical_when_idle timers:
    > Timers that work normally when system is busy. But, will not cause CPU to
    > come out of idle (just to service this timer), when CPU is idle. Instead,
    > this timer will be serviced when CPU eventually wakes up with a subsequent
    > critical_when_idle timer.
    >
    > The main advantage of this is to avoid unnecessary timer interrupts when
    > CPU is idle. If the routine currently called by a timer can wait until next
    > event without any issues, this new timer can be used to setup timer event
    > for that routine. This, with dynticks, allows CPUs to be lazy, allowing them
    > to stay in idle for extended period of time by reducing unnecesary wakeup and
    > thereby reducing the power consumption.
    >
    > This patch:
    > Builds this new timer on top of existing timer infrastructure. It uses
    > last bit in 'base' pointer of timer_list structure to store this
    > extra information about timer. __next_timer_interrupt() function
    > skips over these not_critical_when_idle timers when CPU looks for
    > next timer event for which it has to wake up.

    Fair enough, I guess.

    > This is exported by a new interface add_timer_with_hint() and also a new
    > parameter is added to existing add_timer_on() interface.
    >

    A few cosmetic and interface things:

    > --- linux-2.6.20.orig/kernel/timer.c 2007-03-16 14:13:19.000000000 -0700
    > +++ linux-2.6.20/kernel/timer.c 2007-03-16 14:51:15.000000000 -0700
    > @@ -74,7 +74,7 @@
    > tvec_t tv3;
    > tvec_t tv4;
    > tvec_t tv5;
    > -} ____cacheline_aligned_in_smp;
    > +} ____cacheline_aligned;

    hm, that's an unrelated bugfix.

    > ...
    >
    > +extern struct tvec_t_base_s boot_tvec_bases;
    > +/*
    > + * Note that all tvec_bases is 2 byte aligned and lower bit of
    > + * base in timer_list is guaranteed to be zero. Use the LSB for
    > + * the new flag to indicate whether it is OK to skip timer callback
    > + * when CPU is idle.
    > + */
    > +#define TBASE_FLAG_DELAYED_ON_IDLE (0x1)
    > +
    > +#define TBASE_GET_BASE_PTR(x) \
    > + ((struct tvec_t_base_s *)((unsigned long)x & \
    > + (~TBASE_FLAG_DELAYED_ON_IDLE)))
    > +
    > +#define TBASE_GET_DELAYED_ON_IDLE(x) \
    > + ((unsigned long)x & TBASE_FLAG_DELAYED_ON_IDLE)
    > +
    > +#define TBASE_SET_DELAYED_ON_IDLE(x) \
    > + ((struct tvec_t_base_s *)((unsigned long)x | \
    > + TBASE_FLAG_DELAYED_ON_IDLE))
    > +
    > +#define TBASE_CLEAR_DELAYED_ON_IDLE(x) \
    > + ((struct tvec_t_base_s *)((unsigned long)x & \
    > + (~TBASE_FLAG_DELAYED_ON_IDLE)))
    > +
    > +#define TBASE_MERGE_DELAYED_ON_IDLE(x,f) \
    > + ((f) ? TBASE_SET_DELAYED_ON_IDLE(x) : TBASE_CLEAR_DELAYED_ON_IDLE(x))

    Can we implement these as lower-case-named inline functions? I don't think
    there's any reason why they have to be macros?

    > struct timer_list {
    > struct list_head entry;
    > unsigned long expires;
    > @@ -23,7 +50,6 @@
    > #endif
    > };
    >
    > -extern struct tvec_t_base_s boot_tvec_bases;
    >
    > #define TIMER_INITIALIZER(_function, _expires, _data) { \
    > .function = (_function), \
    > @@ -62,7 +88,8 @@
    > return timer->entry.next != NULL;
    > }
    >
    > -extern void add_timer_on(struct timer_list *timer, int cpu);
    > +extern void add_timer_on(struct timer_list *timer, int cpu,
    > + int not_critical_when_idle);

    That `not_critical_when_idle' is a real mouthful. Could we just use
    "deferrable"? With a nice comment in a strategic spot which explains what
    it's all about?

    > extern int del_timer(struct timer_list * timer);
    > extern int __mod_timer(struct timer_list *timer, unsigned long expires);
    > extern int mod_timer(struct timer_list *timer, unsigned long expires);
    > @@ -144,6 +171,16 @@
    > static inline void add_timer(struct timer_list *timer)
    > {
    > BUG_ON(timer_pending(timer));
    > + timer->base = TBASE_CLEAR_DELAYED_ON_IDLE(timer->base);
    > + __mod_timer(timer, timer->expires);
    > +}
    > +
    > +static inline void add_timer_with_hint(struct timer_list *timer,
    > + int not_critical_when_idle)
    > +{
    > + BUG_ON(timer_pending(timer));
    > + timer->base = TBASE_MERGE_DELAYED_ON_IDLE(timer->base,
    > + not_critical_when_idle);
    > __mod_timer(timer, timer->expires);
    > }

    I'm not sure I really like the idea of modifying a timer's state when
    installing it. Plus mod_timer() is a superset of add_timer(), and people
    might want to use mod_timer().

    I think it would be better to specify the type of the timer when we're
    initialising it, not when we're installing it.

    So would it not be nicer to do:

    setup_deferrable_timer(timer, my_handler, my_data);
    add_timer(timer, whenever);
    or
    mod_timer(timer, whenever);

    ?

    > - add_timer_on(timer, cpu);
    > + add_timer_on(timer, cpu, 0);

    OK.


    -
    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: 2007-03-18 09:13    [W:0.029 / U:1.036 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site