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

[Paul Cercueil <paul@crapouillou.net>]

Hi Thomas,


Le mar., févr. 11, 2020 at 11:28, Thomas Gleixner <tglx@linutronix.de> 
a écrit :
> Paul!
> 
> Paul Cercueil <paul@crapouillou.net> 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