Message-Id: <1242242497.9777.2.camel@jstultz-laptop>
Date:	Wed, 13 May 2009 12:21:37 -0700
From:	John Stultz <johnstul@...ibm.com>
To:	Jon Hunter <jon-hunter@...com>
Cc:	Ingo Molnar <mingo@...e.hu>, Thomas Gleixner <tglx@...utronix.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH] Dynamic Tick: Allow 32-bit machines to sleep   
 formorethan2.15 seconds

On Wed, 2009-05-13 at 12:54 -0500, Jon Hunter wrote:
> John Stultz wrote:
> >>> Alternatively instead of NSEC_PER_SEC/HZ, we could always drop the
> >>> larger of NSEC_PER_SEC/HZ or max_deferment/10? That way we should scale
> >>> up without a problem. 
> >> Yes, may be this would be a safer option. Thinking about this I was 
> >> wondering if we should always use max_deferement/10, because I did not 
> >> think that there would ever be a case where NSEC_PER_SEC/HZ would be 
> >> greater. If NSEC_PER_SEC/HZ was greater than max_deferement/10 this 
> >> would imply that the clocksource would wrap after only 10 jiffies, if I 
> >> have the math right...
> > 
> > Right, but even with such limitiations, if an arch can skip every 5
> > ticks, they probably will try, right? :)
> 
> Sure, but I guess I was wondering if there would ever be a clocksource 
> that would overflow in 10-20 ticks? If not then it would be safe to 
> always use -10% or -5% margin and we can forget about NSEC_PER_SEC/HZ.
> 
> Unless I am understanding this wrong, but I thought we are just trying 
> to make sure we never sleep for a time longer than the total time a 
> clocksource can count.
> 
> > That sounds reasonable to me.
> 
> Great.
> 
> >> One final question, I noticed in clocksource.h that the definition of 
> >> function cyc2ns returns a type of s64, however, in the function itself a 
> >> variable of type u64 is used and returned. Should this function be 
> >> modified as follows?
> >>
> >>   static inline s64 cyc2ns(struct clocksource *cs, cycle_t cycles)
> >>   {
> >> -       u64 ret = (u64)cycles;
> >> +       s64 ret = (s64)cycles;
> >>          ret = (ret * cs->mult) >> cs->shift;
> >>          return ret;
> >>   }
> > 
> > Damn. So this brings up an issue I had missed prior.
> 
> Any comments on whether this should be u64 versus s64?

I'd leave it alone for now. I'm concerns that in large multiplies, if
its a s64 the sign might get extended down by the shift. I need to look
at it in more detail though.


> > I'll have to think about how that would change
> > timekeeping_max_deferment() and how we'd have to calculate a reasonable
> > max efficiently.
> > 
> > Other then this issue (which is my fault for not noticing it earlier),
> > you're patch looks great. I just feel badly for making you rev this
> > thing over and over. 
> 
> No problem, its fine. Its more important for us to get this right so I 
> am happy to help where I can.
> 
> > One option if you're itching to push it in and be done with it: Make
> > timekeeping_max_deferment() return just 1 second for now. Your patch
> > provides the right infrastructure for the timekeeping code to provide
> > its limits to the clockevents code. So you can use a safe short constant
> > value for now, and we can extend that out correctly in a future patch.
> 
> How about going back to your original thought and making it 50% margin 
> for now? In other words, use max_deferment/2? Therefore, for clocksource 
> that can count for 10s of years before overflowing it will not be as 
> severe.

Well, even the 50% margin might be trouble, since it may overflow the
cyc2ns() function if the shift is large (on many clocksources is is
fairly large).

-john




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

Powered by blists - more mailing lists