On Wed, 20 Feb 2019 11:16:10 +0200
Yordan Karadzhov <ykaradzhov@xxxxxxxxxx> wrote:
> As explaned in the change log of
>
> e54616484 ("Do not copy the Upper Overflow bin when shifting forward"),
>
> the lower edge of the Upper Overflow bin is unusual (shift + 1). Because
> of this, the content of the Upper Overflow bin cannot be copied, when
> shifting the visible area forward. It has to be recalculated instead.
> However, this is not enough to fix the bug. The last bin of the old histo
> cannot be copied as well. This is because its upper edge is shifted
> too (+1).
>
> Reported-by: Tzvetomir Stoyanov <tstoyanov@xxxxxxxxxx>
> Fixes: e54616484 ("Do not copy the Upper Overflow bin when shifting forward")
> Signed-off-by: Yordan Karadzhov <ykaradzhov@xxxxxxxxxx>
> ---
> kernel-shark/src/libkshark-model.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
> index b71a9b8..b80f71e 100644
> --- a/kernel-shark/src/libkshark-model.c
> +++ b/kernel-shark/src/libkshark-model.c
> @@ -488,23 +488,30 @@ void ksmodel_shift_forward(struct kshark_trace_histo *histo, size_t n)
> ksmodel_set_lower_edge(histo);
>
> /*
> - * Copy the the mapping indexes of all overlaping bins starting from
> - * bin "0" of the new histo. Note that the number of overlaping bins
> - * is histo->n_bins - n.
> * We will do a sanity check. ksmodel_set_lower_edge() sets map[0]
> * index of the new histo. This index should then be equal to map[n]
> * index of the old histo.
> */
> assert (histo->map[0] == histo->map[n]);
> +
> + /*
> + * Copy the mapping indexes of all overlaping bins starting from
> + * bin "0" of the new histo. Note that the number of overlaping bins
> + * is histo->n_bins - n. However, the last bin of the models is
> + * unusual. Its size has been increased by "1" in order make sure that
> + * the last entry of the dataset will fall into it (see the comment in
> + * ksmodel_set_next_bin_edge()). Because of this, we do not want to
> + * copy the very last bin of the old histo. We are going to recalculate
> + * its content instead. */
> memmove(&histo->map[0], &histo->map[n],
> - sizeof(histo->map[0]) * (histo->n_bins - n));
> + sizeof(histo->map[0]) * (histo->n_bins - n - 1));
>
> /*
> * Calculate only the content of the new (non-overlapping) bins.
> * Start from the last copied bin and set the edge of each consecutive
> * bin.
> */
> - bin = histo->n_bins - n - 1;
> + bin = histo->n_bins - n - 2;
Is it possible that we could have histo->n_bins == n - 1?
-- Steve
> for (; bin < histo->n_bins; ++bin) {
> ksmodel_set_next_bin_edge(histo, bin, last_row);
> if (histo->map[bin + 1] > 0)
|