On Mon, Dec 12, 2022 at 12:02:32PM +0100, Morten Brørup wrote: > > From: Bruce Richardson [mailto:bruce.richard...@intel.com] > > Sent: Monday, 12 December 2022 11.32 > > > > On Mon, Dec 12, 2022 at 02:42:55PM +0800, Huisong Li wrote: > > > Some lib telemetry interfaces add the 'u32' and 'u64' data by the > > > rte_tel_data_add_dict/array_int API. This may cause data conversion > > error > > > or data truncation. > > > > > > The 'u32' data can not be assigned to signed 32-bit integer. However, > > > assigning to u64 is very wasteful, after all, the buffer capacity of > > each > > > transfer is limited. So it is necessary for 'u32' data to add usigned > > > 32-bit integer type and a series of 'u32' operation APIs. > > > > > > This patchset uses the new 'u32' API to resolve the problem of data > > > conversion error, and use the 'u64' API to add 'u64' data. > > > > > > In addition, this patchset introduces two APIs to store u32 and u64 > > > values as hexadecimal encoded strings in telemetry library. > > > > > > --- -v3: fix a misspelling mistake in commit log. -v2: - fix ABI > > break > > > warning. - introduce two APIs to store u32 and u64 values as > > hexadecimal > > > encoded strings. > > > > > I'm not convinced about adding the u32 value generically to the > > telemetry > > lib - except in the case of having explicit function calls for u32 vs > > u64 > > hex strings. Having a u32 type doesn't gain us any space internally > > over a > > u64 value, since all values are in a union type. Also, for output as > > json, > > the numeric values are all output as decimal values, meaning that the > > value > > 1 appears as the same size in the output string whether it is a u32 or > > u64 > > type. Now, it may save space in a future binary output format, but even > > then it still may not do so. > > I agree that a u32 doesn't gain any space internally. > > However, many SNMP counters are unsigned 32 bit, and expected to wrap around > as such. > > So I suppose the u32 type might be useful for SNMP, if obtained through the > telemetry library. > > Alternatively, we could somehow reuse the u64 type and require the > application to pass (value & UINT32_MAX) to the u64 functions. To make this > easy to use, we should add some wrappers to do it for the application. And > eventually we would probably end up with something very similar to this patch. >
I think just using the u64 functions is probably simplest and best right now. If we add support for something like snmp then yes, it would make sense to explicitly add it, but it seems like a lot of extra code for little or no benefit until we support something like that. > > > > Therefore, I'd tend to keep the existing u64 type as-is, and instead > > only > > add the functions for outputting hex values. Those hex output functions > > could take an additional parameter indicating the desired hex output > > length, as there could well be cases where we want just 16-bit hex > > value > > too. > > The way I read the patch series, the hex output length is not fixed, but an > u64 value of 5 will be output as 0x5, not 0x0000000000000005. > > So the only benefit of having both u32_hex and u64_hex functions is to avoid > type promotion from uint32_t to uint64_t on input for 32 bit values. > > Instead of passing a 3rd parameter or adding u16_hex functions, we could > consider just having one set of hex functions using uint64_t for the value, > and rely on type promotion for 32 and 16 bit values. > +1 to having only a single hex function, and I think type promotion should work fine. However, I still think it might be worthwhile allowing the user to pass in a min output length parameter too. I find for many hex strings having the leading zeros to explicitly show the length can be useful. The value "0" could cover the current behaviour of no-padding, otherwise the parameter should indicate the number of bits to be displayed. (If we want to lock it down we can use an enum parameter rather than int to limit it to 0, 8, 16, 32 or 64 bit displayed values). All that said, I'm not massively concerned if we want to just keep the current approach of always just printing without any leading zeros. It's a nice-to-have only for me. /Bruce
