linux-kernel - Re: [PATCH] Add support for deferrable timers (respun)

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20070327223625.GA30923@linux-os.sc.intel.com>
Date:	Tue, 27 Mar 2007 15:36:25 -0700
From:	Venki Pallipadi <venkatesh.pallipadi@...el.com>
To:	Oleg Nesterov <oleg@...sign.ru>
Cc:	Venki Pallipadi <venkatesh.pallipadi@...el.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	akpm@...ux-foundation.org, davej@...emonkey.org.uk,
	johnstul@...ibm.com, mingo@...e.hu, tglx@...utronix.de
Subject: Re: [PATCH] Add support for deferrable timers (respun)

On Wed, Mar 28, 2007 at 02:22:27AM +0400, Oleg Nesterov wrote:
> On 03/27, Venki Pallipadi wrote:
> >
> > @@ -368,7 +368,7 @@
> >  
> >  	for (;;) {
> >  		tvec_base_t *prelock_base = timer->base;
> > -		base = timer_get_base(timer);
> > +		base = tbase_get_base(prelock_base);
> >  		if (likely(base != NULL)) {
> >  			spin_lock_irqsave(&base->lock, *flags);
> >  			if (likely(prelock_base == timer->base))
> 
> Looks correct to me... Personally, I'd prefer
> 
> 	static tvec_base_t *lock_timer_base(struct timer_list *timer,
> 						unsigned long *flags)
> 		__acquires(timer->base->lock)
> 	{
> 		tvec_base_t *base;
> 
> 		for (;;) {
> 			base = timer_get_base(timer);
> 			if (likely(base != NULL)) {
> 				spin_lock_irqsave(&base->lock, *flags);
> 				if (likely(base == timer_get_base(timer))
> 					return base;
> 				/* The timer has migrated to another CPU */
> 				spin_unlock_irqrestore(&base->lock, *flags);
> 			}
> 			cpu_relax();
> 		}
> 	}
> 
> but this is a matter of taste.

I thought about this. But, chose the other one just to save one additional
'and' overhead.


> 
> A minor nitpick,
> 
> > +/* new_base is guaranteed to have last bit not set, in all callers below */
> > +static inline void timer_set_base(struct timer_list *timer,
> > +                                       struct tvec_t_base_s *old_base,
> > +                                       struct tvec_t_base_s *new_base)
> > +{
> > +       timer->base = (struct tvec_t_base_s *)((unsigned long)(new_base) |
> > +                                              tbase_get_deferrable(old_base));
> > +}
> 
> looks a little bit ugly, but may be this is just me. How about
> 
> 	void timer_set_base(struct timer_list *timer, struct tvec_t_base_s *new_base)
> 	{
> 		timer->base = (struct tvec_t_base_s *)
> 			((unsigned long)(new_base) | tbase_get_deferrable(timer->base));
> 	}
> 
> __mod_timer:
> 	-	tvec_base_t *old_base = timer->base;
> 	-	timer->base = NULL;
> 	+	timer_set_base(timer, NULL);
>
> ?

I agree the above suggestion is clean. But, it will have one additional 'and'
operation when we set NULL. I saw some concern from Andrew earlier on overhead
this patch was adding.

> 
> > +                       /* Make sure that tvec_base is 2 byte aligned */
> > +                       if (tbase_get_deferrable(base)) {
> > +                               WARN_ON(1);
> > +                               kfree(base);
> > +                               return -ENOMEM;
> > +                       }
> 
> Not a comment, but a question: do we really need this?

AFAIK, kmalloc_node should return an even address always. I was just being
paranoid and wanted to assert it here as otherwise some normal timer may end up
being deferred timer.

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/