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 5DF4541BB3; Thu, 2 Feb 2023 22:26:52 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EC4D540EDC; Thu, 2 Feb 2023 22:26:51 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 55BDC40693 for ; Thu, 2 Feb 2023 22:26:50 +0100 (CET) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: Sign changes through function signatures Date: Thu, 2 Feb 2023 22:26:48 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D876F8@smartserver.smartshare.dk> In-Reply-To: <5155790.0VBMTVartN@thomas> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: Sign changes through function signatures Thread-Index: Adk3R0IuWztJ4E+vSZCLOJj4C+zXQwAAFdpg References: <20230202202604.GA23284@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <5155790.0VBMTVartN@thomas> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Thomas Monjalon" , "Ben Magistro" , "Tyler Retzlaff" , Cc: "Olivier Matz" , , , , , "Stefan Baranoff" , , 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 > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Thursday, 2 February 2023 21.45 >=20 > 02/02/2023 21:26, Tyler Retzlaff: > > On Thu, Feb 02, 2023 at 02:23:39PM -0500, Ben Magistro wrote: > > > Hello, > > > > > > While making some updates to our code base for 22.11.1 that were > missed in > > > our first pass through, we hit the numa node change[1]. In the > process of > > > updating our code, we noticed that a couple functions > (rx/tx_queue_setup, > > > maybe more that we aren't using) state they accept `SOCKET_ID_ANY` > but the > > > function signature then asks for an unsigned integer while > `SOCKET_ID_ANY` > > > is `-1`. Following it through the redirect to the "real" function > it also > > > asks for an unsigned integer which is then passed on to one or = more > > > functions asking for an integer. As an example using the the i40e > driver > > > -- we would call `rte_eth_tx_queue_setup` [2] which ultimately > calls > > > `i40e_dev_tx_queue_setup`[3] which finally calls > `rte_zmalloc_socket`[4] > > > and `rte_eth_dma_zone_reserve`[5]. > > > > > > I guess what I am looking for is clarification on if this is > intentional or > > > if this is additional cleanup that may need to be completed/be > desirable so > > > that signs are maintained through the call paths and avoid > potentially > > > producing sign-conversion warnings. From the very quick glance I > took at > > > the i40e driver, it seems these are just passed through to other > functions > > > and no direct use/manipulation occurs (at least in the mentioned > functions). > > > > i believe this is just sloppyness with sign in our api surface. i = too > > find it frustrating that use of these api force either explicit > > casts or suffer having to suppress warnings. > > > > in the past examples of this have been cleaned up without full > deprecation > > notices but there are a lot of instances. i also feel (unpopular > opinion) > > that for some integer types like this that have constrained range / > number > > spaces it would be of value to introduce a typedef that can be used > > consistently. > > > > for now you'll just have to add the casts and hopefully in the = future > we > > will fix the api making them unnecessary. of course feel free to > submit > > patches too, it would be great to have these cleaned up. >=20 > I agree it should be cleaned up. > Those IDs should accept negative values. > Not sure which type we should choose (int, int32_t, or a typedef). Why would we use a signed socket ID? We don't use signed port IDs. To = me, unsigned seems the way to go. (A minor detail: With unsigned we can = use the entire range of values minus one (for the magic "any" value), = whereas with signed we can only use the positive range of values. This = detail is completely irrelevant when using 32 bit for socket ID, but = could be relevant if using fewer bits.) Also, we don't need 32 bit for socket ID. 8 or 16 bit should suffice, = like port ID. But reducing from 32 bit would probably cause major ABI = breakage. >=20 > Another thing to check is the name of the variable. > It should be a socket ID when talking about CPU, > and a NUMA node ID when talking about memory. >=20 > And last but not the least, > how can we keep ABI compatibility? > I hope we can use function versioning to avoid deprecation and > breaking. >=20 > Trials and suggestions are welcome. Signedness is not the only problem with the socket ID. The meaning of = SOCKET_ID_ANY is excessively overloaded. If we want to clean this up, we = should consider the need for another magic value SOCKET_ID_NONE for = devices connected to the chipset, as discussed in this other email = thread [1]. And as discussed there, there are also size problems, = because some device structures use 8 bit to hold the socket ID. And functions should always return -1, never SOCKET_ID_ANY, to indicate = error. [1]: = http://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35D87684@smartser= ver.smartshare.dk/ I only bring warnings and complications to the discussion here, no = solutions. Sorry! :-(