Re: [PATCH v4 1/2] sched: Add sched_clock_register_new()

[Thomas Gleixner]

Paul!

Paul Cercueil <paul@xxxxxxxxxxxxxxx> writes:

> The sched_clock_register_new() behaves like sched_clock_register() but

This function name does not make any sense. Two years from now you are
going to provide sched_clock_register_new_2_dot_0() ?

> takes an extra parameter which is passed to the read callback.

This lacks any form of justification why this function and the data
pointer is required.

>   * @sched_clock_mask:   Bitmask for two's complement subtraction of non 64bit
>   *			clocks.
>   * @read_sched_clock:	Current clock source (or dummy source when suspended).
> + * @data:		Callback data for the current clock source.
>   * @mult:		Multipler for scaled math conversion.
>   * @shift:		Shift value for scaled math conversion.
>   *
> @@ -39,7 +40,8 @@ struct clock_read_data {
>  	u64 epoch_ns;
>  	u64 epoch_cyc;
>  	u64 sched_clock_mask;
> -	u64 (*read_sched_clock)(void);
> +	u64 (*read_sched_clock)(void *);

How is that supposed to work without fixing up _all_ sched clock
instances? So the below typecast

> +void __init
> +sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
> +{
> +	sched_clock_register_new((u64 (*)(void *))read, bits, rate, NULL);

makes it compile.

By pure luck this does not explode in your face at runtime when the
existing read(void) functions are called with an argument. Any stack
based argument passing calling convention would fall flat on it's nose.

While clever this is really an ugly hack.

As the clocksource for which you are doing this is a single instance,
what's wrong with having some static storage for the information you
need as any other driver which has the same problem does as well?

If there is really a point in avoiding a few bytes of static storage,
then this needs to be cleaned up treewide and not hacked around.

Thanks,

        tglx