Re: [PATCH v5 01/14] cpu-timers, icount: new modules

[Claudio Fontana <cfontana@suse.de>]

Hello Richard,

On 8/14/20 10:20 PM, Richard Henderson wrote:
> On 8/12/20 11:32 AM, Claudio Fontana wrote:
>> +/*
>> + * Return the icount enablement state:
>> + *
>> + * 0 = Disabled - Do not count executed instructions.
>> + * 1 = Enabled - Fixed conversion of insn to ns via "shift" option
>> + * 2 = Enabled - Runtime adaptive algorithm to compute shift
>> + */
>> +int icount_enabled(void);
> 
> Why does use_icount need to change to a function?


It is not useful at this point, I'll change this.


> 
> If it does, or even if this just comes under the heading of cleanup, it should
> certainly be done in a separate patch.


Yes, I'll move it to a different series entirely.

> 
> Either way, I think we should expose the fact that this is always disabled when
> #ifndef CONFIG_TCG, just like we do for tcg_enabled().

Will do.

> 
>> -        if (use_icount) {
>> -            return cpu_get_icount();
>> +        if (icount_enabled()) {
>> +            return icount_get();
> 
> Renaming of other functions like this should also be done in a separate patch.

Is there a compelling reason to separate the new module/class from what are effectively its methods?
It seemed to me that creating a new icount module would warrant the name changes in the same patch.

> 
> 
> r~
> 

I will be out of office until end of the month,

thanks,

Claudio