Re: [PATCH RESEND v1 06/16] vfs: add temp calculation function

[Zhi Yong Wu]

On Thu, Jan 10, 2013 at 8:53 AM, David Sterba <dsterba@xxxxxxx> wrote:
> On Thu, Dec 20, 2012 at 10:43:25PM +0800, zwu.kernel@xxxxxxxxx wrote:
>> --- a/fs/hot_tracking.c
>> +++ b/fs/hot_tracking.c
>> @@ -25,6 +25,14 @@
>>  static struct kmem_cache *hot_inode_item_cachep __read_mostly;
>>  static struct kmem_cache *hot_range_item_cachep __read_mostly;
>>
>> +static u64 hot_raw_shift(u64 counter, u32 bits, bool dir)
>> +{
>> +     if (dir)
>> +             return counter << bits;
>> +     else
>> +             return counter >> bits;
>> +}
>
> I don't understand the purpose of this function, it obscures a simple
> bitwise shift.
The following words seem to mean that you prefer removing this
shifting function?

>
>> +
>>  /*
>>   * Initialize the inode tree. Should be called for each new inode
>>   * access or other user of the hot_inode interface.
>> @@ -315,6 +323,72 @@ static void hot_freq_data_update(struct hot_freq_data *freq_data, bool write)
>>  }
>>
>>  /*
>> + * hot_temp_calc() is responsible for distilling the six heat
>> + * criteria down into a single temperature value for the data,
>> + * which is an integer between 0 and HEAT_MAX_VALUE.
>
> I didn't find HEAT_MAX_VALUE defined anywhere.
This micro is only used in some comments, OK, i will replace it with 255.
>
>> + */
>> +static u32 hot_temp_calc(struct hot_freq_data *freq_data)
>> +{
>> +     u32 result = 0;
>> +
>> +     struct timespec ckt = current_kernel_time();
>> +     u64 cur_time = timespec_to_ns(&ckt);
>> +
>> +     u32 nrr_heat = (u32)hot_raw_shift((u64)freq_data->nr_reads,
>> +                                     NRR_MULTIPLIER_POWER, true);
>> +     u32 nrw_heat = (u32)hot_raw_shift((u64)freq_data->nr_writes,
>> +                                     NRW_MULTIPLIER_POWER, true);
>
> So many typecasts, some of them unnecessary and in connection with
> hot_raw_shift this is hard to read and understand.
>
>         u32 nrr_heat = (u32)((u64)freq_data->nr_reads << NRR_MULTIPLIER_POWER);
Do you prefer this format instead of ?
>
> is not much better without a comment why this is doing the right thing.
>
>> +
>> +     u64 ltr_heat =
>> +     hot_raw_shift((cur_time - timespec_to_ns(&freq_data->last_read_time)),
>> +                     LTR_DIVIDER_POWER, false);
>> +     u64 ltw_heat =
>> +     hot_raw_shift((cur_time - timespec_to_ns(&freq_data->last_write_time)),
>> +                     LTW_DIVIDER_POWER, false);
>> +
>> +     u64 avr_heat =
>> +     hot_raw_shift((((u64) -1) - freq_data->avg_delta_reads),
>> +                     AVR_DIVIDER_POWER, false);
>> +     u64 avw_heat =
>> +     hot_raw_shift((((u64) -1) - freq_data->avg_delta_writes),
>> +                     AVW_DIVIDER_POWER, false);
>> +
>> +     /* ltr_heat is now guaranteed to be u32 safe */
>> +     if (ltr_heat >= hot_raw_shift((u64) 1, 32, true))
>> +             ltr_heat = 0;
>> +     else
>> +             ltr_heat = hot_raw_shift((u64) 1, 32, true) - ltr_heat;
>> +
>> +     /* ltw_heat is now guaranteed to be u32 safe */
>> +     if (ltw_heat >= hot_raw_shift((u64) 1, 32, true))
>> +             ltw_heat = 0;
>> +     else
>> +             ltw_heat = hot_raw_shift((u64) 1, 32, true) - ltw_heat;
>> +
>> +     /* avr_heat is now guaranteed to be u32 safe */
>> +     if (avr_heat >= hot_raw_shift((u64) 1, 32, true))
>> +             avr_heat = (u32) -1;
>> +
>> +     /* avw_heat is now guaranteed to be u32 safe */
>> +     if (avw_heat >= hot_raw_shift((u64) 1, 32, true))
>> +             avw_heat = (u32) -1;
>> +
>> +     nrr_heat = (u32)hot_raw_shift((u64)nrr_heat,
>> +             (3 - NRR_COEFF_POWER), false);
>> +     nrw_heat = (u32)hot_raw_shift((u64)nrw_heat,
>> +             (3 - NRW_COEFF_POWER), false);
>> +     ltr_heat = hot_raw_shift(ltr_heat, (3 - LTR_COEFF_POWER), false);
>> +     ltw_heat = hot_raw_shift(ltw_heat, (3 - LTW_COEFF_POWER), false);
>> +     avr_heat = hot_raw_shift(avr_heat, (3 - AVR_COEFF_POWER), false);
>> +     avw_heat = hot_raw_shift(avw_heat, (3 - AVW_COEFF_POWER), false);
>> +
>> +     result = nrr_heat + nrw_heat + (u32) ltr_heat +
>> +             (u32) ltw_heat + (u32) avr_heat + (u32) avw_heat;
>
> Reading through the function up to here I've got lost in the shifts that
> I don't see the meaning of the resulting value and how can I interpet it
> if I watch it change over time. What are the expected weights of the
> number and time factors? There are more details in the documentation, but
> the big picture is blurred by talking implementation details.
>
> Let's put the impl. details here and write a better user documentation
> with a few examples to the docs. Is it possible to describe some common
> access patterns and how they affect the temperature?
>
> You've been benchmarking this patchset, I'm sure you can write up a few
> examples based on that.
>
>> +
>> +     return result;
>> +}
>
> david



-- 
Regards,

Zhi Yong Wu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/