Re: [PATCH v3 1/4] tracing: Print the bucket size as unsigned

[Donggeun Yoo]

On Mon, 14 Sep 2026 05:46:14 +0000, sashiko-bot@xxxxxxxxxx wrote:
> [Severity: High]
> This isn't a bug introduced by this patch, but does the unsigned division
> in hist_field_bucket() break the bucketing logic for negative numbers?
>
> If the field is signed and negative, its 2's complement representation is
> large. Performing unsigned division via div64_ul() or an unsigned cast
> without checking hist_field->is_signed seems like it would calculate the
> remainder incorrectly. This would happen whenever a histogram is configured
> with buckets over a signed field that takes negative values, mapping them
> to the wrong bucket intervals.

Yes, and the two other issues in this mail are real as well. The 32-bit
truncation in DEFINE_HIST_FIELD_FN() and the one in hist_field_log2() are
posted:

  https://lore.kernel.org/all/20260917015532.103081-1-donggeunyoo.kernel@xxxxxxxxx/
  https://lore.kernel.org/all/20260917023834.216893-1-donggeunyoo.kernel@xxxxxxxxx/

This one I have not posted. When I went to write it, it turned out to need
more change than either of those, and I would rather hear what people think
of the approach first.

What it looks like today. The largest multiple of ten that fits in a u64 is
2^64 - 6, so with .buckets=10 over a signed field the groups below zero are
-6..-1, then -16..-7, then -26..-17: none of the boundaries fall on a
multiple of ten and the group next to zero holds six values. At the end of
the range the two ends meet. S64_MAX and S64_MIN are 9223372036854775807
and 9223372036854775808 unsigned, and both divide down to
9223372036854775800, so one group holds both.

One detail in the report is off. hist_field->is_signed is 0 on a .buckets
key even over a signed field. create_hist_field() takes the modifier
branch, copies size and type from operands[0] and stops. hist_debug on
"keys=arg.buckets=10" over an s32 field prints

  type: s32 ... is_signed: 0

The member that does carry it is hist_field->field->is_signed, which is
already what create_tracing_map_fields() passes to tracing_map_cmp_num() to
pick the sort comparator - so the tree orders such a key signed today while
grouping and printing it unsigned.

The rendering half of that is posted separately:

  https://lore.kernel.org/all/20260917045918.370993-1-donggeunyoo.kernel@xxxxxxxxx/

What I have for the grouping:

 - Take the signedness from hist_field->field->is_signed, so the grouping,
   the sort and the rendering all come from one place.

 - For a negative value, round toward negative infinity instead of dividing
   the two's complement. Boundaries then stay on multiples of the size on
   both sides of zero, so .buckets=10 groups -10..-1 and 0..9.

 - Clamp the lowest group at S64_MIN. The boundary below it is not
   representable in the u64 a key is stored in, so that group is short,
   the same way .buckets already has a short group at the top of an
   unsigned range. Its printed end has to come from the true boundary
   rather than start + size - 1, or it overlaps the group above it. With
   .buckets=10 it prints as

     { arg: ~ -9223372036854775808--9223372036854775801 } hitcount: 2

   and the next group starts at -9223372036854775800. With a size that
   divides 2^63 nothing is short.

 - Print the range signed, which cannot be done on its own: rendering
   today's grouping signed would name a range that does not contain
   S64_MIN.

Three things I would like an opinion on.

Should .buckets interpret signedness at all? histogram.rst says "in general
the semantics of a given field aren't interpreted when applying a modifier
to it", which reads against this. On the other side, the sort comparator is
already chosen from field->is_signed, so the tree does interpret it, just
not for grouping.

Is the short group at S64_MIN acceptable? I do not see a way to avoid it
that keeps the boundaries on multiples of the size, and anchoring the grid
at S64_MIN instead puts zero inside a group.

Is this a fix or a change? It alters what .buckets prints for any signed
field holding negative values. No ftrace selftest uses .buckets, but it is
still visible output, so I am not sure a Fixes: tag is the right framing.

I have it written and measured against three arms in QEMU if an RFC posting
would be more useful than this description.

Thanks,
Donggeun