| Message ID | 20190221124205.21115-4-ykaradzhov@vmware.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9336dd6bcd381b50ebdc707c90d86e00ceb78d7a |
| Headers | show |
| Series | KernelShark visualization model fixes | expand |
On Thu, Feb 21, 2019 at 02:42:05PM +0200, Yordan Karadzhov wrote: > The modification of the last bin of the model makes no sense (this is > my mistake). The comment above the code that is doing this modification > is partially correct, however it speaks about increasing the size of > the last bin, while the code below the comment changes the lower edge > of this bin. The actual increase of the size of the last bin is done in > ksmodel_set_upper_edge() where the lower edge of the Upper Overflow bin > gets shifted (max + 1). This effectively increases the size of the last bin. > > Reported-by: Tzvetomir Stoyanov <tstoyanov@vmware.com> > Fixes: f97e31f00 ("kernel-shark-qt: Introduce the visualization model ..") > Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com> > --- > kernel-shark/src/libkshark-model.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c > index b6d3612..4bd1e2c 100644 > --- a/kernel-shark/src/libkshark-model.c > +++ b/kernel-shark/src/libkshark-model.c > @@ -266,15 +266,6 @@ static void ksmodel_set_next_bin_edge(struct kshark_trace_histo *histo, > /* Calculate the beginning of the next bin. */ > time = histo->min + next_bin * 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; > /* > * Find the index of the first entry inside > * the next bin (timestamp > time). > -- > 2.17.1 > Patches 1, 2 and 3 look good to me. Acked-by: Slavomir Kaslev <kaslevs@vmware.com> Side note: ksmodel_shift_backward/ksmodel_shift_forward look pretty similar. Can they be generalized to an either-directional ksmodel_shift taking the shift parameter as signed? If ksmodel_shift_backward/ksmodel_shift_forward need to be kept for backward compatibility, they can call ksmodel_shift (with a flipped sign in the backward case). Thank you, -- Slavi
On 21.02.19 г. 18:15 ч., Slavomir Kaslev wrote: > On Thu, Feb 21, 2019 at 02:42:05PM +0200, Yordan Karadzhov wrote: >> The modification of the last bin of the model makes no sense (this is >> my mistake). The comment above the code that is doing this modification >> is partially correct, however it speaks about increasing the size of >> the last bin, while the code below the comment changes the lower edge >> of this bin. The actual increase of the size of the last bin is done in >> ksmodel_set_upper_edge() where the lower edge of the Upper Overflow bin >> gets shifted (max + 1). This effectively increases the size of the last bin. >> >> Reported-by: Tzvetomir Stoyanov <tstoyanov@vmware.com> >> Fixes: f97e31f00 ("kernel-shark-qt: Introduce the visualization model ..") >> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com> >> --- >> kernel-shark/src/libkshark-model.c | 9 --------- >> 1 file changed, 9 deletions(-) >> >> diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c >> index b6d3612..4bd1e2c 100644 >> --- a/kernel-shark/src/libkshark-model.c >> +++ b/kernel-shark/src/libkshark-model.c >> @@ -266,15 +266,6 @@ static void ksmodel_set_next_bin_edge(struct kshark_trace_histo *histo, >> /* Calculate the beginning of the next bin. */ >> time = histo->min + next_bin * 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; >> /* >> * Find the index of the first entry inside >> * the next bin (timestamp > time). >> -- >> 2.17.1 >> > > Patches 1, 2 and 3 look good to me. > > Acked-by: Slavomir Kaslev <kaslevs@vmware.com> > > Side note: ksmodel_shift_backward/ksmodel_shift_forward look pretty similar. Can > they be generalized to an either-directional ksmodel_shift taking the shift > parameter as signed? If ksmodel_shift_backward/ksmodel_shift_forward need to be > kept for backward compatibility, they can call ksmodel_shift (with a flipped > sign in the backward case). > Thanks for the comment! You are right. I think this can be done, but in a separate patch. Yordan > Thank you, > > -- Slavi >
On Thu, 21 Feb 2019 16:38:29 +0000 Yordan Karadzhov <ykaradzhov@vmware.com> wrote: > > Acked-by: Slavomir Kaslev <kaslevs@vmware.com> Thanks for reviewing Slavomir! > > > > Side note: ksmodel_shift_backward/ksmodel_shift_forward look pretty similar. Can > > they be generalized to an either-directional ksmodel_shift taking the shift > > parameter as signed? If ksmodel_shift_backward/ksmodel_shift_forward need to be > > kept for backward compatibility, they can call ksmodel_shift (with a flipped > > sign in the backward case). > > > Thanks for the comment! > You are right. I think this can be done, but in a separate patch. I agree on both accounts. -- Steve
On Thu, 21 Feb 2019 18:15:32 +0200 Slavomir Kaslev <kaslevs@vmware.com> wrote: > Patches 1, 2 and 3 look good to me. > > Acked-by: Slavomir Kaslev <kaslevs@vmware.com> Slavomir, is it OK that I put "Reviewed-by" instead of "Acked-by". A reviewed-by caries a bit more weight and states that you did a review. An ack is more of "yeah, I looked it over and I'm OK with the idea", but doesn't actually mean you reviewed it. -- Steve
On Fri, Feb 22, 2019 at 9:34 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Thu, 21 Feb 2019 18:15:32 +0200 > Slavomir Kaslev <kaslevs@vmware.com> wrote: > > > Patches 1, 2 and 3 look good to me. > > > > Acked-by: Slavomir Kaslev <kaslevs@vmware.com> > > Slavomir, is it OK that I put "Reviewed-by" instead of "Acked-by". A > reviewed-by caries a bit more weight and states that you did a review. > An ack is more of "yeah, I looked it over and I'm OK with the idea", > but doesn't actually mean you reviewed it. Thanks for pointing it out. My bad, I didn't realize there was a difference[1]. Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com> [1]https://www.kernel.org/doc/html/v4.20/process/submitting-patches.html#reviewer-s-statement-of-oversight
diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c index b6d3612..4bd1e2c 100644 --- a/kernel-shark/src/libkshark-model.c +++ b/kernel-shark/src/libkshark-model.c @@ -266,15 +266,6 @@ static void ksmodel_set_next_bin_edge(struct kshark_trace_histo *histo, /* Calculate the beginning of the next bin. */ time = histo->min + next_bin * 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; /* * Find the index of the first entry inside * the next bin (timestamp > time).
The modification of the last bin of the model makes no sense (this is my mistake). The comment above the code that is doing this modification is partially correct, however it speaks about increasing the size of the last bin, while the code below the comment changes the lower edge of this bin. The actual increase of the size of the last bin is done in ksmodel_set_upper_edge() where the lower edge of the Upper Overflow bin gets shifted (max + 1). This effectively increases the size of the last bin. Reported-by: Tzvetomir Stoyanov <tstoyanov@vmware.com> Fixes: f97e31f00 ("kernel-shark-qt: Introduce the visualization model ..") Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com> --- kernel-shark/src/libkshark-model.c | 9 --------- 1 file changed, 9 deletions(-)