lkml.org 
[lkml]   [2007]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRE: [PATCH 1/2] Add not_critical_when_idle timer
    Date
    From


    >-----Original Message-----
    >From: Andrew Morton [mailto:akpm@linux-foundation.org]
    >Sent: Sunday, March 18, 2007 1:10 AM
    >To: Pallipadi, Venkatesh
    >Cc: Dave Jones; tglx@linutronix.de; linux-kernel
    >Subject: Re: [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:
    >
    >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.

    This is not totally unrelated, as I want this base structure to be
    aligned in UP case as well, as the lower bits in the pointer to base is
    overloaded.

    >> ...
    >>
    >> +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?

    Yes. Will do.

    >>
    >> -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?
    >

    Yes. deferrable sounds better. Will change.

    >> 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);
    >

    OK. I will look at having the setup functions this way. I guess this
    should be better as we will never want to use a single timer in both
    deferrrable and non-deferrable modes. Will refresh the patch with these
    changes soon.

    Thanks,
    Venki
    -
    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 14:23    [W:0.035 / U:0.472 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site