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 0709C43BC0; Sat, 24 Feb 2024 12:13:48 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 83F21402C8; Sat, 24 Feb 2024 12:13:47 +0100 (CET) Received: from fhigh8-smtp.messagingengine.com (fhigh8-smtp.messagingengine.com [103.168.172.159]) by mails.dpdk.org (Postfix) with ESMTP id 76759402A8 for ; Sat, 24 Feb 2024 12:13:46 +0100 (CET) Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailfhigh.nyi.internal (Postfix) with ESMTP id ED8BF114009B; Sat, 24 Feb 2024 06:13:45 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Sat, 24 Feb 2024 06:13:45 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm1; t=1708773225; x=1708859625; bh=BI3anVy39MfFiLOaXXhSOYpEB/o+b6oBzersXp1sWQk=; b= jclmY5vGgfgv6nZQOQcYCdgi+oQeFuMT4GZ7PzUyfs+CWNdou+7o/m4yeiYuUnnC YJFFMt+92f4i6x+5p5HvWcFezxMbsyctEIeTXsSNqJZLdXzJzehQOlA7r8Oh8d3M TRFMFnp4Xe1aXaEPcgeaQQRSC9ci4drB7eMfrTaD/1tSmTfPwXmBrckz6C0S5toB gcebyWz+91h1k3r1WCLrxhSkThPLCDeGxVf3HQE712mEVsOPUdETKz9XOk/uDqpJ pCS7pBFVq424164F6MHolzaPfnXtxmkEK2MvyrfUO1Psf0hrLgUpMgNEw74rgk/B sE/j7t3HHzMYSYpGU/rFgw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1708773225; x= 1708859625; bh=BI3anVy39MfFiLOaXXhSOYpEB/o+b6oBzersXp1sWQk=; b=H nlf/pgY+i0cbcQfC6meYG5j142WMAWlLzj+CF8Y2Ipi4VNdKExqClt5BYxtOo8od o9ImHcdBdrG1zjv+zmEILTAw7L4GUZr1HklomVwGtoMQvN9pdeacmpwy/pTxnPD+ ZnyIjuXdYFDO3jbgQMGl5Ce83DNHnz8Zk+RsfDV3gCqboMcAfKTHc0nZgQZvSAHN IzbeHcWEr5Q/TMSaWhPZ81w86dOe44TQ6NONvtksxzjMJ6iAo8Ckjs4PJPlkt6z8 pUEMh1spEddGhdKF5pZWIGRYos2Y9LyFpMDW73So2AKtHBrhs9FnGkkz/UItchnv GfAHDsgaw2dtzXLWnPw+g== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvledrfeekgddvhecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvfevufffkfgjfhgggfgtsehtqhertddttddunecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepfefhjeeluedvvedtuddtuedtvefhieejtefhffeujefhteduudev tdektdeikeffnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sat, 24 Feb 2024 06:13:39 -0500 (EST) From: Thomas Monjalon To: Tyler Retzlaff , Morten =?ISO-8859-1?Q?Br=F8rup?= Cc: dev@dpdk.org, Ajit Khaparde , Andrew Boyer , Andrew Rybchenko , Bruce Richardson , Chenbo Xia , Chengwen Feng , Dariusz Sosnowski , David Christensen , Hyong Youb Kim , Jerin Jacob , Jie Hai , Jingjing Wu , John Daley , Kevin Laatz , Kiran Kumar K , Konstantin Ananyev , Maciej Czekaj , Matan Azrad , Maxime Coquelin , Nithin Dabilpuram , Ori Kam , Ruifeng Wang , Satha Rao , Somnath Kotur , Suanming Mou , Sunil Kumar Kori , Viacheslav Ovsiienko , Yisen Zhuang , Yuying Zhang Subject: Re: [PATCH v5 00/22] stop using RTE_MARKER extensions Date: Sat, 24 Feb 2024 12:13:36 +0100 Message-ID: <3245779.AJdgDx1Vlc@thomas> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F262@smartserver.smartshare.dk> References: <1706657173-26166-1-git-send-email-roretzla@linux.microsoft.com> <1708762927-14126-1-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35E9F262@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 24/02/2024 11:42, Morten Br=F8rup: > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > RTE_MARKER typedefs are a GCC extension unsupported by MSVC. This series > > hides the markers when building with MSVC and updates libraries and > > drivers to access compatibly typed pointers to the rte_mbuf struct > > offsets. > >=20 > > This series, does the following. > >=20 > > Introduces a new macro __rte_marker(type, name) which is used to > > conditionally expand RTE_MARKER fields empty when building with GCC. >=20 > Important typo: GCC -> MSVC. >=20 > Correct me if I'm wrong: When building with MSVC, the RTE_MARKER fields d= on't exist at all. Yes it should trigger a compiler error when using an old marker with MSVC. So no need of this wrapper __rte_marker(). > > Updates existing inline functions accessing cacheline{0,1} markers in > > the rte_mbuf struct to stop using the markers and instead uses the mbuf > > fields directly. > >=20 > > Introduces 2 new inline functions to allow drivers to access rearm_data > > and rx_descriptor_fields1 descriptors without using the RTE_MARKER > > fields. > >=20 > > Updates all drivers to use the new inline rte_mbuf struct accessors for > > rearm_data and rx_descriptor_fields1. >=20 > Accessor functions like these are BS for structures that are part of the = public API. What is BS? > Nobody will use the accessor functions! When developing an application (o= r lib or driver), the developer will look at the structure and access the r= elevant fields directly; why would the developer start looking elsewhere fo= r accessor functions? Yes that's why we need to reference the accessors inside the mbuf structure. Also we should check new code (with checkpatch) for not using markers anymo= re. > Another alternative would be to remove the rearm_data and rx_descriptor_f= ields1 fields from the structure, and in the drivers address the first fiel= d of the group, and preferably add some static_asserts to check the sequenc= e of the fields they cover. I don't like this alternative, but I'm putting = it out there for discussion/inspiration. > I prefer the union+struct approach to visibly group the fields together. Unions make the mbuf struct more complicate just for compatibility. > Overall, I dislike approach taken in this version of the patch series. > On the surface, it has minimal changes to the mbuf structure. > But underneath, some of the fields (the markers) may or may not exist, de= pending on compiler, and this fact is not obvious when looking at the struc= ture. I think this will degrade future MSVC compatibility for both applicat= ions, libraries and PMDs. With appropriate checks, we won't use markers anymore. > As Thomas stressed, we should take special care of the mbuf structure! > It has to be modified for MSVC compatibility, so we have to find the best= compromise. > Personally, I prefer the previous approach over this version. >=20 > Maybe we need to compromise on API compatibility to make this a beautiful= modification. >=20 > @Thomas, looking at the mbuf and eal patches, what do you think about thi= s version of the series? I prefer this series with following changes: =2D no __rte_marker wrapper =2D make sure cache line padding is effective without markers =2D no direct access of fields for cache line prefetch =2D comments in mbuf =2D checkpatch