| Message ID | 20190612142053.14439-1-ykaradzhov@vmware.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] kernel-shark: Fix a bug in ksmodel_set_next_bin_edge() part II | expand |
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;
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; >
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
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;
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(-)