From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id C224EA04DB; Thu, 15 Oct 2020 12:09:45 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9B6071DE04; Thu, 15 Oct 2020 12:09:44 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id CF5051DDFA for ; Thu, 15 Oct 2020 12:09:42 +0200 (CEST) IronPort-SDR: C+qM7WvJdNBRDvmiKMUjk3yvgSCRRfUJ2pBHhqyWojG5kaKBSBAiBvg+Oq2UiFoAdRQ7txf8St B37LAZC/vbyA== X-IronPort-AV: E=McAfee;i="6000,8403,9774"; a="166427938" X-IronPort-AV: E=Sophos;i="5.77,378,1596524400"; d="scan'208";a="166427938" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Oct 2020 03:09:39 -0700 IronPort-SDR: Fmb1HfkQbP+rL+zYLPX33KZ9THDZXpmSIb1+X0LpFmNbh1Hn07dG1lGjZnaQS8cwI1dSv4MMyp Hifa1tyWfDpw== X-IronPort-AV: E=Sophos;i="5.77,378,1596524400"; d="scan'208";a="521787166" Received: from orozen-mobl.ger.corp.intel.com (HELO [10.213.243.220]) ([10.213.243.220]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Oct 2020 03:09:37 -0700 To: "Ananyev, Konstantin" , "dev@dpdk.org" Cc: "Ma, Liang J" , Jan Viktorin , Ruifeng Wang , David Christensen , "Richardson, Bruce" , "Hunt, David" , "jerinjacobk@gmail.com" , "thomas@monjalon.net" , "McDaniel, Timothy" , "Eads, Gage" , "Macnamara, Chris" References: <532f45c5d79b4c30a919553d322bb66e91534466.1602258833.git.anatoly.burakov@intel.com> From: "Burakov, Anatoly" Message-ID: <493732d1-379e-51f8-1740-8df784ab96c7@intel.com> Date: Thu, 15 Oct 2020 11:09:35 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 02/10] eal: add power management intrinsics X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 14-Oct-20 6:48 PM, Ananyev, Konstantin wrote: > > >> >> From: Liang Ma >> >> Add two new power management intrinsics, and provide an implementation >> in eal/x86 based on UMONITOR/UMWAIT instructions. The instructions >> are implemented as raw byte opcodes because there is not yet widespread >> compiler support for these instructions. >> >> The power management instructions provide an architecture-specific >> function to either wait until a specified TSC timestamp is reached, or >> optionally wait until either a TSC timestamp is reached or a memory >> location is written to. The monitor function also provides an optional >> comparison, to avoid sleeping when the expected write has already >> happened, and no more writes are expected. >> >> For more details, please refer to Intel(R) 64 and IA-32 Architectures >> Software Developer's Manual, Volume 2. >> >> Signed-off-by: Liang Ma >> Signed-off-by: Anatoly Burakov >> Acked-by: David Christensen >> --- >> >> Notes: >> v6: >> - Add spinlock-enabled version to allow pthread-wait-like >> constructs with umwait >> - Clarify comments >> - Added experimental tags to intrinsics >> - Added endianness support >> v5: >> - Removed return values >> - Simplified intrinsics and hardcoded C0.2 state >> - Added other arch stubs >> >> lib/librte_eal/arm/include/meson.build | 1 + >> .../arm/include/rte_power_intrinsics.h | 58 ++++++++ >> .../include/generic/rte_power_intrinsics.h | 111 +++++++++++++++ >> lib/librte_eal/include/meson.build | 1 + >> lib/librte_eal/ppc/include/meson.build | 1 + >> .../ppc/include/rte_power_intrinsics.h | 58 ++++++++ >> lib/librte_eal/x86/include/meson.build | 1 + >> .../x86/include/rte_power_intrinsics.h | 132 ++++++++++++++++++ >> 8 files changed, 363 insertions(+) >> create mode 100644 lib/librte_eal/arm/include/rte_power_intrinsics.h >> create mode 100644 lib/librte_eal/include/generic/rte_power_intrinsics.h >> create mode 100644 lib/librte_eal/ppc/include/rte_power_intrinsics.h >> create mode 100644 lib/librte_eal/x86/include/rte_power_intrinsics.h >> >> diff --git a/lib/librte_eal/arm/include/meson.build b/lib/librte_eal/arm/include/meson.build >> index 73b750a18f..c6a9f70d73 100644 >> --- a/lib/librte_eal/arm/include/meson.build >> +++ b/lib/librte_eal/arm/include/meson.build >> @@ -20,6 +20,7 @@ arch_headers = files( >> 'rte_pause_32.h', >> 'rte_pause_64.h', >> 'rte_pause.h', >> +'rte_power_intrinsics.h', >> 'rte_prefetch_32.h', >> 'rte_prefetch_64.h', >> 'rte_prefetch.h', >> diff --git a/lib/librte_eal/arm/include/rte_power_intrinsics.h b/lib/librte_eal/arm/include/rte_power_intrinsics.h >> new file mode 100644 >> index 0000000000..b04ba10c76 >> --- /dev/null >> +++ b/lib/librte_eal/arm/include/rte_power_intrinsics.h >> @@ -0,0 +1,58 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright(c) 2020 Intel Corporation >> + */ >> + >> +#ifndef _RTE_POWER_INTRINSIC_ARM_H_ >> +#define _RTE_POWER_INTRINSIC_ARM_H_ >> + >> +#ifdef __cplusplus >> +extern "C" { >> +#endif >> + >> +#include >> + >> +#include "generic/rte_power_intrinsics.h" >> + >> +/** >> + * This function is not supported on ARM. >> + */ > > Here and in other places - please follow dpdk coding convention > for function definitions, i.e: > static inline void > rte_power_monitor(... > Yep, will do. >> +static inline void rte_power_monitor(const volatile void *p, >> +const uint64_t expected_value, const uint64_t value_mask, >> +const uint64_t tsc_timestamp, const uint8_t data_sz) >> +{ >> +RTE_SET_USED(p); >> +RTE_SET_USED(expected_value); >> +RTE_SET_USED(value_mask); >> +RTE_SET_USED(tsc_timestamp); >> +RTE_SET_USED(data_sz); >> +} > > You can probably put NOP implementations of these rte_powe_* functions > into generic/rte_power_intrinsics.h. > So, wouldn't need to duplicate them for every non-supported arch. > Same as it was done for rte_wait_until_equal_*(). > Will look into it. >> + * >> + * This file define APIs for advanced power management, >> + * which are architecture-dependent. >> + */ >> + >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice >> + * >> + * Monitor specific address for changes. This will cause the CPU to enter an >> + * architecture-defined optimized power state until either the specified >> + * memory address is written to, a certain TSC timestamp is reached, or other >> + * reasons cause the CPU to wake up. >> + * >> + * Additionally, an `expected` 64-bit value and 64-bit mask are provided. If >> + * mask is non-zero, the current value pointed to by the `p` pointer will be >> + * checked against the expected value, and if they match, the entering of >> + * optimized power state may be aborted. >> + * >> + * @param p >> + * Address to monitor for changes. Must be aligned on an 64-byte boundary. > > Is 64B alignment really needed? I'm not 100% sure to be honest, but it's there just in case. I can remove it. >> 'rte_prefetch.h', >> +'rte_power_intrinsics.h', >> 'rte_pause.h', >> 'rte_rtm.h', >> 'rte_rwlock.h', >> diff --git a/lib/librte_eal/x86/include/rte_power_intrinsics.h b/lib/librte_eal/x86/include/rte_power_intrinsics.h >> new file mode 100644 >> index 0000000000..9ac8e6eef6 >> --- /dev/null >> +++ b/lib/librte_eal/x86/include/rte_power_intrinsics.h >> @@ -0,0 +1,132 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright(c) 2020 Intel Corporation >> + */ >> + >> +#ifndef _RTE_POWER_INTRINSIC_X86_64_H_ >> +#define _RTE_POWER_INTRINSIC_X86_64_H_ > > Why '_64_H'? > My understanding was these ops are supported 32-bit mode too. Yeah, artifact of early implementation. Will fix. > >> + >> +#ifdef __cplusplus >> +extern "C" { >> +#endif >> + >> +#include >> + >> +#include "generic/rte_power_intrinsics.h" >> + >> +static inline uint64_t __get_umwait_val(const volatile void *p, >> +const uint8_t sz) >> +{ >> +switch (sz) { >> +case 1: > > Just as a nit: > case sizeof(type_x): > return *(const volatile type_x *)p; Thanks, will fix. > >> +return *(const volatile uint8_t *)p; >> +case 2: >> +return *(const volatile uint16_t *)p; >> +case 4: >> +return *(const volatile uint32_t *)p; >> +case 8: >> +return *(const volatile uint64_t *)p; >> +default: >> +/* this is an intrinsic, so we can't have any error handling */ > > RTE_ASSERT(0); ? Great idea, will add. -- Thanks, Anatoly