[dpdk-dev] [PATCH v6 02/10] eal: add power management intrinsics

Burakov, Anatoly anatoly.burakov at intel.com
Thu Oct 15 12:09:35 CEST 2020


On 14-Oct-20 6:48 PM, Ananyev, Konstantin wrote:
> 
> 
>>
>> From: Liang Ma <liang.j.ma at intel.com>
>>
>> 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 <liang.j.ma at intel.com>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
>> Acked-by: David Christensen <drc at linux.vnet.ibm.com>
>> ---
>>
>> 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 <rte_common.h>
>> +
>> +#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 <rte_common.h>
>> +
>> +#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


More information about the dev mailing list