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

[Paul Cercueil]

Hi Thomas,


Le mar., févr. 11, 2020 at 11:28, Thomas Gleixner <tglx@xxxxxxxxxxxxx> 
a écrit :
> 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() ?

I'm open to suggestions :)
The point of using a different function was to avoid a huge patchset to 
fix the 50+ drivers that use sched_clock_register().

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

Alright, I really didn't think it was that bad. Next time I'll use a 
wrapper.

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

The fact that every other driver with the same problem decides to add a 
workaround instead of a proper fix does not mean that the problem does 
not exist.

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

My allergy of static storage is not worth the trouble of a treewide 
pathset so I'll just drop this patch and send a v5.

Thanks,
-Paul