Re: [PATCH v3 1/3] eal: add function rte_size_to_str

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2025-03-13 10:09, Bruce Richardson wrote:
> On Wed, Mar 12, 2025 at 12:28:32PM -0700, Andre Muezerie wrote:
>> It's common to use %' in the printf format specifier to make large numbers
>> more easily readable by having the thousands grouped. However, this
>> grouping does not work on Windows. Therefore, a function is needed to make
>> uint64_t numbers more easily readable. There are at least two tests that
>> can benefit from this new function.
>>
>> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
>> ---
> 
> Thanks Andre, comments inline below.
> 
> /Bruce
> 
>>   lib/eal/common/eal_common_string_fns.c | 44 ++++++++++++++++++++++++++
>>   lib/eal/include/rte_common.h           | 31 ++++++++++++++++++
>>   lib/eal/version.map                    |  3 ++
>>   3 files changed, 78 insertions(+)
>>
>> diff --git a/lib/eal/common/eal_common_string_fns.c b/lib/eal/common/eal_common_string_fns.c
>> index 9ca2045b18..4cc7f35652 100644
>> --- a/lib/eal/common/eal_common_string_fns.c
>> +++ b/lib/eal/common/eal_common_string_fns.c
>> @@ -4,6 +4,7 @@
>>   
>>   #include <ctype.h>
>>   #include <errno.h>
>> +#include <inttypes.h>
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   
>> @@ -87,6 +88,12 @@ rte_str_to_size(const char *str)
>>   		endptr++; /* allow 1 space gap */
>>   
>>   	switch (*endptr) {
>> +	case 'E': case 'e':
>> +		size *= 1024; /* fall-through */
>> +	case 'P': case 'p':
>> +		size *= 1024; /* fall-through */
>> +	case 'T': case 't':
>> +		size *= 1024; /* fall-through */
>>   	case 'G': case 'g':
>>   		size *= 1024; /* fall-through */
>>   	case 'M': case 'm':
>> @@ -98,3 +105,40 @@ rte_str_to_size(const char *str)
>>   	}
>>   	return size;
>>   }
>> +
>> +int
>> +rte_size_to_str(char *buf, int buf_size,
>> +		uint64_t count, bool use_iec)
> 
> Minor nit, I don't think you should need to wrap this, we can have lines up
> to 100 chars long.
> 
>> +{
>> +	const char *prefix = "kMGTPE";
> 
> Why is "k" in lower case compared to the others all in upper-case?

That's how International Bureau of Weights and Measures likes it, 
although they keep them a factor 1000 and not 1024 apart.

https://www.bipm.org/documents/20126/41483022/SI-Brochure-9-EN.pdf

Page 143, table 7.

> Also, these are suffixes not prefixes. :-)
> 
>> +	const unsigned int base = use_iec ? 1024 : 1000;
>> +	uint64_t powi = 1;
>> +	uint16_t powj = 1;
>> +	uint8_t precision = 2;
>> +
>> +	if (count < base)
>> +		return snprintf(buf, buf_size, "%"PRIu64" ", count);
>> +
>> +	/* increase value by a factor of 1000/1024 and store
>> +	 * if result is something a human can read
>> +	 */
>> +	for (;;) {
>> +		powi *= base;
>> +		if (count / base < powi)
> 
> This would seem more logical to me as "count / powi < base" since it would
> match the initial check for "count < base" (which is essentially the same
> check since powi == 1).
> 
>> +			break;
>> +
>> +		if (!prefix[1])
> 
> Since prefix is character string, the comparison should be against '\0',
> according to DPDK coding style. The "!" should only be used on bool values.
> So "if (prefix[1] == '\0')" or "if (*(prefix + 1) == '\0')".
> 
>> +			break;
>> +		++prefix;
>> +	}
>> +
>> +	/* try to guess a good number of digits for precision */
>> +	for (; precision > 0; precision--) {
>> +		powj *= 10;
>> +		if (count / powi < powj)
>> +			break;
>> +	}
>> +
>> +	return snprintf(buf, buf_size, "%.*f %c%s", precision,
>> +			(double)count / powi, *prefix, use_iec ? "i" : "");
>> +}
>> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
>> index 386f11ae40..781c56adcd 100644
>> --- a/lib/eal/include/rte_common.h
>> +++ b/lib/eal/include/rte_common.h
>> @@ -14,9 +14,11 @@
>>   
>>   #include <assert.h>
>>   #include <limits.h>
>> +#include <stdbool.h>
>>   #include <stdint.h>
>>   #include <stdalign.h>
>>   
>> +#include <rte_compat.h>
>>   #include <rte_config.h>
>>   
>>   /* OS specific include */
>> @@ -919,6 +921,35 @@ __extension__ typedef uint64_t RTE_MARKER64[0];
>>   uint64_t
>>   rte_str_to_size(const char *str);
>>   
>> +/**
>> + * Converts the uint64_t value provided to a human-readable string.
>> + * It null-terminates the string, truncating the data if needed.
>> + *
>> + * Sample outputs with "use_iec" disabled and enabled:
>> + * 0 : "0 ", "0 "
>> + * 700 : "700 ", "700 "
>> + * 1000 : "1.00 k", "1000 "
>> + * 1024 : "1.02 k", "1.00 ki"
>> + * 21474836480 : "21.5 G", "20.0 Gi"
>> + * 109951162777600 : "110 T", "100 Ti"
>> + *
> 
> I would omit the space before the suffixes in the output. As well as
> looking better to me, it also solves the issue of the non-suffixed numbers
> having a trailing space.
> 
>> + * @param buf
>> + *     Buffer to write the string to.
>> + * @param buf_size
>> + *     Size of the buffer.
>> + * @param count
>> + *     Number to convert.
>> + * @param use_iec
>> + *     If true, use IEC units (1024-based), otherwise use SI units (1000-based).
>> + * @return
>> + *     Number of characters written (not including the null-terminator),
>> + *     or that would have been required when the buffer is too small.
>> + */
>> +__rte_experimental
>> +int
>> +rte_size_to_str(char *buf, int buf_size,
>> +		uint64_t count, bool use_iec);
> 
> Again, no need to wrap.
> 
>> +
>>   /**
>>    * Function to terminate the application immediately, printing an error
>>    * message and returning the exit_code back to the shell.
>> diff --git a/lib/eal/version.map b/lib/eal/version.map
>> index a20c713eb1..01b6a7c190 100644
>> --- a/lib/eal/version.map
>> +++ b/lib/eal/version.map
>> @@ -398,6 +398,9 @@ EXPERIMENTAL {
>>   	# added in 24.11
>>   	rte_bitset_to_str;
>>   	rte_lcore_var_alloc;
>> +
>> +	# added in 25.07
>> +	rte_size_to_str;
>>   };
>>   
>>   INTERNAL {
>> -- 
>> 2.48.1.vfs.0.1
>>