On Mon, Dec 12, 2022 at 12:16:07PM +0000, Bruce Richardson wrote: > On Mon, Dec 12, 2022 at 01:03:52PM +0100, Morten Brørup wrote: > > > From: Bruce Richardson [mailto:bruce.richard...@intel.com] > > > Sent: Monday, 12 December 2022 12.21 > > > > > > 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. > > > > <rant> > > If we wanted to fix this generally, we should rely on type promotion, so > > the existing _int function should be updated to take an int64_t value, and > > the _u64 function should be renamed to _uint (and still take an uint64_t > > value). However, that would break the ABI, and would need to go through > > some process for that. So let's not change this now. > > </rant> > > Yes, not making "int" an "i64" type was a poor design decision on my part > in the earlier versions. Thankfully negative values are rarely needed > beyond the range of 32-bits, but we should probably look to update this as > you suggest at the next ABI break window. > Actually, most of the work for this can be done without affecting ABI, I believe, and for the two functions that would be affected, function versioning could be used to cover those. I think it's better to make the change now using versioning rather than waiting, as it's likely to be forgotten if we wait.
I'll work up a patchset for this so we can review and discuss... /Bruce
