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 599DC4290B; Mon, 10 Apr 2023 16:12:18 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D643340DDC; Mon, 10 Apr 2023 16:12:17 +0200 (CEST) Received: from forward502c.mail.yandex.net (forward502c.mail.yandex.net [178.154.239.210]) by mails.dpdk.org (Postfix) with ESMTP id 4DF9F40A81 for ; Mon, 10 Apr 2023 16:12:16 +0200 (CEST) Received: from mail-nwsmtp-smtp-production-main-59.iva.yp-c.yandex.net (mail-nwsmtp-smtp-production-main-59.iva.yp-c.yandex.net [IPv6:2a02:6b8:c0c:992a:0:640:c3bc:0]) by forward502c.mail.yandex.net (Yandex) with ESMTP id 835A55EC9A; Mon, 10 Apr 2023 17:12:15 +0300 (MSK) Received: by mail-nwsmtp-smtp-production-main-59.iva.yp-c.yandex.net (smtp/Yandex) with ESMTPSA id BCV251dWs4Y0-DMEzo2w2; Mon, 10 Apr 2023 17:12:14 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1681135934; bh=29ucc8yjq5WhrThQPGanVgsKUTvmSnMYGYRcfW6RNa4=; h=From:In-Reply-To:Cc:Date:References:To:Subject:Message-ID; b=HttvTG9mfRgCSnEjUGgokl2ShAgIePIvW8B4pE0E7bc4tXosbBLaN3EFnygYhhqm7 BlsunWYQpURDS6v8mduz1FkGuG9v9CSNrVqza9SwpGGcE2wGY9eXQjCWk+8ezedH2m x6E9w2ii5FwwvT5i/kp5WUQWx9Gz6bje+6PHRZzw= Authentication-Results: mail-nwsmtp-smtp-production-main-59.iva.yp-c.yandex.net; dkim=pass header.i=@yandex.ru Message-ID: <0ba33168-cb4c-b8c7-1416-560ccb851835@yandex.ru> Date: Mon, 10 Apr 2023 15:12:11 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH 3/9] eal: use barrier intrinsics when compiling with msvc Content-Language: en-US To: Tyler Retzlaff , =?UTF-8?Q?Morten_Br=c3=b8rup?= Cc: Konstantin Ananyev , dev@dpdk.org, bruce.richardson@intel.com, david.marchand@redhat.com, thomas@monjalon.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> <20230405153848.GA31673@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> From: Konstantin Ananyev In-Reply-To: <20230405153848.GA31673@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 05/04/2023 16:38, Tyler Retzlaff пишет: > 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. That's an interesting analogy - never thought about programming as a decease :) Back to the subject, at least to me two clear files with one conditional include is much more sane then one file with bunch of ifdefs inside. >> >> 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. About special rte_common.h for msvc - that sounds like a good option to me. > 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 >