Re: [PATCH v4 01/10] timekeeper: introduce extended clocksource reading callback

[John Stultz]

On Wed, Aug 2, 2017 at 7:38 AM, Denis Plotnikov
<dplotnikov@xxxxxxxxxxxxx> wrote:
> The callback provides extended information about just read
> clocksource value.
>
> It's going to be used in cases when detailed system information
> needed for further time related values calculation, e.g in KVM
> masterclock settings calculation.
>
> Signed-off-by: Denis Plotnikov <dplotnikov@xxxxxxxxxxxxx>
> ---
>  include/linux/clocksource.h | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index a78cb18..786a522 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -48,7 +48,14 @@ struct module;
>   *                     400-499: Perfect
>   *                             The ideal clocksource. A must-use where
>   *                             available.
> - * @read:              returns a cycle value, passes clocksource as argument
> + * @read:              returns a cycle value (might be not quite cycles:
> + *                     see pvclock) passes clocksource as argument
> + * @read_with_stamp:   saves cycles value (got from timekeeper) and cycles
> + *                     stamp (got from hardware counter value and used by
> + *                     timekeeper to calculate the cycles value) to
> + *                     corresponding input pointers return true if cycles
> + *                     stamp holds real cycles and false if it holds some
> + *                     time derivative value
>   * @enable:            optional function to enable the clocksource
>   * @disable:           optional function to disable the clocksource
>   * @mask:              bitmask for two's complement
> @@ -78,6 +85,8 @@ struct module;
>   */
>  struct clocksource {
>         u64 (*read)(struct clocksource *cs);
> +       bool (*read_with_stamp)(struct clocksource *cs,
> +                               u64 *cycles, u64 *cycles_stamp);
>         u64 mask;

I'm not really fan of an interface that leaks magic data to users that
know enough.

And its not clear from this if the magic data is standardized or
different clocksources export different data?

What exactly are the attributes you're trying to pull from the
lower-level hardware that you can't get otherwise (without using the
update_pvclock_gtod() since, if I'm understanding that apparently
gives you too much detail to deal with)?

thanks
-john