diff mbox series

[v17,03/11] eal: change API of power intrinsics

Message ID f19c29343c55fd614caef1ae16bf37eb0899f8ae.1610635488.git.anatoly.burakov@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers show
Series Add PMD power management | expand

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Anatoly Burakov Jan. 14, 2021, 2:46 p.m. UTC
Instead of passing around pointers and integers, collect everything
into struct. This makes API design around these intrinsics much easier.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---

Notes:
    v16:
    - Add error handling

 drivers/event/dlb/dlb.c                       | 10 ++--
 drivers/event/dlb2/dlb2.c                     | 10 ++--
 lib/librte_eal/arm/rte_power_intrinsics.c     | 20 +++-----
 .../include/generic/rte_power_intrinsics.h    | 50 ++++++++-----------
 lib/librte_eal/ppc/rte_power_intrinsics.c     | 20 +++-----
 lib/librte_eal/x86/rte_power_intrinsics.c     | 42 +++++++++-------
 6 files changed, 70 insertions(+), 82 deletions(-)

Comments

Thomas Monjalon Jan. 18, 2021, 10:26 p.m. UTC | #1
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?

> +	                       **/
> +	uint64_t mask;   /**< 64-bit mask to extract current value from addr */
> +	uint8_t data_sz; /**< Data size (in bytes) that will be used to compare
> +	                  *   expected value with the memory address. Can be 1,
> +	                  *   2, 4, or 8. Supplying any other value will lead to
> +	                  *   undefined result. */

Other parameters are not prefixed with "data_",
so I think this field could be simply named "size".

> +};

I understand this struct is a direct translation of what existed
in 20.11 as function parameters and comments.
If you agree, these comments could be addressed in a separate patch.
Anatoly Burakov Jan. 19, 2021, 10:29 a.m. UTC | #2
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.

> 
>> +	                       **/
>> +	uint64_t mask;   /**< 64-bit mask to extract current value from addr */
>> +	uint8_t data_sz; /**< Data size (in bytes) that will be used to compare
>> +	                  *   expected value with the memory address. Can be 1,
>> +	                  *   2, 4, or 8. Supplying any other value will lead to
>> +	                  *   undefined result. */
> 
> Other parameters are not prefixed with "data_",
> so I think this field could be simply named "size".

OK.

> 
>> +};
> 
> I understand this struct is a direct translation of what existed
> in 20.11 as function parameters and comments.
> If you agree, these comments could be addressed in a separate patch.
> 

I'll be respinning anyway, so might as well do some quick fixups.
Thomas Monjalon Jan. 19, 2021, 10:42 a.m. UTC | #3
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.
Anatoly Burakov Jan. 19, 2021, 11:23 a.m. UTC | #4
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".
Thomas Monjalon Jan. 19, 2021, 2:17 p.m. UTC | #5
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
Anatoly Burakov Jan. 20, 2021, 10:32 a.m. UTC | #6
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.
Thomas Monjalon Jan. 20, 2021, 10:38 a.m. UTC | #7
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"?
Anatoly Burakov Jan. 20, 2021, 11:05 a.m. UTC | #8
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.
Thomas Monjalon Jan. 20, 2021, 11:11 a.m. UTC | #9
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?
Anatoly Burakov Jan. 20, 2021, 11:17 a.m. UTC | #10
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.
diff mbox series

Patch

diff --git a/drivers/event/dlb/dlb.c b/drivers/event/dlb/dlb.c
index 0c95c4793d..d2f2026291 100644
--- a/drivers/event/dlb/dlb.c
+++ b/drivers/event/dlb/dlb.c
@@ -3161,6 +3161,7 @@  dlb_dequeue_wait(struct dlb_eventdev *dlb,
 		/* Interrupts not supported by PF PMD */
 		return 1;
 	} else if (dlb->umwait_allowed) {
+		struct rte_power_monitor_cond pmc;
 		volatile struct dlb_dequeue_qe *cq_base;
 		union {
 			uint64_t raw_qe[2];
@@ -3181,9 +3182,12 @@  dlb_dequeue_wait(struct dlb_eventdev *dlb,
 		else
 			expected_value = 0;
 
-		rte_power_monitor(monitor_addr, expected_value,
-				  qe_mask.raw_qe[1], timeout + start_ticks,
-				  sizeof(uint64_t));
+		pmc.addr = monitor_addr;
+		pmc.val = expected_value;
+		pmc.mask = qe_mask.raw_qe[1];
+		pmc.data_sz = sizeof(uint64_t);
+
+		rte_power_monitor(&pmc, timeout + start_ticks);
 
 		DLB_INC_STAT(ev_port->stats.traffic.rx_umonitor_umwait, 1);
 	} else {
diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
index 86724863f2..c9a8a02278 100644
--- a/drivers/event/dlb2/dlb2.c
+++ b/drivers/event/dlb2/dlb2.c
@@ -2870,6 +2870,7 @@  dlb2_dequeue_wait(struct dlb2_eventdev *dlb2,
 	if (elapsed_ticks >= timeout) {
 		return 1;
 	} else if (dlb2->umwait_allowed) {
+		struct rte_power_monitor_cond pmc;
 		volatile struct dlb2_dequeue_qe *cq_base;
 		union {
 			uint64_t raw_qe[2];
@@ -2890,9 +2891,12 @@  dlb2_dequeue_wait(struct dlb2_eventdev *dlb2,
 		else
 			expected_value = 0;
 
-		rte_power_monitor(monitor_addr, expected_value,
-				  qe_mask.raw_qe[1], timeout + start_ticks,
-				  sizeof(uint64_t));
+		pmc.addr = monitor_addr;
+		pmc.val = expected_value;
+		pmc.mask = qe_mask.raw_qe[1];
+		pmc.data_sz = sizeof(uint64_t);
+
+		rte_power_monitor(&pmc, timeout + start_ticks);
 
 		DLB2_INC_STAT(ev_port->stats.traffic.rx_umonitor_umwait, 1);
 	} else {
diff --git a/lib/librte_eal/arm/rte_power_intrinsics.c b/lib/librte_eal/arm/rte_power_intrinsics.c
index 7e7552fa8a..5f1caaf25b 100644
--- a/lib/librte_eal/arm/rte_power_intrinsics.c
+++ b/lib/librte_eal/arm/rte_power_intrinsics.c
@@ -8,15 +8,11 @@ 
  * This function is not supported on ARM.
  */
 int
-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_power_monitor(const struct rte_power_monitor_cond *pmc,
+		const uint64_t tsc_timestamp)
 {
-	RTE_SET_USED(p);
-	RTE_SET_USED(expected_value);
-	RTE_SET_USED(value_mask);
+	RTE_SET_USED(pmc);
 	RTE_SET_USED(tsc_timestamp);
-	RTE_SET_USED(data_sz);
 
 	return -ENOTSUP;
 }
@@ -25,16 +21,12 @@  rte_power_monitor(const volatile void *p, const uint64_t expected_value,
  * This function is not supported on ARM.
  */
 int
-rte_power_monitor_sync(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_spinlock_t *lck)
+rte_power_monitor_sync(const struct rte_power_monitor_cond *pmc,
+		const uint64_t tsc_timestamp, rte_spinlock_t *lck)
 {
-	RTE_SET_USED(p);
-	RTE_SET_USED(expected_value);
-	RTE_SET_USED(value_mask);
+	RTE_SET_USED(pmc);
 	RTE_SET_USED(tsc_timestamp);
 	RTE_SET_USED(lck);
-	RTE_SET_USED(data_sz);
 
 	return -ENOTSUP;
 }
diff --git a/lib/librte_eal/include/generic/rte_power_intrinsics.h b/lib/librte_eal/include/generic/rte_power_intrinsics.h
index 37e4ec0414..3ad53068d5 100644
--- a/lib/librte_eal/include/generic/rte_power_intrinsics.h
+++ b/lib/librte_eal/include/generic/rte_power_intrinsics.h
@@ -18,6 +18,18 @@ 
  * which are architecture-dependent.
  */
 
+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.
+	                       **/
+	uint64_t mask;   /**< 64-bit mask to extract current value from addr */
+	uint8_t data_sz; /**< Data size (in bytes) that will be used to compare
+	                  *   expected value with the memory address. Can be 1,
+	                  *   2, 4, or 8. Supplying any other value will lead to
+	                  *   undefined result. */
+};
+
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
@@ -35,20 +47,11 @@ 
  * @warning It is responsibility of the user to check if this function is
  *   supported at runtime using `rte_cpu_get_intrinsics_support()` API call.
  *
- * @param p
- *   Address to monitor for changes.
- * @param expected_value
- *   Before attempting the monitoring, the `p` address may be read and compared
- *   against this value. If `value_mask` is zero, this step will be skipped.
- * @param value_mask
- *   The 64-bit mask to use to extract current value from `p`.
+ * @param pmc
+ *   The monitoring condition structure.
  * @param tsc_timestamp
  *   Maximum TSC timestamp to wait for. Note that the wait behavior is
  *   architecture-dependent.
- * @param data_sz
- *   Data size (in bytes) that will be used to compare expected value with the
- *   memory address. Can be 1, 2, 4 or 8. Supplying any other value will lead
- *   to undefined result.
  *
  * @return
  *   0 on success
@@ -56,10 +59,8 @@ 
  *   -ENOTSUP if unsupported
  */
 __rte_experimental
-int 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);
-
+int rte_power_monitor(const struct rte_power_monitor_cond *pmc,
+		const uint64_t tsc_timestamp);
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
@@ -80,20 +81,11 @@  int rte_power_monitor(const volatile void *p,
  * @warning It is responsibility of the user to check if this function is
  *   supported at runtime using `rte_cpu_get_intrinsics_support()` API call.
  *
- * @param p
- *   Address to monitor for changes.
- * @param expected_value
- *   Before attempting the monitoring, the `p` address may be read and compared
- *   against this value. If `value_mask` is zero, this step will be skipped.
- * @param value_mask
- *   The 64-bit mask to use to extract current value from `p`.
+ * @param pmc
+ *   The monitoring condition structure.
  * @param tsc_timestamp
  *   Maximum TSC timestamp to wait for. Note that the wait behavior is
  *   architecture-dependent.
- * @param data_sz
- *   Data size (in bytes) that will be used to compare expected value with the
- *   memory address. Can be 1, 2, 4 or 8. Supplying any other value will lead
- *   to undefined result.
  * @param lck
  *   A spinlock that must be locked before entering the function, will be
  *   unlocked while the CPU is sleeping, and will be locked again once the CPU
@@ -105,10 +97,8 @@  int rte_power_monitor(const volatile void *p,
  *   -ENOTSUP if unsupported
  */
 __rte_experimental
-int rte_power_monitor_sync(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_spinlock_t *lck);
+int rte_power_monitor_sync(const struct rte_power_monitor_cond *pmc,
+		const uint64_t tsc_timestamp, rte_spinlock_t *lck);
 
 /**
  * @warning
diff --git a/lib/librte_eal/ppc/rte_power_intrinsics.c b/lib/librte_eal/ppc/rte_power_intrinsics.c
index 929e0611b0..5e5a1fff5a 100644
--- a/lib/librte_eal/ppc/rte_power_intrinsics.c
+++ b/lib/librte_eal/ppc/rte_power_intrinsics.c
@@ -8,15 +8,11 @@ 
  * This function is not supported on PPC64.
  */
 int
-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_power_monitor(const struct rte_power_monitor_cond *pmc,
+		const uint64_t tsc_timestamp)
 {
-	RTE_SET_USED(p);
-	RTE_SET_USED(expected_value);
-	RTE_SET_USED(value_mask);
+	RTE_SET_USED(pmc);
 	RTE_SET_USED(tsc_timestamp);
-	RTE_SET_USED(data_sz);
 
 	return -ENOTSUP;
 }
@@ -25,16 +21,12 @@  rte_power_monitor(const volatile void *p, const uint64_t expected_value,
  * This function is not supported on PPC64.
  */
 int
-rte_power_monitor_sync(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_spinlock_t *lck)
+rte_power_monitor_sync(const struct rte_power_monitor_cond *pmc,
+		const uint64_t tsc_timestamp, rte_spinlock_t *lck)
 {
-	RTE_SET_USED(p);
-	RTE_SET_USED(expected_value);
-	RTE_SET_USED(value_mask);
+	RTE_SET_USED(pmc);
 	RTE_SET_USED(tsc_timestamp);
 	RTE_SET_USED(lck);
-	RTE_SET_USED(data_sz);
 
 	return -ENOTSUP;
 }
diff --git a/lib/librte_eal/x86/rte_power_intrinsics.c b/lib/librte_eal/x86/rte_power_intrinsics.c
index 2a38440bec..6be5c8b9f1 100644
--- a/lib/librte_eal/x86/rte_power_intrinsics.c
+++ b/lib/librte_eal/x86/rte_power_intrinsics.c
@@ -46,9 +46,8 @@  __check_val_size(const uint8_t sz)
  * Intel(R) 64 and IA-32 Architectures Software Developer's Manual.
  */
 int
-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_power_monitor(const struct rte_power_monitor_cond *pmc,
+		const uint64_t tsc_timestamp)
 {
 	const uint32_t tsc_l = (uint32_t)tsc_timestamp;
 	const uint32_t tsc_h = (uint32_t)(tsc_timestamp >> 32);
@@ -57,7 +56,10 @@  rte_power_monitor(const volatile void *p, const uint64_t expected_value,
 	if (!wait_supported)
 		return -ENOTSUP;
 
-	if (__check_val_size(data_sz) < 0)
+	if (pmc == NULL)
+		return -EINVAL;
+
+	if (__check_val_size(pmc->data_sz) < 0)
 		return -EINVAL;
 
 	/*
@@ -68,14 +70,15 @@  rte_power_monitor(const volatile void *p, const uint64_t expected_value,
 	/* set address for UMONITOR */
 	asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf7;"
 			:
-			: "D"(p));
+			: "D"(pmc->addr));
 
-	if (value_mask) {
-		const uint64_t cur_value = __get_umwait_val(p, data_sz);
-		const uint64_t masked = cur_value & value_mask;
+	if (pmc->mask) {
+		const uint64_t cur_value = __get_umwait_val(
+				pmc->addr, pmc->data_sz);
+		const uint64_t masked = cur_value & pmc->mask;
 
 		/* if the masked value is already matching, abort */
-		if (masked == expected_value)
+		if (masked == pmc->val)
 			return 0;
 	}
 	/* execute UMWAIT */
@@ -93,9 +96,8 @@  rte_power_monitor(const volatile void *p, const uint64_t expected_value,
  * Intel(R) 64 and IA-32 Architectures Software Developer's Manual.
  */
 int
-rte_power_monitor_sync(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_spinlock_t *lck)
+rte_power_monitor_sync(const struct rte_power_monitor_cond *pmc,
+		const uint64_t tsc_timestamp, rte_spinlock_t *lck)
 {
 	const uint32_t tsc_l = (uint32_t)tsc_timestamp;
 	const uint32_t tsc_h = (uint32_t)(tsc_timestamp >> 32);
@@ -104,7 +106,10 @@  rte_power_monitor_sync(const volatile void *p, const uint64_t expected_value,
 	if (!wait_supported)
 		return -ENOTSUP;
 
-	if (__check_val_size(data_sz) < 0)
+	if (pmc == NULL || lck == NULL)
+		return -EINVAL;
+
+	if (__check_val_size(pmc->data_sz) < 0)
 		return -EINVAL;
 
 	/*
@@ -115,14 +120,15 @@  rte_power_monitor_sync(const volatile void *p, const uint64_t expected_value,
 	/* set address for UMONITOR */
 	asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf7;"
 			:
-			: "D"(p));
+			: "D"(pmc->addr));
 
-	if (value_mask) {
-		const uint64_t cur_value = __get_umwait_val(p, data_sz);
-		const uint64_t masked = cur_value & value_mask;
+	if (pmc->mask) {
+		const uint64_t cur_value = __get_umwait_val(
+				pmc->addr, pmc->data_sz);
+		const uint64_t masked = cur_value & pmc->mask;
 
 		/* if the masked value is already matching, abort */
-		if (masked == expected_value)
+		if (masked == pmc->val)
 			return 0;
 	}
 	rte_spinlock_unlock(lck);