>-----Original Message-----
>From: Andrew Morton [mailto:[EMAIL PROTECTED]
>Sent: Sunday, March 18, 2007 1:10 AM
>To: Pallipadi, Venkatesh
>Cc: Dave Jones; [EMAIL PROTECTED]; linux-kernel
>Subject: Re: [PATCH 1/2] Add not_critical_when_idle timer
>
>On Fri, 16 Mar 2007 15:07:35 -0700 Venkatesh Pallipadi
><[EMAIL PROTECTED]> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
