Re: [PATCH v13 12/32] x86,fs/resctrl: Support binary fixed point event counters

Dave Martin
2025-11-11T17:31:17Z

Hi,

On Wed, Nov 05, 2025 at 06:27:48PM -0800, Luck, Tony wrote:
> On Wed, Nov 05, 2025 at 03:31:07PM -0800, Luck, Tony wrote:
> > > > +
> > > > + if (!binary_bits) {
> > > > + seq_printf(m, "%llu.0\n", val);
> > > > + return;
> > > > + }
>
> I can't completely escape a test for !binary_bits. Most of the
> flow works ok (doing nothing, working towards frac == 0 when
> it comes time for the snprintf()).
>
> But the round-up code:
>
> frac += 1ull << (binary_bits - 1);
>
> goes badly wrong if binary_bits == 0.
>
> I could write it like this:
>
>
> static void print_event_value(struct seq_file *m, unsigned int binary_bits, u64 val)
> {
> char buf[decplaces[MAX_BINARY_BITS] + 1];
> unsigned long long frac = 0;
>
> if (binary_bits) {
> /* Mask off the integer part of the fixed-point value. */
> frac = val & GENMASK_ULL(binary_bits - 1, 0);
>
> /*
> * Multiply by 10^{desired decimal places}. The integer part of
> * the fixed point value is now almost what is needed.
> */
> frac *= int_pow(10ull, decplaces[binary_bits]);

I guess there was already a discussion on whether it is worth
precomputing this multiplier.

int_pow() is not free, but if implemented in the standard way, it
should be pretty fast on 64-bit arches (which is all we care about).

(I've not checked.)

> /*
> * Round to nearest by adding a value that would be a "1" in the
> * binary_bits + 1 place. Integer part of fixed point value is
> * now the needed value.
> */
> frac += 1ull << (binary_bits - 1);
>
> /*
> * Extract the integer part of the value. This is the decimal
> * representation of the original fixed-point fractional value.
> */
> frac >>= binary_bits;

Looks reasonable. It's your call whether this is simpler, I guess.

> }
>
> /*
> * "frac" is now in the range [0 .. 10^decplaces). I.e. string
> * representation will fit into chosen number of decimal places.
> */
> snprintf(buf, sizeof(buf), "%0*llu", decplaces[binary_bits], frac);
>
> seq_printf(m, "%llu.%s\n", val >> binary_bits, buf);

Can we get rid of buf, actually?

I don't see why we can't just do

seq_printf(m, "%llu.%0*llu",
val >> binary_bits, decplaces[binary_bits], frac);

...?

This avoids having to care about the size of buf.

seq_file's crystal ball knows how to make its buffer large enough.

Cheers
---Dave