[dpdk-dev] [PATCH v17 03/11] eal: change API of power intrinsics

Burakov, Anatoly anatoly.burakov at intel.com
Wed Jan 20 12:17:48 CET 2021


On 20-Jan-21 11:11 AM, Thomas Monjalon wrote:
> 20/01/2021 12:05, Burakov, Anatoly:
>> On 20-Jan-21 10:38 AM, Thomas Monjalon wrote:
>>> 20/01/2021 11:32, Burakov, Anatoly:
>>>> On 19-Jan-21 2:17 PM, Thomas Monjalon wrote:
>>>>> 19/01/2021 12:23, Burakov, Anatoly:
>>>>>> On 19-Jan-21 10:42 AM, Thomas Monjalon wrote:
>>>>>>> 19/01/2021 11:29, Burakov, Anatoly:
>>>>>>>> On 18-Jan-21 10:26 PM, Thomas Monjalon wrote:
>>>>>>>>> 14/01/2021 15:46, Anatoly Burakov:
>>>>>>>>>> +struct rte_power_monitor_cond {
>>>>>>>>>> +	volatile void *addr;  /**< Address to monitor for changes */
>>>>>>>>>> +	uint64_t val;         /**< Before attempting the monitoring, the address
>>>>>>>>>> +	                       *   may be read and compared against this value.
>>>>>>>>>
>>>>>>>>> "may" be read and compared?
>>>>>>>>> Is there a case where there is no read and compare?
>>>>>>>>
>>>>>>>> Yes, if the mask is not set.
>>>>>>>
>>>>>>> If the mask is not set, the address is "read" anyway
>>>>>>> or it is only "watched" for any change?
>>>>>>>
>>>>>>> Sorry the mechanism is really not clear to me.
>>>>>>>
>>>>>>
>>>>>> The "value" is only used to avoid the sleep, i.e. to check if the write
>>>>>> has already happened. We're waiting on *a write* rather than *a value*,
>>>>>> so it's not equivalent to "wait until equal" call. It's more of a "sleep
>>>>>> until something happens".
>>>>>
>>>>> Please make things explicit in doxygen.
>>>>> The behaviour of each case should be explained crystal clear.
>>>>> Thanks
>>>>
>>>> It is explained in the comments to `rte_power_monitor()` call. But OK,
>>>> i'll add more clarification for the struct too.
>>>
>>> Please avoid the word "may" in API description.
>>>
>>> This is what is explained in rte_power_monitor:
>>> "
>>>    * 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.
>>> "
>>>
>>> Can we replace "may" by "will"?
>>>
>>
>> Yep, we can. However, the "may" part was intended to leave some wiggle
>> room for a different implementation, should the need arise, and i find
>> "will" to be needlessly prescriptive. Frankly, i do not see the need for
>> such a detailed description of what the API does under the hood, as long
>> as it's clear what its effects are. The main purpose is waiting for a
>> write. The mask is only used to check whether the expected write has
>> already happened by the time we're calling the API. Whether the CPU then
>> does or does not go to sleep is not really relevant IMO.
> 
> I think it is relevant but I may be wrong.
> Any other opinions?
> 

I have no objection in documenting that further, so i'll go ahead and do 
it :) It's just that i think it's not necessary to be *this* detailed 
about how the API does things.

-- 
Thanks,
Anatoly


More information about the dev mailing list