From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D5AF2A0032; Mon, 12 Dec 2022 13:16:27 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7DE064021D; Mon, 12 Dec 2022 13:16:27 +0100 (CET) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mails.dpdk.org (Postfix) with ESMTP id D216D40151 for ; Mon, 12 Dec 2022 13:16:25 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1670847386; x=1702383386; h=date:from:to:cc:subject:message-id:references: content-transfer-encoding:in-reply-to:mime-version; bh=/KlIvcsZ7pA10qTc+Ipp5I5oic26vawOJK5YsVuTeg4=; b=A5Nq8ExKII1ZLb4H/o4K7dHuLe/bwJ2taHp4dTPA0T03RKShuk0piHfs RXyYokodyXiTfGk2CFgGpzC/Y0fcq4FuOcUnzpm7SDdQ7Rj3FvvEvmRrJ erWcQBJHL8EipR1ugZyVk6u9xdVwkI6mYPR4Za9DOQdQ4j1igCJ0txd1c +e2Ht1fZkJ0Rwi95o0eZdBawy4EWlMSkM2rsfgTrBswyRXk6cW+YY0tf/ Wq41GoyZhgnokzlDuEKCIxB1KjieeJl4dc7Pd8UVF0T2aPKb/gKGdPnA4 WZNt+9iJHCdEIDp7oooz+xjYD5+itMErvNZ7Myl+EttsFSQkytl5BFh58 Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10558"; a="404093855" X-IronPort-AV: E=Sophos;i="5.96,238,1665471600"; d="scan'208";a="404093855" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Dec 2022 04:16:24 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10558"; a="790472661" X-IronPort-AV: E=Sophos;i="5.96,238,1665471600"; d="scan'208";a="790472661" Received: from orsmsx602.amr.corp.intel.com ([10.22.229.15]) by fmsmga001.fm.intel.com with ESMTP; 12 Dec 2022 04:16:24 -0800 Received: from orsmsx601.amr.corp.intel.com (10.22.229.14) by ORSMSX602.amr.corp.intel.com (10.22.229.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16; Mon, 12 Dec 2022 04:16:23 -0800 Received: from orsedg603.ED.cps.intel.com (10.7.248.4) by orsmsx601.amr.corp.intel.com (10.22.229.14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16 via Frontend Transport; Mon, 12 Dec 2022 04:16:23 -0800 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (104.47.55.102) by edgegateway.intel.com (134.134.137.100) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.16; Mon, 12 Dec 2022 04:16:23 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FaCP7XTvnklgq7lDLtBJcmwlAfkQV6oH/d1fLysgP0kyREgYwPWRJilFPnqpJ3GSvNjXNzLJCRH/rK992FEasXippfBlKzBY5he7pWpq7F5MeR3DKDNB0vcdGmgk1hCFJeCHbgf/ckrIkf7ZHNuiGgCx3e+hf+tFpVFPyfiEV4vAICzBdaxv3feFp+jXPPPBqNHN3AVMU6J9rqqeMTcGTufy9ga/GVDD/Y3LSe5tiy6wsSuYFp9aXXK5+GY1p1JDaIuF+qp+SrviVwFfo8Y859EVv9kgAp91RFoVYggE+la26X/u13qN5FAR6gPRMm30PVLAzx7C/pbQI8JIcr2d9A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=l7eWj2U4rNhPtFv5xv1u/fVCkdfwFjAMPj38yyYYVI0=; b=iIPO13hQju/a7D8X70WAGbrijDSNBKIqpwQD34ZWsrQD4AiCY9L0DoKaDnNBRoLjHZPBS/YxaP0tQIKY/Ye/4nYNVQQyXug5oQlE0/ijPESTH6EQ19hEBJTRNYSoS+Vhd0a6WnaNnzW4KMAW2IoxxanWi2tC37Y1leWdk4RC608zMOcTMyDIMTctgQLYxpx/PXUJ2kp4jcgOI9Tlr0JvARLVA9eNEyvR1AhgQREsaC3CqDJJZDHHCy2hKPyEz3HyTYaTzjNW6xwAgMfhRfJjruAyAvBjaERBKeYgklDFgIDXlQ1EhwSsmzGcK/ntSzD8q9wUE+e3EPlj3Q7icZbz6Q== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; Received: from MWHPR11MB1629.namprd11.prod.outlook.com (2603:10b6:301:d::21) by SJ0PR11MB5680.namprd11.prod.outlook.com (2603:10b6:a03:305::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5880.19; Mon, 12 Dec 2022 12:16:16 +0000 Received: from MWHPR11MB1629.namprd11.prod.outlook.com ([fe80::18bd:edae:ad31:a228]) by MWHPR11MB1629.namprd11.prod.outlook.com ([fe80::18bd:edae:ad31:a228%3]) with mapi id 15.20.5880.019; Mon, 12 Dec 2022 12:16:16 +0000 Date: Mon, 12 Dec 2022 12:16:07 +0000 From: Bruce Richardson To: Morten =?iso-8859-1?Q?Br=F8rup?= CC: Huisong Li , , , , , Subject: Re: [PATCH V3 00/11] telemetry: add u32 value type and hex integer string API Message-ID: References: <20221208080540.62913-1-lihuisong@huawei.com> <20221212064306.39232-1-lihuisong@huawei.com> <98CBD80474FA8B44BF855DF32C47DC35D87585@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D87586@smartserver.smartshare.dk> Content-Type: text/plain; charset="iso-8859-1" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87586@smartserver.smartshare.dk> X-ClientProxiedBy: LO2P265CA0438.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:e::18) To MWHPR11MB1629.namprd11.prod.outlook.com (2603:10b6:301:d::21) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: MWHPR11MB1629:EE_|SJ0PR11MB5680:EE_ X-MS-Office365-Filtering-Correlation-Id: 3449ddf8-465c-4947-0086-08dadc3aaa18 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: ruazHvz1GTJvPbjW7j8Pb4TYxokf4nrypI82riPZXXlXmAj1/DgE38GYQLgOcOYTh1BAbeKLC7qpYykUlIMdIQlEWHFHuTp6KP0wxRpZWAekyFk8ymR8Fhfu7zKfYM7hGk7ZhW+DR+RKhAXsKVitGS2CfCWqpDHDB8PFIiW5LbxurZruDjl3cnrIQ0B+kJwO4x5TRUl6q5qhLCsKJUacbfUvDHXOtE+/fajyPlS4C6tnymr2DxMZL1Q8IGN5zm3wlJgYT1imYbuUmbTnsbB87Uz8i7Tj9JstgQ8C2XUewHHj+1r1MvcFsn9qIGSjH4F43oEyygwrUorFEb3fjiEN+uWLDmA5Lq2xslMPr1u1SYloSFY7AGMp6ezPIEF6hPRA4HPHkBC85SRNqzxIFXklcxLvzcHMjcpJz4QCP6rUqhLUBlhrYoYYISZiXjzcqTu6FlO8KnWFa47CJAU+sjAkd4WXMwzlG2fqgScV5iSa5hM2H+jenThWcy8Jl6fJRnmUpAIUfRArtDOwi8rGpL3FFnOwEIBnkFQZdqbENgiJttDhMGzhwqLj5s0tdScSYEtWw2bUlO2dyBogMli6bnOUz4mrr/dULy2o4AFam5O52bxxWlWlWpjT+2OFWkZI/ahkfj+i7FdxZZvf8C/N7u+9GvlAGlhWozEcUumK88+d5mj34RazkoppHQydDnvMUjwW X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:MWHPR11MB1629.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230022)(136003)(396003)(376002)(346002)(39860400002)(366004)(451199015)(82960400001)(38100700002)(86362001)(41300700001)(6486002)(478600001)(6666004)(8936002)(66556008)(66946007)(66476007)(8676002)(4326008)(6916009)(83380400001)(316002)(2906002)(66574015)(186003)(6512007)(26005)(6506007)(44832011)(5660300002)(67856001); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?Mij6JJVrsLIZqbeP/sL/7HNpkpkCCwjLQd+G1UxEFU50b5Wb3JlpUHx53t?= =?iso-8859-1?Q?N8krR+0I32P/2qOX4UsljZdBOrBX0vEuZOYt6x1Luo9EuPWNhqcNr4AJVb?= =?iso-8859-1?Q?r4U8li2mxPJwqcHe2e0V7s5IBmwrXLJOdh6PVRqhMVANvd+nJHnfisl5kh?= =?iso-8859-1?Q?4grQbaqNOR7cVdDTz/JbmgvpKdXI1N6xVOVE8RI7FbzXkjQlU8GJURb0HW?= =?iso-8859-1?Q?vCszs8H36uxGRfp7x6p0iBoqALjX/XR4TE6Z5EKwcRMNv8wdgqQjqC5S8j?= =?iso-8859-1?Q?CLwgf5U+mTjv+MfcmVS2WTQ2cb9skQ0xtf8JFhkRafZ3ALg7a/cTA1oBDN?= =?iso-8859-1?Q?m/KkMgINGkpWoc0nheUsO9HozEDVrSb63LbkUjxWjp5VwUYkALBZsYWcqu?= =?iso-8859-1?Q?lxVfR25pLZacCjYNn2eCpjeDtc4lSEVGbJwsL19wWsIYEYa1ZSe/NwdmMI?= =?iso-8859-1?Q?QhDGaQktXwnLlMxQAaaqcmfdOLBzf5m0T9tESTLIOnEFBHFozMj50/hPYA?= =?iso-8859-1?Q?fjjAKtrjIiteMQfWCZ5gTTBASqly+GbDET0j3Hlq6B2Mg2NjFeJ8Iinio5?= =?iso-8859-1?Q?kbSp1w/oLwJdLP8ulh9/dvaUG+MKBPZ76gZ/cN46G/oGLSrcKvGyy9rnM4?= =?iso-8859-1?Q?YmM6Op/9V6FZDwi+d1mscEe5VUcxd+NRAYyxzyMF/X+fRSmdrT+UmoM2LO?= =?iso-8859-1?Q?1w8ZICRAICIAGRy44D/UMSa74sIqMDOwtmc3w0l37n6qyZFPzKDhDd4yjD?= =?iso-8859-1?Q?VyT3Zp4wcReEU1P3DfhhA5NdOXnteh+3+16hk0ONtPrtak2aRD44K5Xy8k?= =?iso-8859-1?Q?CZIw4+jPpk8ZMaF7blBsmWb7woFfuAxnnD4OCECzz2RAvp77oq/8wls56l?= =?iso-8859-1?Q?NISb8vqGErwMtzcAD8OPs7MvGPLYvlYsNAqvlF5h5nzP/eblYWqtpGDPHo?= =?iso-8859-1?Q?0bBTjLBpL+F9Nh9fP4RshbiH295+kNpTiq+pMLK3ME0eASNxJsqpVJA6/O?= =?iso-8859-1?Q?jDu1trlRiuDcWR+qFIwaHhEjvKbDfpl4p5MOry2jg/w1rlZR5v272Jj60+?= =?iso-8859-1?Q?x+ye/Xuv+uMdpkvfa5ZAsxJntR+2RZCWAS/2QJ5MIrfcfGFIbV/8iHJ4SF?= =?iso-8859-1?Q?ve7T9oHfKAPqrVuwrxh7bP6UwNKNBZjQ8lzOwLfDhiffptXLF+2AcNxwKG?= =?iso-8859-1?Q?xQTCeBEqu9yqtl+llk+O6OmdR3JP4zq18YRqKpdyXUoHMmzjmBHhPTHS0Y?= =?iso-8859-1?Q?WNTeD6kaUqq5vfKzLITLAZ4MdlgWeLorSwFzefRpbIPykbBQWkDEdqF9Ms?= =?iso-8859-1?Q?BfqJ+/RVjJX5YkGvC0Fx3uO2Z/uwFxXB4AmVMyLimhn9/q5ArudyWJiqVM?= =?iso-8859-1?Q?1TKVFxa/e9zRO5sB8YrtlpzTVDgVyrF60WVA+oKTKg9mqBebaSJdOVvRGt?= =?iso-8859-1?Q?el21qBw1kP6cKjk582JG+xvk1QFCEDonA+DbuxL0H8fFow8ngVs9Zxa4KM?= =?iso-8859-1?Q?7D2T2GzOiptMHGTdNZKU1tVj5RPPOI+7Df0jNHMCuBzwPVrZlaCMW1UiJZ?= =?iso-8859-1?Q?J2owCLK9RJwtFA68EgMoUie8jmD/t0AdTgCcxGFl4ICe0ktfiBWJn+eK3u?= =?iso-8859-1?Q?3yKigCo2rIxC2HjXYPWNxFj0J19u5BkBMqGjoJUV2V7hv8ZUETU1iMpA?= =?iso-8859-1?Q?=3D=3D?= X-MS-Exchange-CrossTenant-Network-Message-Id: 3449ddf8-465c-4947-0086-08dadc3aaa18 X-MS-Exchange-CrossTenant-AuthSource: MWHPR11MB1629.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2022 12:16:16.4539 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: dJ8EZGMUqHu4enY5Dn5aLIlJQ6HhLA8PJUAYruA0s05W322pAeydPrhFcvJVuOCVkHKBgU6lK94rnVZ5YL68VeRnxy6YeaYY6FbwMlsyj7k= X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5680 X-OriginatorOrg: intel.com X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Mon, Dec 12, 2022 at 01:03:52PM +0100, Morten Brørup wrote: > > From: Bruce Richardson [mailto:bruce.richardson@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.richardson@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. > > > 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. > 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. > > I tend to agree with Bruce on this: Let's get rid of the new u32 functions, and rely on the u64 functions for that instead. > > > > > > > > > > > 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). > > An extra parameter for minimum output length (in number of input bits) would be very nice, and makes avoids a set of functions for each bit width. > > (I don't think it should be lock it down to some special bit lengths; there is no need to prevent 24 bit or other exotic bit widths.) > > Something like this: > > char str[64]; // Big enough length. > if (bits != 0) { > char format[16]; // Big enough length. > sprintf(format, "0x0%u%" PRIx64, (bits + 3) / 4); > sprintf(str, format, value); > } else { > sprintf(str, "0x%" PRIx64, value); > } > LGTM.