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

["Luck, Tony" <tony.luck@intel.com>]

Hi Dave,

On Tue, Nov 11, 2025 at 05:31:12PM +0000, Dave Martin wrote:
> 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).

Earlier versions of the patch had the precomputed value. Reinette
pointed me to int_pow(). It is in lib/math/int_pow.c and does seem
to be pretty efficient.

> 
> (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);

The buf[] was only there for trimming the trailing zeroes. Now that is
gone the result can be sent directly to seq_printf() as you suggest.
> 
> ...?
> 
> 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

-Tony