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 BDDFB43101; Fri, 25 Aug 2023 17:44:49 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 956E140A89; Fri, 25 Aug 2023 17:44:49 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id B59B440A7A for ; Fri, 25 Aug 2023 17:44:47 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id B88412127C93; Fri, 25 Aug 2023 08:44:46 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com B88412127C93 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1692978286; bh=MOy54iSK7GPvNkDNCkogG25MlknA16DVYOozUwBprNE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ktWHdFHZT+1uxLmNjwbUmTLMICz1I77TpyqU97grbe177iEn1UOfN6bpFiCjKPLkj uHTz20RyTwuU3+YPadrFxW4sQPSfZ9s5pzC45eK3QstV4SmLQKx8whUbSJQ/VSqwr8 YZvmQXaVDnoYtmle6KlYNhA3/LvHE01hiq296E/c= Date: Fri, 25 Aug 2023 08:44:46 -0700 From: Tyler Retzlaff To: Bruce Richardson Cc: Morten =?iso-8859-1?Q?Br=F8rup?= , dev@dpdk.org, Konstantin Ananyev , Ciara Power , thomas@monjalon.net, david.marchand@redhat.com, david.hunt@intel.com Subject: Re: [PATCH v11 05/16] eal: use umonitor umwait and tpause intrinsics Message-ID: <20230825154446.GA15412@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1680558751-17931-1-git-send-email-roretzla@linux.microsoft.com> <1691781658-32520-1-git-send-email-roretzla@linux.microsoft.com> <1691781658-32520-6-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35D87B41@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: 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 Fri, Aug 25, 2023 at 04:12:59PM +0100, Bruce Richardson wrote: > On Fri, Aug 25, 2023 at 04:56:51PM +0200, Morten Brørup wrote: > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > > > Sent: Friday, 25 August 2023 16.13 > > > > > > On Fri, Aug 11, 2023 at 12:20:47PM -0700, Tyler Retzlaff wrote: > > > > Inline assembly is not supported for MSVC x64 instead use _umonitor, > > > > _umwait and _tpause intrinsics. > > > > > > > > Signed-off-by: Tyler Retzlaff > > > > Acked-by: Morten Brørup > > > > Acked-by: Konstantin Ananyev > > > > --- > > > > lib/eal/x86/rte_power_intrinsics.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/lib/eal/x86/rte_power_intrinsics.c > > > b/lib/eal/x86/rte_power_intrinsics.c > > > > index f749da9..4066d13 100644 > > > > --- a/lib/eal/x86/rte_power_intrinsics.c > > > > +++ b/lib/eal/x86/rte_power_intrinsics.c > > > > @@ -109,9 +109,13 @@ > > > > */ > > > > > > > > /* set address for UMONITOR */ > > > > +#if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__) > > > > + _umonitor(pmc->addr); > > > > +#else > > > > > > This change is unfortunately giving build errors on system with WAITPKG, > > > since the intrinsics do not take volatile parameters, unlike the inline > > > ASM > > > which works fine with both volatile and non-volatile variables. This is > > > the > > > error I see: > > > > > > ../lib/eal/x86/rte_power_intrinsics.c: In function 'rte_power_monitor': > > > ../lib/eal/x86/rte_power_intrinsics.c:113:22: error: passing argument 1 > > > of '_umonitor' discards 'volatile' qualifier from pointer target type [- > > > Werror=discarded-qualifiers] > > > 113 | _umonitor(pmc->addr); > > > | ~~~^~~~~~ > > > > > > The easy fix for now seems to be just dropping the "|| > > > defined(__WAITPKG__)" part of the #ifdef, and leave the intrinsic for > > > MSVC > > > only. > > > Any objections? > > > > I wonder if omitting the "volatile" qualifier is correct for this parameter. Although the authors of _umonitor() apparently think so, I have seen built-ins with questionable qualifiers before, so I wouldn't trust it to be correct. omitting the volatile qualifier is okay. often the reason why parameters are qualified is as a convenience so that you may pass a volatile qualified argument (and not force removal of the qualification). > > > > Replacing inline assembly with built-ins is generally preferable. So I would prefer if you typecast it away, or temporarily disable that warning. > > > Simple typecasting doesn't work to remove the warning. However, if we > typecast via "uintptr_t" it does seem to work. yes, this is canonical way to remove the qualification. > > #if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__) > - _umonitor(pmc->addr); > + uintptr_t addr_val = (uintptr_t)pmc->addr; > + _umonitor((void *)addr_val); > #else > > Seems bit clunky to me. If we want a one-line workaround, we can probably > use RTE_PTR_ADD to do the typecasting via uintptr_t for us. > > /Bruce