On Wed, 20 Feb 2019 17:29:34 +0200
"Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> wrote:
> On 20.02.19 г. 16:51 ч., Steven Rostedt wrote:
> > 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?
Sorry, I meant n + 1.
histo->n_bins = 5, n = 4.
bin = (5) - (4) - 2 = -1.
-- Steve
>
> This is not possible.
> Several lines above in the code we have
>
>
> if (n >= histo->n_bins) {
> /*
> * No overlap between the new and the old ranges. Recalculate
> * all bins from scratch. First calculate the new range.
> */
> ksmodel_set_bining(histo, histo->n_bins, histo->min,
> histo->max);
>
> ksmodel_fill(histo, histo->data, histo->data_size);
> return;
> }
|