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 C8FCB428D4; Wed, 5 Apr 2023 17:38:50 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B529441153; Wed, 5 Apr 2023 17:38:50 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id D071E41133 for ; Wed, 5 Apr 2023 17:38:48 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 1DFC9210DED6; Wed, 5 Apr 2023 08:38:48 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 1DFC9210DED6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1680709128; bh=l/Et02A+64meAaXZ60k48mdETSNXwtddHsi72HGbP/I=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=AX+TAaFM9Y26HpBEkPyYFvGBbFUgeRTH+0Sh3B9P9cpeL/+KB33e3SO6Yv4IB7evx 60kGVEtheOHp5DOY5IyheatZv6JYqxFxxrS2jYbedImK2gUHtHHz/ywuZ0eFgp0jBZ o7b+1Bn/jwybpOoKARY/7+878VnGK9U5fRc9DGjA= Date: Wed, 5 Apr 2023 08:38:48 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Konstantin Ananyev , Konstantin Ananyev , dev@dpdk.org, bruce.richardson@intel.com, david.marchand@redhat.com, thomas@monjalon.net Subject: Re: [PATCH 3/9] eal: use barrier intrinsics when compiling with msvc Message-ID: <20230405153848.GA31673@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1680558751-17931-1-git-send-email-roretzla@linux.microsoft.com> <1680558751-17931-4-git-send-email-roretzla@linux.microsoft.com> <754a877d020a4a199a5310b469e876a7@huawei.com> <20230404154906.GC23247@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20230405000441.GA24769@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D87858@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87858@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) 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 Wed, Apr 05, 2023 at 02:35:47PM +0200, Morten Brørup wrote: > > From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com] > > Sent: Wednesday, 5 April 2023 12.57 > > > > > > >>Another ore generic comment - do we really need to pollute all that code > > with RTE_TOOLCHAIN_MSVC ifdefs? > > > > >>Right now we have ability to have subdir per arch (x86/arm/etc.). > > > > >>Can we treat x86+windows+msvc as a special arch? > > > > > > > > > >i asked this question previously and confirmed in the technical board > > > > >meeting. the answer i received was that the community did not want new > > > > >directory/headers introduced for compiler support matrix and i should > > > > >use #ifdef in the existing headers. > > > > > > > > Ok, can I then ask at least to minimize number of ifdefs to absolute > > > > minimum? > > > > > > in principal no objection at all, one question though is what to do with > > > comment based documentation attached to macros? e.g. > > > > > > #ifdef SOME_FOO > > > /* some documentation */ > > > #define some_macro > > > #else > > > #define some_macro > > > #endif > > > > > > #ifdef SOME_FOO > > > /* some documentation 2 */ > > > #define some_macro2 > > > #else > > > #define some_macro2 > > > #endif > > > > > > i can either duplicate the documentation for every define so it stays > > > "attached" or i can only document the first expansion. let me know what > > > you expect. > > > > > > so something like this? > > > > > > #ifdef SOME_FOO > > > /* some documentation */ > > > #define some_macro > > > /* some documentation 2 */ > > > #define some_macro2 > > > #else > > > #define some_macro > > > #define some_macro2 > > > #endif > > > > > > or should all documentation be duplicated? which can become a teadious > > > redundancy for anyone maintaining it. keep in mind we might have to make > > > an exception for rte_common.h because it seems doing this would be > > > really ugly there. take a look let me know. > > > > My personal preference would be to keep one documentation block for both cases > > (yes, I suppose it needs to be updated if required): > > > > /* some documentation, probably for both SOME_FOO on/off */ > > #ifdef SOME_FOO > > #define some_macro > > #else > > #define some_macro > > #endif > > > > Or the third option of using a dummy for documentation purposes only. rte_memcpy() does this [1], although it is an inline function and not a macro. > > [1]: https://elixir.bootlin.com/dpdk/v23.03/source/lib/eal/include/generic/rte_memcpy.h#L90 > > No preferences here, just mentioning it! > > > > > > > > > > It is really hard to read an follow acode that is heavily ifdefed. > > Yes, and sometimes it is even harder reading code that is spread across multiple arch-depending files. > > It is a choice between the plague or cholera. > > But it is one of the unavoidable downsides to supporting many architectures and compilers, so we have to seek out the best compromises. yes, there is a conditional that has to be *somewhere*. i would propose for now that it be done per-macro or whatever. but when i get the bulk of the changes in i can commit to refactoring some groups out into their own header. rte_common.h is a good candidate for this because of how many conditionals it will contain. generic/rte_common.h -> msvc/rte_common.h #include "generic/rte_common.h" -> gnu?/rte_common.h #include "generic/rte_common.h" this could carry inline/macro documentation in generic/rte_common.h but again i'd prefer to do this after msvc work is stood up at least to the point of having unit tests working before i start moving code around. for other conditionals i don't think it's worth having a whole file e.g. x86/include/rte_pause.h or something only a couple of blocks are conditional. ty