Re: Using sched_clock for mmio-trace

[Jeff Muizelaar <jeff@infidigm.net>]

On Fri, Feb 16, 2007 at 10:28:50AM -0800, Daniel Walker wrote:
> On Fri, 2007-02-16 at 13:10 -0500, Jeff Muizelaar wrote:
> > On Fri, Feb 16, 2007 at 09:45:21AM -0800, Daniel Walker wrote:
> > > I've been working on a patch set (below), to expose the clocksources
> > > used by generic time to multiple users . It would allow timestamps from
> > > different clocks in a generic way. It's not merged, but I'd appreciate
> > > any input either of you might have..
> > > 
> > > ftp://source.mvista.com/pub/dwalker/clocksource/
> > 
> > Is it possible to see the resulting clocksource.h and maybe
> > clocksource.c after the patch set? That would make looking at it much
> > easier.
> 
> Well you could just apply the patch set, but I stuck them in the same
> directory as above .. I'll delete them in 24 hours or so ..
> 
> At one point I replaced sched_clock() , 
> 
> ftp://source.mvista.com/pub/dwalker/clocksource/clocksource-v10/add_generic_sched_clock.patch
> 
> The API is similar to that version, and sched_clock was the simplest
> user of the API that I've done.

Ok, so it would basically be:

init()
{
	clocksource *clock = clocksource_get_best_clock();
}

trace()
{
	trace.time = cyc2ns(clock, clocksource_read(clock));
}

This seems pretty sane to me. 

The blk-trace code calibrates a per cpu offset for the sched_clock()
time (see blk_trace_check_cpu_time and blk_trace_set_ht_offsets). Does
the clocksource stuff help me with this or would I still need to do
something like that?

I also noticed that you have a clocksource_get_masked_clock() call. This
seems like a pretty awkward API to me. The first thing that came to mind
when I read the name was 'what is a masked clock'. When I realized that
it meant 'a clock w/o this flag', I still found it awkward that one has
to specify what that don't want. e.g 'I don't want a clock that is not
continuous.'

It think it would be better if you had sometime like
'clocksource_get_clock_with_features()' that took flags describing the
needed characteristics instead of the unwanted ones.

e.g.
clocksource_get_clock_with_features(CLOCKSOURCE_STABLE)
or
clocksource_get_clock_with_features(CLOCKSOURCE_PM_UNAFFECTED)


instead of:

clocksource_get_clock_masked(CLOCKSOURCE_UNSTABLE)
clocksource_get_clock_masked(CLOCKSOURCE_PM_AFFECTED)

Especially awkward is the CLOCKSOURCE_64BIT flag, as there isn't really
anyway to specify that I want a 64bit timer, only a way to specify that
I don't.

It also isn't clear what the implications of some of the flags are:
e.g:
	NOT_CONTINUOUS - don't really have any idea what this means.
	UNSTABLE - this means the frequency can change right?
		Does PM_AFFECTED imply UNSTABLE?
	NOT_ATOMIC - does this affect me as user?
	PM_AFFECTED - it looks like the stp code deals with cpu speed
		changing. Does the clocksource code do this for me with
		cyc2ns?  If it does are there any reason I would want to
		avoid PM_AFFECTED clocks? If it doesn't how do I know
		that I need to correct it myself.

Hope this helps,

-Jeff