[dpdk-dev] [PATCH v10 3/9] eal: add intrinsics support check infrastructure

[Burakov, Anatoly]

On 30-Oct-20 3:44 PM, Thomas Monjalon wrote:
> 30/10/2020 16:27, Burakov, Anatoly:
>> On 30-Oct-20 2:09 PM, Thomas Monjalon wrote:
>>> 30/10/2020 14:37, Burakov, Anatoly:
>>>> On 30-Oct-20 10:14 AM, Thomas Monjalon wrote:
>>>>> 30/10/2020 11:09, Burakov, Anatoly:
>>>>>> On 29-Oct-20 9:27 PM, David Marchand wrote:
>>>>>>> On Tue, Oct 27, 2020 at 4:00 PM Liang Ma <liang.j.ma at intel.com> wrote:
>>>>>>>>
>>>>>>>> Currently, it is not possible to check support for intrinsics that
>>>>>>>> are platform-specific, cannot be abstracted in a generic way, or do not
>>>>>>>> have support on all architectures. The CPUID flags can be used to some
>>>>>>>> extent, but they are only defined for their platform, while intrinsics
>>>>>>>> will be available to all code as they are in generic headers.
>>>>>>>>
>>>>>>>> This patch introduces infrastructure to check support for certain
>>>>>>>> platform-specific intrinsics, and adds support for checking support for
>>>>>>>> IA power management-related intrinsics for UMWAIT/UMONITOR and TPAUSE.
>>>>>>>>
>>>>>>>> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
>>>>>>>> Signed-off-by: Liang Ma <liang.j.ma at intel.com>
>>>>>>>> Acked-by: David Christensen <drc at linux.vnet.ibm.com>
>>>>>>>> Acked-by: Jerin Jacob <jerinj at marvell.com>
>>>>>>>> Acked-by: Ruifeng Wang <ruifeng.wang at arm.com>
>>>>>>>> Acked-by: Ray Kinsella <mdr at ashroe.eu>
>>>>>>>
>>>>>>> Coming late to the party, it seems crowded...
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> diff --git a/lib/librte_eal/include/generic/rte_cpuflags.h b/lib/librte_eal/include/generic/rte_cpuflags.h
>>>>>>>> index 872f0ebe3e..28a5aecde8 100644
>>>>>>>> --- a/lib/librte_eal/include/generic/rte_cpuflags.h
>>>>>>>> +++ b/lib/librte_eal/include/generic/rte_cpuflags.h
>>>>>>>> @@ -13,6 +13,32 @@
>>>>>>>>      #include "rte_common.h"
>>>>>>>>      #include <errno.h>
>>>>>>>>
>>>>>>>> +#include <rte_compat.h>
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * Structure used to describe platform-specific intrinsics that may or may not
>>>>>>>> + * be supported at runtime.
>>>>>>>> + */
>>>>>>>> +struct rte_cpu_intrinsics {
>>>>>>>> +       uint32_t power_monitor : 1;
>>>>>>>> +       /**< indicates support for rte_power_monitor function */
>>>>>>>> +       uint32_t power_pause : 1;
>>>>>>>> +       /**< indicates support for rte_power_pause function */
>>>>>>>> +};
>>>>>>>
>>>>>>> - The rte_power library is supposed to be built on top of cpuflags.
>>>>>>> Not the other way around.
>>>>>>> Those capabilities should have been kept inside the rte_power_ API and
>>>>>>> not pollute the cpuflags API.
>>>>>>>
>>>>>>> - All of this should have come as a single patch as the previously
>>>>>>> introduced API is unusable before.
>>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * @warning
>>>>>>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>>>>>>> + *
>>>>>>>> + * Check CPU support for various intrinsics at runtime.
>>>>>>>> + *
>>>>>>>> + * @param intrinsics
>>>>>>>> + *     Pointer to a structure to be filled.
>>>>>>>> + */
>>>>>>>> +__rte_experimental
>>>>>>>> +void
>>>>>>>> +rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics);
>>>>>>>> +
>>>>>>>>      /**
>>>>>>>>       * Enumeration of all CPU features supported
>>>>>>>>       */
>>>>>>>> diff --git a/lib/librte_eal/include/generic/rte_power_intrinsics.h b/lib/librte_eal/include/generic/rte_power_intrinsics.h
>>>>>>>> index fb897d9060..03a326f076 100644
>>>>>>>> --- a/lib/librte_eal/include/generic/rte_power_intrinsics.h
>>>>>>>> +++ b/lib/librte_eal/include/generic/rte_power_intrinsics.h
>>>>>>>> @@ -32,6 +32,10 @@
>>>>>>>>       * checked against the expected value, and if they match, the entering of
>>>>>>>>       * optimized power state may be aborted.
>>>>>>>>       *
>>>>>>>> + * @warning It is responsibility of the user to check if this function is
>>>>>>>> + *   supported at runtime using `rte_cpu_get_features()` API call. Failing to do
>>>>>>>> + *   so may result in an illegal CPU instruction error.
>>>>>>>> + *
>>>>>>>
>>>>>>> - Reading this API description... what am I supposed to do in my
>>>>>>> application or driver who wants to use the new
>>>>>>> rte_power_monitor/rte_power_pause stuff?
>>>>>>>
>>>>>>> I should call rte_cpu_get_features(TOTO) ?
>>>>>>> This comment does not give a hint.
>>>>>>>
>>>>>>> I suppose the intent was to refer to the rte_cpu_get_intrinsics_support() thing.
>>>>>>> This must be fixed.
>>>>>>>
>>>>>>>
>>>>>>> - Again, I wonder why we are exposing all this stuff.
>>>>>>> This should be hidden in the rte_power API.
>>>>>>>
>>>>>>
>>>>>> We're exposing all of this here because the intrinsics are *not* part of
>>>>>> the power API but rather are generic headers within EAL. Therefore, any
>>>>>> infrastructure checking for their support can *not* reside in the power
>>>>>> library, but instead has to be in EAL.
>>>>>>
>>>>>> The intended usage here is to call this function before calling
>>>>>> rte_power_monitor(), such that:
>>>>>>
>>>>>> 	struct rte_cpu_intrinsics intrinsics;
>>>>>>
>>>>>> 	rte_cpu_get_intrinsics_support(&intrinsics);
>>>>>>
>>>>>> 	if (!intrinsics.power_monitor) {
>>>>>> 		// rte_power_monitor not supported and cannot be used
>>>>>> 		return;
>>>>>> 	}
>>>>>
>>>>> This check could be done inside the rte_power API.
>>>>
>>>> I'm not quite clear on exactly what you're asking here.
>>>>
>>>> Do you mean the example code above? If so, code like that is already
>>>> present in the power library, at the callback enable stage.
>>>>
>>>> If you mean to say, i should put this check into the rte_power_monitor
>>>> intrinsic, then no, i don't think it's a good idea to have this
>>>> expensive check every time you call rte_power_monitor.
>>>
>>> No but it can be done at initialization time.
>>> According to what you say above, it is alread done at callback enable stage.
>>> So the app does not need to do the check?
>>
>> Admittedly it's a bit confusing, but please bear with me.
>>
>> There are two separate issues at hand: the intrinsic itself, and the
>> calling code. We provide both.
>>
>> That means, the *calling code* should do the check. In our case, *our*
>> calling code is the callback. However, nothing stops someone else from
>> implementing their own scheme using our intrinsic - in that case, the
>> user will be responsible to check if the intrinsic is supported before
>> using it in their own code, because they won't be using our callback but
>> will be using our intrinsic.
>>
>> So, we have a check *in our calling code*. But if someone were to use
>> the *intrinsic* directly (like DLB), they would have to add their own
>> checks around the intrinsic usage.
>>
>> Our power intrinsic is a static inline function. Are you proposing to
>> add some sort of function pointer wrapper and make it an indirect call
>> instead of a static inline function? (or indeed a proper function)
>>
>>>
>>>> If you mean put this entire infrastructure into the power API - well,
>>>> that kinda defeats the purpose of both having these intrinsics in
>>>> generic headers and having a generic CPU feature check infrastructure
>>>> that was requested of us during the review. We of course can move the
>>>> intrinsic to the power library and outside of EAL, but then anything
>>>> that requires UMWAIT will have to depend on the librte_power.
>>>
>>> Yes the intrinsics can be in EAL if usable.
>>> But it seems DLB author cannot use what is in EAL.
>>
>> I'll let the DLB authors clarify that themselves, but as far as i'm
>> aware, it seems that this is not the case - while their current code
>> wouldn't be able to use these intrinsics by search-and-replace, they
>> will be able to use them with a couple of changes to their code that
>> basically amounted to reimplementation of our intrinsics.
>>
>>>
>>>> Please clarify exactly what changes you would like to see here, and what
>>>> is your objection.
>>>>
>>>>>
>>>>>> 	// proceed with rte_power_monitor usage
>>>>>>
>>>>>> Failing to do that will result in either -ENOTSUP on non-x86, or illegal
>>>>>> instruction crash on x86 that doesn't have that instruction (because we
>>>>>> encode raw opcode).
>>>>>>
>>>>>> I've *not* added this to the previous patches because i wanted to get
>>>>>> this part reviewed specifically, and not mix it with other IA-specific
>>>>>> stuff. It seems that i've succeeded in that goal, as this patch has 4
>>>>>> likes^W acks :)
>>>>>
>>>>> You did not explain the need for rte_cpu_get_features() call.
>>>>>
>>>>
>>>> Did not explain *where*? Are you suggesting i put things about
>>>> rte_power_monitor into documentation for rte_cpu_get_intrinsics_support?
>>>> The documentation for rte_power_monitor already states that one should
>>>> use rte_cpu_get_intrinsics_support API to check if the rte_power_monitor
>>>> is supported on current machine. What else is missing?
>>>
>>> In your example above, you do not call rte_cpu_get_features()
>>> which is documented as required in the EAL doc.
>>>
>>
>> I'm not sure i follow. This is unrelated to rte_cpu_get_features call.
>> The rte_cpu_get_features is a CPUID check, and it was decided not to use
>> it because the WAITPKG CPUID flag is only defined for x86 and not for
>> other archs. This new call (rte_cpu_get_intrinsics_support) is non-arch
>> specific, but will have an arch-specific implementation (which happens
>> to use rte_cpu_get_features to detect support for WAITPKG). I have given
>> the example code of how to detect support for rte_power_monitor using
>> this new code, in the code example you just referred to.
> 
> Please read the API again:
> http://git.dpdk.org/dpdk/tree/lib/librte_eal/include/generic/rte_power_intrinsics.h
> "
>   * @warning It is responsibility of the user to check if this function is
>   *   supported at runtime using `rte_cpu_get_features()` API call.
>   *   Failing to do so may result in an illegal CPU instruction error.
> "
> Why is it referring to rte_cpu_get_features?
> 

Aw, crap. You're right, it's an artifact of earlier implementation. 
We'll fix this. Thanks for letting us know!

-- 
Thanks,
Anatoly