[1/2] kernel-shark: Fix a bug in ksmodel_set_next_bin_edge() part II
diff mbox series

Message ID 20190612142053.14439-1-ykaradzhov@vmware.com
State Superseded
Headers show
Series
  • [1/2] kernel-shark: Fix a bug in ksmodel_set_next_bin_edge() part II
Related show

Commit Message

Yordan Karadzhov June 12, 2019, 2:20 p.m. UTC
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(-)

Comments

Steven Rostedt June 12, 2019, 2:28 p.m. UTC | #1
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;
Yordan Karadzhov June 12, 2019, 2:34 p.m. UTC | #2
Hi Steven,

My idea was to make clear that those two patches are strongly connected 
to the patch another patch called:
kernel-shark: Fix a bug in ksmodel_set_next_bin_edge()

which was send long time ago.

Should I resend with another subject?
Thanks!
Yordan


On 12.06.19 г. 17:28 ч., Steven Rostedt wrote:
> 
> 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;
>
Steven Rostedt June 12, 2019, 2:48 p.m. UTC | #3
On Wed, 12 Jun 2019 14:34:52 +0000
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> Hi Steven,
> 
> My idea was to make clear that those two patches are strongly connected 
> to the patch another patch called:
> kernel-shark: Fix a bug in ksmodel_set_next_bin_edge()

It's best to note the connection in the git change log, and also use a
Fixes: tag.

> 
> which was send long time ago.
> 
> Should I resend with another subject?

Yes please.

Thanks!

-- Steve

Patch
diff mbox series

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;