Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20231024134637.3120277-1-surenb@google.com> <20231024134637.3120277-2-surenb@google.com>
 <ZTfUCiFP3hVJ+EXh@smile.fi.intel.com>
In-Reply-To: <ZTfUCiFP3hVJ+EXh@smile.fi.intel.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Tue, 24 Oct 2023 10:08:29 -0700
Message-ID: <CAJuCfpFY0JF3trYVmp5YjY3A8cH8TejrscRpA+Mjro4aA-TO6g@mail.gmail.com>
Subject: Re: [PATCH v2 01/39] lib/string_helpers: Add flags param to string_get_size()
To:     Andy Shevchenko <andy@kernel.org>
Cc:     akpm@linux-foundation.org, kent.overstreet@linux.dev,
        keescook@chromium.org, rostedt@goodmis.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Oct 24, 2023 at 7:26=E2=80=AFAM Andy Shevchenko <andy@kernel.org> w=
rote:
>
> (Minimized the list of people for my review / comments)

Thanks! I'll let Kent address your feedback.

>
> On Tue, Oct 24, 2023 at 06:45:58AM -0700, Suren Baghdasaryan wrote:
> > From: Kent Overstreet <kent.overstreet@linux.dev>
> >
> > The new flags parameter allows controlling
> >  - Whether or not the units suffix is separated by a space, for
> >    compatibility with sort -h
> >  - Whether or not to append a B suffix - we're not always printing
> >    bytes.
>
> ...
>
> >       string_get_size(nblocks, queue_logical_block_size(q),
> > -                     STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
> > +                     0, cap_str_10, sizeof(cap_str_10));
>
> This doesn't seem right (even if it works). We shouldn't rely on the
> implementation details.
>
> ...
>
> > -     string_get_size(sdkp->capacity, sector_size,
> > -                     STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
>
> > +     string_get_size(sdkp->capacity, sector_size, 0,
> > +                     cap_str_10, sizeof(cap_str_10));
>
> Neither this.
>
> ...
>
> > -/* Descriptions of the types of units to
> > - * print in */
> > -enum string_size_units {
> > -     STRING_UNITS_10,        /* use powers of 10^3 (standard SI) */
> > -     STRING_UNITS_2,         /* use binary powers of 2^10 */
> > +enum string_size_flags {
>
> So, please add UNITS_10 as it is now. It will help if anybody in the futu=
re
> wants to add, e.g., 8-base numbers.
>
> > +     STRING_SIZE_BASE2       =3D (1 << 0),
> > +     STRING_SIZE_NOSPACE     =3D (1 << 1),
> > +     STRING_SIZE_NOBYTES     =3D (1 << 2),
> >  };
>
> Please, add necessary comments.
>
> ...
>
> > +enum string_size_units {
> > +     STRING_UNITS_10,        /* use powers of 10^3 (standard SI) */
> > +     STRING_UNITS_2,         /* use binary powers of 2^10 */
> > +};
>
> And what a point now in having these?
>
> I assume you need to split this to a few patches:
>
> 1) rename parameter to be a flags without renaming the definitions (this =
will
>    touch only string_helpers part);
> 2) do the end job by renaming it all over the drivers;
> 3) add the other flags one-by-one (each in a separate change);
> 4) use new flags where it's needed;
>
> Also see below.
>
> ...
>
> >       static const char *const units_10[] =3D {
> > -             "B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
> > +             "", "k", "M", "G", "T", "P", "E", "Z", "Y"
> >       };
> >       static const char *const units_2[] =3D {
> > -             "B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "Yi=
B"
> > +             "", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei", "Zi", "Yi"
> >       };
>
> Ouch, instead of leaving this and actually "cutting the letter" with NO* =
flags,
> you did something different.
>
> ...
>
> Now the main part. Since in 50+% cases (I briefly estimated, it may be mo=
re)
> this is used in printf() why not introducing a new pointer extension for =
that?
>
> Yes, it may be done separately, but it will look like a double effort to =
me.
> Instead it might give us a possibility to scale w/o touching users each t=
ime
> we want to do something and at the same time hide this complete API under
> printf() implementation.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>