linux-kernel - RE: [PATCH 1/2] Add not_critical_when

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Sun, 18 Mar 2007 06:20:42 -0700
From:	"Pallipadi, Venkatesh" <venkatesh.pallipadi@...el.com>
To:	"Andrew Morton" <akpm@...ux-foundation.org>
Cc:	"Dave Jones" <davej@...hat.com>, <tglx@...utronix.de>,
	"linux-kernel" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/2] Add not_critical_when_idle timer

 

>-----Original Message-----
>From: Andrew Morton [mailto:akpm@...ux-foundation.org] 
>Sent: Sunday, March 18, 2007 1:10 AM
>To: Pallipadi, Venkatesh
>Cc: Dave Jones; tglx@...utronix.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@...el.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/