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

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

[Yordan Karadzhov]

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;
-- 
2.20.1

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

[Yordan Karadzhov]

All time stamps of the trace records are coded with 64 bits, however
on some systems the size_t type can be 32 bits.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/libkshark-model.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
index 0cac924..dde64de 100644
--- a/kernel-shark/src/libkshark-model.c
+++ b/kernel-shark/src/libkshark-model.c
@@ -260,7 +260,8 @@ 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_min, time_max, next_bin = bin + 1;
+	uint64_t time_min, time_max;
+	size_t next_bin = bin + 1;
 	ssize_t row;
 
 	/* Calculate the beginning and the end of the next bin. */
-- 
2.20.1

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

[Steven Rostedt]

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;

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

[Yordan Karadzhov]

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;
> 

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

[Steven Rostedt]

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