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 483BB43BC0; Sat, 24 Feb 2024 12:23:07 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BEF3E402C8; Sat, 24 Feb 2024 12:23:06 +0100 (CET) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 68371402A8 for ; Sat, 24 Feb 2024 12:23:05 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 3E04B2090E; Sat, 24 Feb 2024 12:23:05 +0100 (CET) Content-class: urn:content-classes:message Subject: RE: [PATCH v5 00/22] stop using RTE_MARKER extensions MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Date: Sat, 24 Feb 2024 12:23:04 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F266@smartserver.smartshare.dk> In-Reply-To: <3245779.AJdgDx1Vlc@thomas> X-MS-Has-Attach: X-MimeOLE: Produced By Microsoft Exchange V6.5 X-MS-TNEF-Correlator: Thread-Topic: [PATCH v5 00/22] stop using RTE_MARKER extensions Thread-Index: AdpnEol20zIaXxqCROOVIq75MInq6gAAL29w 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> <3245779.AJdgDx1Vlc@thomas> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Thomas Monjalon" , "Tyler Retzlaff" Cc: , "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" 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: Saturday, 24 February 2024 12.14 >=20 > 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. > > > > > > This series, does the following. > > > > > > Introduces a new macro __rte_marker(type, name) which is used to > > > conditionally expand RTE_MARKER fields empty when building with = GCC. > > > > Important typo: GCC -> MSVC. > > > > Correct me if I'm wrong: When building with MSVC, the RTE_MARKER > fields don't exist at all. >=20 > Yes it should trigger a compiler error when using an old marker with > MSVC. > So no need of this wrapper __rte_marker(). >=20 > > > 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. > > > > > > Introduces 2 new inline functions to allow drivers to access > rearm_data > > > and rx_descriptor_fields1 descriptors without using the RTE_MARKER > > > fields. > > > > > > Updates all drivers to use the new inline rte_mbuf struct = accessors > for > > > rearm_data and rx_descriptor_fields1. > > > > Accessor functions like these are BS for structures that are part of > the public API. >=20 > What is BS? >=20 > > Nobody will use the accessor functions! When developing an = application > (or lib or driver), the developer will look at the structure and = access > the relevant fields directly; why would the developer start looking > elsewhere for accessor functions? >=20 > 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 > anymore. >=20 > > Another alternative would be to remove the rearm_data and > rx_descriptor_fields1 fields from the structure, and in the drivers > address the first field of the group, and preferably add some > static_asserts to check the sequence 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. >=20 > Unions make the mbuf struct more complicate just for compatibility. >=20 > > 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, > depending on compiler, and this fact is not obvious when looking at = the > structure. I think this will degrade future MSVC compatibility for = both > applications, libraries and PMDs. >=20 > With appropriate checks, we won't use markers anymore. >=20 > > 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. > > > > Maybe we need to compromise on API compatibility to make this a > beautiful modification. > > > > @Thomas, looking at the mbuf and eal patches, what do you think = about > this version of the series? >=20 > I prefer this series with following changes: > - no __rte_marker wrapper > - make sure cache line padding is effective without markers > - no direct access of fields for cache line prefetch > - comments in mbuf > - checkpatch >=20 Thomas has provided a lot of good feedback for this version of the = series, showing that the path is worth exploring further. In other words: I'm likely to be convinced. ;-)