Hi Graeme,
On Wed, Jun 15, 2011 at 4:09 PM, Graeme Russ graeme.russ@gmail.com wrote: [snip]
BTW should the deltas return a signed value?
No - times are unsigned utilising the entire range of the u32 so (to - from) will always returned the unsigned delta, even if there is a wrap between them. I cannot imagine we would ever need to measure time backward anyway.
Can you please explain this again in different words as I don't follow. The difference between two unsigned times may be +ve or -ve, surely. For example, say:
now = 100 event_time = 200
then the delta is currently -100. Some people might loop until the delta is >= 0. This is like setting a future time and waiting for it to happen. If the delta is unsigned then this won't work.
No I mean _ms at the end, so it is always last. time_min_ms_since() sounds like a martian trying to learn English. See my comment above:
I read it like: Time API - Minimum Milliseconds Since (Epoch). Last time I checked I wasn't a martian ;)
I see. I wonder how you checked? Skin color and height I suppose.
time_min_since_ms reads like: Time API - Minimum Since Millisecond (Epoch)
Anyway, time_since_ms() is fine by me
time_since_ms() - we shouldn't even define time_since_raw_ms() and time_since_max_ms() in my view.
I agree that the raw versions should be dropped entirely. The max versions should be kept (and will be used for boot profiling as you are after the maximum time an operation could possibly have run)
OK I wasn't suggesting dropping raw, just hiding it. My main point was just that the common one should have the obvious name. Anyway, I suppose raw is not that useful.
This code from your excellent wiki page bothers me. Can we find a way to shrink it?
now = timer_ms_now(); if (time_ms_delta_min(start, now)> timeout)
How about:
if (time_since_ms(start) > timeout)
The idea of the time 'since' an event is more natural than the delta between then and now which seems more abstract.
I tend to agree - I have been thinking about 'since' functions for a while now
Cool.
[snip]
With the 'time_ms_' prefix, it's starting to get rather long, and I'm pushing a lot of timeout checks beyond 80 columns - especially when existing code has variables like 'start_time_tx' - I'm starting to consider dropping the time_ prefix (all time functions will still have a ms_ or us_ prefix anyway) and rename a lot of variables
Now I see why you want units at the start. Seems a bit ugly to me - which lines are getting long - do you mean the time delta check line? If so see above, or perhaps it is shorter without _min.
An example from drivers/net/ns7520_eth.c:
ulStartJiffies = time_ms_now(); while (time_ms_delta_min(ulStartJiffies, time_ms_now()) < NS7520_MII_NEG_DELAY) {
Could be reduced to:
ulStartJiffies = time_ms_now(); while (time_min_ms_since(ulStartJiffies) < NS7520_MII_NEG_DELAY) {
And with a rename:
start_ms = time_ms_now(); while (time_min_ms_since(start_ms) < NS7520_MII_NEG_DELAY) {
or:
start_ms = time_now_ms(); while (time_since_ms(start_ms) < NS7520_MII_NEG_DELAY) {
OK, So the API could now look like:
/* Functions to retrieve the current value of the timers */ u32 time_now_ms(void); u32 time_now_us(void); u64 time_now_ticks(void);
/* Functions to determine 'minimum' time elapsed (the usual case) */ u32 time_since_ms(u32 start); u32 time_since_us(u32 start); u64 time_since_us(u64 start);
time_since_ticks()
/* Functions to determine 'maximum' time elapsed (performance profiling) */ u32 time_max_since_ms(u32 start); u32 time_max_since_us(u32 start); u64 time_max_since_ticks(u64 start);
/* Default udelay() */ void udelay(u32 delay) { u32 start = time_now_us();
while(time_since_us(start) < delay) ; }
Well it has my vote - does it solve your 80-column problems? Some might complain about the proliferation of 'since' functions. I don't have a strong view on this - but it is perhaps preferable to having everyone do things manually. The strongly orthogonal nature of your functions helps with understanding anyway I think.
One interesting aspect to me is whether it might be possible for the 'since' functions to have a debug mode which prints out a message when the boot appears to be hung. Sometimes U-Boot hangs on USB or similar and there is no message displayed. I imagine it might be possible to detect a hung or repeating timeout and print a stack trace. Another thing is how much time was spent hanging around during a boot (as a summary at the end). Just a thought, and NOT suggesting doing anything about it now.
Regards, Simon
Regards,
Graeme