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@xxxxxxxxxx> >> Fixes: f97e31f00 ("kernel-shark-qt: Introduce the visualization model ..") >> Signed-off-by: Yordan Karadzhov <ykaradzhov@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx> > > 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 >