Re: [PATCH 1/2] kernel-shark: Fix a bug in ksmodel_set_next_bin_edge() part II

[Steven Rostedt <rostedt@goodmis.org>]

Hi Yordan,

Please use a more descriptive subject. Seeing a "part II" and a "part
III" in a git log --oneline isn't very useful.

Something like:

 kernel-shark: Only increment the upper edge bin without touching lower edge.

Or something that describes what is being done in a bit more detail.

Thanks!

-- Steve

On Wed, 12 Jun 2019 17:20:52 +0300
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> On a first glance this patch may looks like reverting commit
> 9336dd6bcd38 (kernel-shark: Fix a bug in ksmodel_set_next_bin_edge())
> 
> The point is that for the last bin we want to increment its upper edge
> used  when checking if the bin is empty, but we do not want to touch
> the lower edge time used by kshark_find_entry_by_time().
> 
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/src/libkshark-model.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
> index 978cd70..0cac924 100644
> --- a/kernel-shark/src/libkshark-model.c
> +++ b/kernel-shark/src/libkshark-model.c
> @@ -260,20 +260,30 @@ static size_t ksmodel_set_upper_edge(struct kshark_trace_histo *histo)
>  static void ksmodel_set_next_bin_edge(struct kshark_trace_histo *histo,
>  				      size_t bin, size_t last_row)
>  {
> -	size_t time, next_bin = bin + 1;
> +	size_t time_min, time_max, next_bin = bin + 1;
>  	ssize_t row;
>  
> -	/* Calculate the beginning of the next bin. */
> -	time = histo->min + next_bin * histo->bin_size;
> +	/* Calculate the beginning and the end of the next bin. */
> +	time_min = histo->min + next_bin * histo->bin_size;
> +	time_max = time_min + histo->bin_size;
> +	/*
> +	 * The timestamp of the very last entry of the dataset can be exactly
> +	 * equal to the value of the upper edge of the range. This is very
> +	 * likely to happen when we use ksmodel_set_in_range_bining(). In this
> +	 * case we have to increase the size of the very last bin in order to
> +	 * make sure that the last entry of the dataset will fall into it.
> +	 */
> +	if (next_bin == histo->n_bins - 1)
> +		++time_max;
>  
>  	/*
>  	 * Find the index of the first entry inside
> -	 * the next bin (timestamp > time).
> +	 * the next bin (timestamp > time_min).
>  	 */
> -	row = kshark_find_entry_by_time(time, histo->data, last_row,
> +	row = kshark_find_entry_by_time(time_min, histo->data, last_row,
>  					histo->data_size - 1);
>  
> -	if (row < 0 || histo->data[row]->ts >= time + histo->bin_size) {
> +	if (row < 0 || histo->data[row]->ts >= time_max) {
>  		/* The bin is empty. */
>  		histo->map[next_bin] = KS_EMPTY_BIN;
>  		return;