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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 10 Jan 2013 01:53:02 +0100
From:	David Sterba <dsterba@...e.cz>
To:	zwu.kernel@...il.com
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	viro@...iv.linux.org.uk, david@...morbit.com,
	darrick.wong@...cle.com, andi@...stfloor.org, hch@...radead.org,
	linuxram@...ux.vnet.ibm.com, wuzhy@...ux.vnet.ibm.com
Subject: Re: [PATCH RESEND v1 06/16] vfs: add temp calculation function

On Thu, Dec 20, 2012 at 10:43:25PM +0800, zwu.kernel@...il.com 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.

> +
>  /*
>   * 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.

> + */
> +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);

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/