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 346F1A00C2; Thu, 13 Oct 2022 10:26:44 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 155A242C95; Thu, 13 Oct 2022 10:26:44 +0200 (CEST) Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by mails.dpdk.org (Postfix) with ESMTP id D101042C27; Thu, 13 Oct 2022 10:26:42 +0200 (CEST) Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id 49F2C5C00E1; Thu, 13 Oct 2022 04:26:42 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute4.internal (MEProxy); Thu, 13 Oct 2022 04:26:42 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm3; t=1665649602; x= 1665736002; bh=uN36gr9mn5mJ9VAV8C0NpyuiZsCkelWwbSvuS8ljz8A=; b=A esLoD6mXEK41/5AJmP9oWI9bvQ81cMqXziCymBHCRfWO++MYfpYFmCEpeI+nt8u4 g+0E3YVN6/6pF3CEFWADWTcdbKodYcNiW/jGXCETd3ldyVUkv4UJdNfsOXk+VUkv RBQqCRI5+c2Hrhq3mWkM/RNmSv5lSJW8GZKb/wZu6eg7TR8Sf74VELsVGenuEVg5 MPIUUK7BzcVry8WmksTiiNMQXyLjMPyTD7v/xvjY6Ou60cQUZrtvaBGvmxgZjItn qngpPf2x3R0bY+SSvh//FTDw7q5xST6MjwLkeD51zJrZJiwidF4bQ/NQDtiy9m5x 4pv3vGPUGSiiHNGM+kKog== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm3; t=1665649602; x= 1665736002; bh=uN36gr9mn5mJ9VAV8C0NpyuiZsCkelWwbSvuS8ljz8A=; b=P cpXe5kOIWyzarGnCf87jPBuScOIdzXXD7zsseTWhjHf4zcA5J0wDlpIrh/GzUeqT DVQhm2QtJ9CO4Bft3uLG3N2bZNjqtcjuWKYTBFvrB5QNXAylNeUPwBSRTiZI1uAN mnSZlub8z/ZJOYIxsKNOjbJN/oMABFdvjUwRiJ8RE6xTeVaI3b0arjK/gQj1tFrp Dkvbw8U8cl2OcVCrcuibBnQULCDyBETTmeQNtPSEU2W8TPqAUkgIqEn8F55uqip3 Ns7/vnzilkQ2Q9L7HXcb43eoSnWCBfiXaFYu0Fs+SG8AGHjLSzkPV05dcDrmQth+ GtpLBOjV18iJnINqVWOig== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrfeektddgtdegucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthhqredttddtudenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpeevueegudejtdfhuddvhfdtveetleetkeevhfeuueegfeffffff leeluefhhefgveenucffohhmrghinhepphhrohhofhhpohhinhhtrdgtohhmpdguphgukh drohhrghenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhm pehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 13 Oct 2022 04:26:39 -0400 (EDT) From: Thomas Monjalon To: Morten =?ISO-8859-1?Q?Br=F8rup?= , Jerin Jacob , Sachin Saxena , Hemant Agrawal , Ori Kam , Liron Himi , Pavan Nikhilesh Bhagavatula Cc: "Van Haaren, Harry" , Jerin Jacob Kollanukkaran , "dev@dpdk.org" , "Li, WeiyuanX" , Ferruh Yigit , Andrew Rybchenko , "david.marchand@redhat.com" , techboard@dpdk.org Subject: Re: xstats id type Date: Thu, 13 Oct 2022 10:26:37 +0200 Message-ID: <4855955.TLkxdtWsSY@thomas> In-Reply-To: References: <98CBD80474FA8B44BF855DF32C47DC35D873D4@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D873DA@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" 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 13/10/2022 09:12, Pavan Nikhilesh Bhagavatula: > From: Morten Br=F8rup > > +TO: rawdev maintainers, regexdev maintainers > >=20 > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > Sent: Wednesday, 12 October 2022 22.44 > > > > > > 12/10/2022 18:47, Jerin Jacob: > > > > On Wed, Oct 12, 2022 at 9:58 PM Thomas Monjalon > > > > > wrote: > > > > > > > > > > 12/10/2022 18:16, Jerin Jacob: > > > > > > On Wed, Oct 12, 2022 at 9:05 PM Morten Br=F8rup > > > wrote: > > > > > > > > > > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > > > > > Sent: Wednesday, 12 October 2022 17.13 > > > > > > > > > > > > > > > > 12/10/2022 14:14, Van Haaren, Harry: > > > > > > > > > From: Morten Br=F8rup > > > > > > > > > > From: Van Haaren, Harry > > > [mailto:harry.van.haaren@intel.com] > > > > > > > > > > > From: Jerin Jacob > > > > > > > > > > > > On Wed, Oct 12, 2022 at 1:40 PM Morten Br=F8rup wro= te: > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Jerin (eventdev maintainer), > > > > > > > > > > > > > > > > > > > > > > > > + harry.van.haaren@intel.com as the changes in > > > > > > > > drivers/event/sw. > > > > > > > > > > > > > > > > > > > > > > Thanks Jerin. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > While looking into bug #1101 [1], I noticed a mix > > > of unsigned > > > > > > > > int > > > > > > > > > > > and uint32_t in > > > > > > > > > > > > the test code, which will fail on 64-bit big endian > > > CPUs. > > > > > > > > > > > > > > > > > > > > > > Aha; that we can fix. I am curious why this isn't fou= nd > > > in > > > > > > > > CI/reported > > > > > > > > > > > before. > > > > > > > > > > > > > > > > > > > > We probably don't test any 64-bit *big endian* > > > architectures. Just > > > > > > > > a guess. > > > > > > > > > > > > > > > > > > Seems so yes. > > > > > > > > > > > > > > > > > > > > > > Specifically, rte_event_dev_xstats_reset() is > > > called with the > > > > > > > > "ids" > > > > > > > > > > > parameter > > > > > > > > > > > > pointing to an unsigned int [2], but that parameter > > > is a > > > > > > > > pointer to > > > > > > > > > > > an uint32_t. > > > > > > > > > > > > > > > > > > > > > > > > > > I think the type of the ids array parameter to > > > > > > > > > > > rte_event_dev_xstats_reset() should > > > > > > > > > > > > be changed to unsigned int array, like in the other > > > > > > > > > > > rte_event_dev_xxx() functions. > > > > > > > > > > > > > > > > > > > > > > In this case, we have the option to change the type of > > > a variable > > > > > > > > in a > > > > > > > > > > > test-case, or change API and cause API/ABI breakage. > > > > > > > > > > > > > > > > > > > > Well.. yes, but I would phrase that last option: Change > > > the > > > > > > > > API/ABI, so related > > > > > > > > > > functions consistently use the same type for the same > > > variable, > > > > > > > > instead of randomly > > > > > > > > > > mixing uint64_t, uint32_t and unsigned int, depending on > > > function. > > > > > > > > > > > > > > > > > > Aah ok; I see your point now; there is inconsistent usage > > > of > > > > > > > > uint32_t/unsigned int > > > > > > > > > between the Eventdev APIs itself. Agree this is sub- > > > optimal, and > > > > > > > > would have been > > > > > > > > > nice to have spotted before the Eventdev API was > > > stabilized. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Unfortunately, these functions are not marked > > > experimental, so > > > > > > > > breaking API/ABI is > > > > > > > > > > hard to do. :-( > > > > > > > > > > > > > > > > > > Agreed again. > > > > > > > > > > > > > > > > 22.11 is a breaking release, > > > > > > > > and changing type in the API is not much impactful, > > > > > > > > so that's something you can change now, > > > > > > > > or be quiet forever :) > > > > > > > > > > > > > > Question: > > > > > > > 1. Only change the "xstats id" type in the one eventdev > > > function, which deviates from other eventdev functions, or > > > > > > > 2. Change the "xstats id" type for all xstats functions across > > > all device types, for consistency across device types? > > > > > > > > > > > > > > If 2, then what would be a good type? > > > > > > > > > > > > +1 for second option and the type as uint32_t > > > > > > > > > > > > > > > > > > > > Ethdev uses uint64_t for xstats id, and (speaking without > > > knowledge about its internals) that seems like overkill to me. Arrays > > > of these are being used, so size does matter. > > > > > > > > > > uint64_t is not overkill if you consider having stats per queue > > > with a predictable scheme. > > > > > That's an improvement I would like to work on, > > > > > > > > You mean to use a bitmask hence uint64_t. > > > > Currently it is mapped as arrays so 2^64 stats may not be needed. > > > > > > > > No strong opinion, I was just curious to understand "stats per queue > > > > with a predictable scheme" and how uint64_t helps with that. > > > > > > Yes I mean some bits are used for the queue number. > > > Something like in slide 11 of this presentation: > > > https://urldefense.proofpoint.com/v2/url?u=3Dhttp- > > 3A__fast.dpdk.org_events_slides_DPDK-2D2019-2D09-2DEthernet- > > 5FStatistics.pdf&d=3DDwIFAw&c=3DnKjWec2b6R0mOyPaz7xtfQ&r=3D1cjuAHrGh74 > > 5jHNmj2fD85sUMIJ2IPIDsIJzo6FN6Z0&m=3DApdcbroZzSNlcY1t4c8iv9HZk6YSJOA > > Hpg93zuyIEEWa6xkViBTdoCA3iir_FCtW&s=3DwEMA0lnyrTmxmmDINhzOagGvV > > Z3TcIrzfK5NbJHafdM&e=3D > >=20 > > With this presentation in mind, I strongly agree with Thomas that uint6= 4_t is > > the best choice of type for xstats id. > >=20 > > A quick search shows that both eventdev and rawdev use "unsigned int", > > except rte_event_dev_xstats_reset() and rte_rawdev_xstats_reset(), which > > both use uint32_t. And regexdev uses uint16_t. Other device APIs don't = have > > xstats. >=20 > Harry,=20 > Are you working on a patch for this change? If not I will do it. >=20 > Thomas, > Are you ok to break the ABI without deprication notice i.e. make ID as u6= 4 for eventdev? Yes, it is only increasing size of function parameters, right? The only problematic part is that the application must pass a pointer of the right size, meaning some application code change. It would be an exception in the process, so I am Cc'ing the techboard for more opinions.