[dpdk-dev] [PATCH v1 1/7] power_intrinsics: allow monitor checks inversion
Burakov, Anatoly
anatoly.burakov at intel.com
Wed Jun 23 12:00:39 CEST 2021
On 23-Jun-21 10:55 AM, Ananyev, Konstantin wrote:
>
>
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov at intel.com>
>> Sent: Wednesday, June 23, 2021 10:43 AM
>> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org; Richardson, Bruce <bruce.richardson at intel.com>
>> Cc: Loftus, Ciara <ciara.loftus at intel.com>; Hunt, David <david.hunt at intel.com>
>> Subject: Re: [PATCH v1 1/7] power_intrinsics: allow monitor checks inversion
>>
>> On 21-Jun-21 1:56 PM, Ananyev, Konstantin wrote:
>>>
>>> Hi Anatoly,
>>>
>>>> Previously, the semantics of power monitor were such that we were
>>>> checking current value against the expected value, and if they matched,
>>>> then the sleep was aborted. This is somewhat inflexible, because it only
>>>> allowed us to check for a specific value.
>>>>
>>>> This commit adds an option to reverse the check, so that we can have
>>>> monitor sleep aborted if the expected value *doesn't* match what's in
>>>> memory. This allows us to both implement all currently implemented
>>>> driver code, as well as support more use cases which don't easily map to
>>>> previous semantics (such as waiting on writes to AF_XDP counter value).
>>>>
>>>> Since the old behavior is the default, no need to adjust existing
>>>> implementations.
>>>>
>>>> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
>>>> ---
>>>> lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
>>>> lib/eal/x86/rte_power_intrinsics.c | 5 ++++-
>>>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/eal/include/generic/rte_power_intrinsics.h b/lib/eal/include/generic/rte_power_intrinsics.h
>>>> index dddca3d41c..1006c2edfc 100644
>>>> --- a/lib/eal/include/generic/rte_power_intrinsics.h
>>>> +++ b/lib/eal/include/generic/rte_power_intrinsics.h
>>>> @@ -31,6 +31,10 @@ struct rte_power_monitor_cond {
>>>> * 4, or 8. Supplying any other value will result in
>>>> * an error.
>>>> */
>>>> + uint8_t invert; /**< Invert check for expected value (e.g. instead of
>>>> + * checking if `val` matches something, check if
>>>> + * `val` *doesn't* match a particular value)
>>>> + */
>>>> };
>>>>
>>>> /**
>>>> diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
>>>> index 39ea9fdecd..5d944e9aa4 100644
>>>> --- a/lib/eal/x86/rte_power_intrinsics.c
>>>> +++ b/lib/eal/x86/rte_power_intrinsics.c
>>>> @@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
>>>> const uint64_t masked = cur_value & pmc->mask;
>>>>
>>>> /* if the masked value is already matching, abort */
>>>> - if (masked == pmc->val)
>>>> + if (!pmc->invert && masked == pmc->val)
>>>> + goto end;
>>>> + /* same, but for inverse check */
>>>> + if (pmc->invert && masked != pmc->val)
>>>> goto end;
>>>> }
>>>>
>>>
>>> Hmm..., such approach looks too 'patchy'...
>>> Can we at least replace 'inver' with something like:
>>> enum rte_power_monitor_cond_op {
>>> _EQ, NEQ,...
>>> };
>>> Then at least new comparions ops can be added in future.
>>> Even better I think would be to just leave to PMD to provide a comparison callback.
>>> Will make things really simple and generic:
>>> struct rte_power_monitor_cond {
>>> volatile void *addr;
>>> int (*cmp)(uint64_t val);
>>> uint8_t size;
>>> };
>>> And then in rte_power_monitor(...):
>>> ....
>>> const uint64_t cur_value = __get_umwait_val(pmc->addr, pmc->size);
>>> if (pmc->cmp(cur_value) != 0)
>>> goto end;
>>> ....
>>>
>>
>> I like the idea of a callback, but these are supposed to be
>> intrinsic-like functions, so putting too much into them is contrary to
>> their goal, and it's going to make the API hard to use in simpler cases
>> (e.g. when we're explicitly calling rte_power_monitor as opposed to
>> letting the RX callback do it for us). For example, event/dlb code calls
>> rte_power_monitor explicitly.
>
> Good point, I didn't know that.
> Would be interesting to see how do they use it.
To be fair, it should be possible to rewrite their code using a
callback. Perhaps adding a (void *) parameter for any custom data
related to the callback (because C doesn't have closures...), but
otherwise it should be doable, so the question isn't that it's
impossible to rewrite event/dlb code to use callbacks, it's more of an
issue with complicating usage of already-not-quite-straightforward API
even more.
>
>>
>> It's going to be especially "fun" to do these indirect function calls
>> from inside transactional region on call to multi-monitor.
>
> But the callback is not supposed to do any memory reads/writes.
> Just mask/compare of the provided value with some constant.
Yeah, but with callbacks we can't really control that, can we? I mean i
guess a *sane* implementation wouldn't do that, but still, it's
theoretically possible to perform more complex checks and even touch
some unrelated data in the process.
>
>> I'm not
>> opposed to having a callback here, but maybe others have more thoughts
>> on this?
>>
>> --
>> Thanks,
>> Anatoly
--
Thanks,
Anatoly
More information about the dev
mailing list