Re: [PATCH] lib/string_helpers: fix string_get_size() unit promotion after rounding

[Andy Shevchenko]

First of all, do not top-post!

Second, I just realised now that the discussion was done in private emails, do
not do this. Please, answer again with all what was discussed to the public
mailing lists as this message does.

On Fri, Apr 10, 2026 at 09:11:59PM +0545, Shuvam Pandey wrote:
> I think the open question is whether string_get_size() is intended to
> round across a unit boundary at all.
> There seem to be two possible interpretations:
> 
> 1. Keep the current 3-significant figure rounding, and if that rounding
> carries to the unit divisor, renormalize into the next unit. That is
> what my patch does.
> 
> 2. Treat the unit boundary as hard, so values below 1.00 MiB / 1.00 MB
> stay below that threshold and are not rounded across it.
> 
> If (2) is the intended behavior, then this is broader than the
> carry-renormalization case and my patch is not the right fix.
> 
> Which behavior should string_get_size() implement here, or am I
> missing a third intended behavior?
> 
> On Thu, Apr 9, 2026 at 1:55 AM Shuvam Pandey <shuvampandey1@xxxxxxxxx>
> wrote:
> 
> > I checked the additional base 10 values as well.
> >
> > For blk_size = 1, the current code gives:
> >
> > 999999:
> >
> > STRING_UNITS_10: before 1000 kB, after 1.00 MB
> >
> > 1000000:
> >
> > STRING_UNITS_10: before 1.00 MB, after 1.00 MB
> >
> > 1000001:
> >
> > STRING_UNITS_10: before 1.00 MB, after 1.00 MB
> >
> > 1048575:
> >
> > STRING_UNITS_2: before 1024 KiB, after 1.00 MiB
> >
> > 1048576:
> >
> > STRING_UNITS_2: before 1.00 MiB, after 1.00 MiB
> >
> > 1048577:
> >
> > STRING_UNITS_2: before 1.00 MiB, after 1.00 MiB
> >
> > So this patch only changes the carry-to-divisor cases, where the current
> > code rounds the value up but still prints it in the old unit.
> >
> > That said, I think your point about the broader semantics is valid.
> >
> > string_get_size() has long rounded to 3 significant figures, and my
> > change only makes that existing behavior consistent after a carry. If
> > intended behavior is instead that values like 1048575 should stay
> > below 1.00 MiB, then the real issue is not just the missing promotion
> > here, but the rounding policy itself.
> >
> > So I think the patch is correct for the current implementation, but it
> > may not be the right fix if the intended behavior near unit boundaries
> > is supposed to be different.
> >
> > I'll take another look at the expected behavior around these boundary
> > cases and follow up once I've re-evaluated that.
> >
> > On Thu, Apr 9, 2026 at 1:38 AM Andy Shevchenko <
> > andriy.shevchenko@xxxxxxxxx> wrote:
> >
> >> On Tue, Apr 07, 2026 at 08:48:59PM +0545, Shuvam Pandey wrote:
> >> > For blk_size = 1:
> >> >
> >> > 1048575:
> >> >   STRING_UNITS_10: before 1.05 MB, after 1.05 MB
> >> >   STRING_UNITS_2:  before 1024 KiB, after 1.00 MiB
> >> >
> >> > 1048576:
> >> >   STRING_UNITS_10: before 1.05 MB, after 1.05 MB
> >> >   STRING_UNITS_2:  before 1.00 MiB, after 1.00 MiB
> >> >
> >> > 1048577:
> >> >   STRING_UNITS_10: before 1.05 MB, after 1.05 MB
> >> >   STRING_UNITS_2:  before 1.00 MiB, after 1.00 MiB
> >> >
> >> > Likewise around 2 MiB:
> >> >
> >> > 2097151:
> >> >   STRING_UNITS_10: before 2.10 MB, after 2.10 MB
> >> >   STRING_UNITS_2:  before 2.00 MiB, after 2.00 MiB
> >> >
> >> > 2097152:
> >> >   STRING_UNITS_10: before 2.10 MB, after 2.10 MB
> >> >   STRING_UNITS_2:  before 2.00 MiB, after 2.00 MiB
> >> >
> >> > 2097153:
> >> >   STRING_UNITS_10: before 2.10 MB, after 2.10 MB
> >> >   STRING_UNITS_2:  before 2.00 MiB, after 2.00 MiB
> >> >
> >> > So the change only affects the carry-to-divisor case itself; values that
> >> > are already in the next unit stay unchanged.
> >> >
> >> > I also rechecked the neighboring value 1048574, and it changes from 1024
> >> > KiB to 1.00 MiB as well. I can add coverage for 1048574..1048577 in v2
> >> if
> >> > you think that would be useful.
> >>
> >> Also with base 10 for 999999 1000000 1000001...
> >>
> >> I'm trying to understand the current behaviour better and the logic of
> >> your
> >> change. But looking at the above I think your patch doesn't improve the
> >> case.
> >> And might even be another bug in the current code. I mean, do we have to
> >> round
> >> the numbers? 1048575 and lower are not equal 1.00 MiB strictly speaking.
> >> It's rather 0.999 MiB.
> >>
> >> > On Tue, Apr 7, 2026 at 8:38 PM Andy Shevchenko <
> >> andriy.shevchenko@xxxxxxxxx>
> >> > wrote:
> >> >
> >> > > On Tue, Apr 07, 2026 at 08:18:06PM +0545, Shuvam Pandey wrote:
> >> > > > string_get_size() rounds the fractional part before formatting the
> >> > > > result. If that carry pushes the integer part to the unit divisor,
> >> the
> >> > > > value is still printed in the old unit.
> >> > > >
> >> > > > This yields outputs like "1000 kB" for 999500 bytes and "1024 KiB"
> >> for
> >> > > > 1048064 bytes instead of promoting them to the next unit.
> >> > >
> >> > > What does it print for 1048575, 1048576, 1048577 respectively?
> >> > > (before and after your patch)
> >> > >
> >> > > All the same for numbers around 2MiB.
> >> > >
> >> > > > Renormalize the value after the carry so it is promoted before
> >> > > > formatting. Add KUnit coverage for the decimal and binary boundary
> >> > > > cases.

-- 
With Best Regards,
Andy Shevchenko