Enhancements for PMD power management

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 <[email protected]>

---
 lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
 lib/eal/x86/rte_power_intrinsics.c             | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

lib/eal/include/generic/rte_power_intrinsics.h +4 -0

index dddca3d41c..1006c2edfc 100644
@@ -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)
+	                  */
 };
 
 /**

lib/eal/x86/rte_power_intrinsics.c +0 -0

index 39ea9fdecd..5d944e9aa4 100644
@@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
 		const uint64_t masked = cur_value & pmc->mask;

Hi Anatoly,

Quoted 41 lines

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 <[email protected]>

---
  lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
  lib/eal/x86/rte_power_intrinsics.c             | 5 ++++-
  2 files changed, 8 insertions(+), 1 deletion(-)

lib/eal/include/generic/rte_power_intrinsics.h +4 -0

index dddca3d41c..1006c2edfc 100644
@@ -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)
+                       */
  };
 
  /**

lib/eal/x86/rte_power_intrinsics.c +0 -0

index 39ea9fdecd..5d944e9aa4 100644
@@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
               const uint64_t masked = cur_value & pmc->mask;

lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 126

Quoted 10 lines

lib/eal/x86/rte_power_intrinsics.c +4 -1

 
               /* 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;

....

-----Original Message-----
From: Burakov, Anatoly <[email protected]>
Sent: Wednesday, June 23, 2021 10:43 AM
To: Ananyev, Konstantin <[email protected]>; [email protected]; Richardson, Bruce <[email protected]>
Cc: Loftus, Ciara <[email protected]>; Hunt, David <[email protected]>
Subject: Re: [PATCH v1 1/7] power_intrinsics: allow monitor checks inversion

Quoted 68 lines · Konstantin

Hi Anatoly,

Quoted 41 lines

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 <[email protected]>

---
  lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
  lib/eal/x86/rte_power_intrinsics.c             | 5 ++++-
  2 files changed, 8 insertions(+), 1 deletion(-)

lib/eal/include/generic/rte_power_intrinsics.h +4 -0

index dddca3d41c..1006c2edfc 100644
@@ -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)
+                       */
  };
 
  /**

lib/eal/x86/rte_power_intrinsics.c +0 -0

index 39ea9fdecd..5d944e9aa4 100644
@@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
               const uint64_t masked = cur_value & pmc->mask;

lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 126

Quoted 10 lines

lib/eal/x86/rte_power_intrinsics.c +4 -1

 
               /* 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;
};

Quoted 7 lines

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.

It's going to be especially "fun" to do these indirect function calls from inside transactional region on call to multi-monitor.

I'm not
opposed to having a callback here, but maybe others have more thoughts on this?

--
Thanks,
Anatoly

Quoted 86 lines

-----Original Message-----
From: Burakov, Anatoly <[email protected]>
Sent: Wednesday, June 23, 2021 10:43 AM
To: Ananyev, Konstantin <[email protected]>; [email protected]; Richardson, Bruce <[email protected]>
Cc: Loftus, Ciara <[email protected]>; Hunt, David <[email protected]>
Subject: Re: [PATCH v1 1/7] power_intrinsics: allow monitor checks inversion

Quoted 68 lines · Konstantin

Hi Anatoly,

Quoted 41 lines

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 <[email protected]>

---
   lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
   lib/eal/x86/rte_power_intrinsics.c             | 5 ++++-
   2 files changed, 8 insertions(+), 1 deletion(-)

lib/eal/include/generic/rte_power_intrinsics.h +4 -0

index dddca3d41c..1006c2edfc 100644
@@ -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)
+                       */
   };
 
   /**

lib/eal/x86/rte_power_intrinsics.c +0 -0

index 39ea9fdecd..5d944e9aa4 100644
@@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
                const uint64_t masked = cur_value & pmc->mask;

lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 126

Quoted 10 lines

lib/eal/x86/rte_power_intrinsics.c +4 -1

 
                /* 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;
};

Quoted 7 lines

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;

....

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

I'm not
opposed to having a callback here, but maybe others have more thoughts on this?

--
Thanks,
Anatoly

Quoted 82 lines

Quoted 75 lines

Quoted 67 lines

Quoted 43 lines

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 <[email protected]>

---
   lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
   lib/eal/x86/rte_power_intrinsics.c             | 5 ++++-
   2 files changed, 8 insertions(+), 1 deletion(-)

lib/eal/include/generic/rte_power_intrinsics.h +4 -0

index dddca3d41c..1006c2edfc 100644
@@ -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)
+                       */
   };
 
   /**

lib/eal/x86/rte_power_intrinsics.c +0 -0

index 39ea9fdecd..5d944e9aa4 100644
@@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
                const uint64_t masked = cur_value & pmc->mask;

lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 126

Quoted 10 lines

lib/eal/x86/rte_power_intrinsics.c +4 -1

 
                /* 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;
};

Quoted 7 lines

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;

....

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 4 lines

It's going to be especially "fun" to do these indirect function calls from inside transactional region on call to multi-monitor.

Quoted 2 lines

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.

Quoted 9 lines

Quoted 9 lines

I'm not
opposed to having a callback here, but maybe others have more thoughts on this?

--
Thanks,
Anatoly

--
Thanks,
Anatoly

Quoted 98 lines

Quoted 82 lines

Quoted 75 lines

Quoted 67 lines

Quoted 43 lines

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 <[email protected]>

---
    lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
    lib/eal/x86/rte_power_intrinsics.c             | 5 ++++-
    2 files changed, 8 insertions(+), 1 deletion(-)

lib/eal/include/generic/rte_power_intrinsics.h +4 -0

index dddca3d41c..1006c2edfc 100644
@@ -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)
+                       */
    };
 
    /**

lib/eal/x86/rte_power_intrinsics.c +0 -0

index 39ea9fdecd..5d944e9aa4 100644
@@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
                 const uint64_t masked = cur_value & pmc->mask;

lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 126

Quoted 10 lines

lib/eal/x86/rte_power_intrinsics.c +4 -1

 
                 /* 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;
};

Quoted 7 lines

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;

....

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 4 lines

It's going to be especially "fun" to do these indirect function calls from inside transactional region on call to multi-monitor.

Quoted 7 lines

Quoted 2 lines

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.

Yep, PMD developer can ignore recommendations and do whatever
he wants in the call-back. We can't control it.
If he touches some memory in it - probably there will be more spurious wakeups and less power saves. In principle it is the same with all other PMD dev-ops - we have to trust that they are doing what they have to.

Quoted 15 lines

Quoted 10 lines

Quoted 10 lines

I'm not
opposed to having a callback here, but maybe others have more thoughts on this?

--
Thanks,
Anatoly

--
Thanks,
Anatoly

Quoted 106 lines

Quoted 98 lines

Quoted 82 lines

Quoted 75 lines

Quoted 67 lines

Quoted 43 lines

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 <[email protected]>

---
    lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
    lib/eal/x86/rte_power_intrinsics.c             | 5 ++++-
    2 files changed, 8 insertions(+), 1 deletion(-)

lib/eal/include/generic/rte_power_intrinsics.h +4 -0

index dddca3d41c..1006c2edfc 100644
@@ -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)
+                       */
    };
 
    /**

lib/eal/x86/rte_power_intrinsics.c +0 -0

index 39ea9fdecd..5d944e9aa4 100644
@@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
                 const uint64_t masked = cur_value & pmc->mask;

lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 126

Quoted 10 lines

lib/eal/x86/rte_power_intrinsics.c +4 -1

 
                 /* 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;
};

Quoted 7 lines

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;

....

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 4 lines

It's going to be especially "fun" to do these indirect function calls from inside transactional region on call to multi-monitor.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

Yep, PMD developer can ignore recommendations and do whatever
he wants in the call-back. We can't control it.
If he touches some memory in it - probably there will be more spurious wakeups and less power saves. In principle it is the same with all other PMD dev-ops - we have to trust that they are doing what they have to.

I did a quick prototype for this, and i don't think it is going to work.

Callbacks with just "current value" as argument will be pretty limited and will only really work for cases where we know what we are expecting. However, for cases like event/dlb or net/mlx5, the expected value is (or appears to be) dependent upon some internal device data, and is not constant like in case of net/ixgbe for example.

This can be fixed by passing an opaque pointer, either by storing it in the monitor condition, or by passing it directly to rte_power_monitor at invocation time.

The latter doesn't work well because when we call rte_power_monitor from inside the rte_power library, we lack the context necessary to get said opaque pointer.

The former doesn't work either, because the only place where we can get this argument is inside get_monitor_addr, but the opaque pointer must persist after we exit that function in order to avoid use-after-free - which means that it either has to be statically allocated (which means it's not thread-safe for a non-trivial case), or dynamically allocated (which a big no-no on a hotpath).

Any other suggestions? :)

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

I'm not
opposed to having a callback here, but maybe others have more thoughts on this?

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

--
Thanks,
Anatoly

--
Thanks,
Anatoly

--
Thanks,
Anatoly

Quoted 130 lines · Konstantin

Quoted 106 lines

Quoted 98 lines

Quoted 82 lines

Quoted 75 lines

Quoted 67 lines

Quoted 43 lines

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 <[email protected]>

---
     lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
     lib/eal/x86/rte_power_intrinsics.c             | 5 ++++-
     2 files changed, 8 insertions(+), 1 deletion(-)

lib/eal/include/generic/rte_power_intrinsics.h +4 -0

index dddca3d41c..1006c2edfc 100644
@@ -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)
+                       */
     };
 
     /**

lib/eal/x86/rte_power_intrinsics.c +0 -0

index 39ea9fdecd..5d944e9aa4 100644
@@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
                  const uint64_t masked = cur_value & pmc->mask;

lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 126

Quoted 10 lines

lib/eal/x86/rte_power_intrinsics.c +4 -1

 
                  /* 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;
};

Quoted 7 lines

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;

....

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 4 lines

It's going to be especially "fun" to do these indirect function calls from inside transactional region on call to multi-monitor.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

Yep, PMD developer can ignore recommendations and do whatever
he wants in the call-back. We can't control it.
If he touches some memory in it - probably there will be more spurious wakeups and less power saves. In principle it is the same with all other PMD dev-ops - we have to trust that they are doing what they have to.

I did a quick prototype for this, and i don't think it is going to work.

Callbacks with just "current value" as argument will be pretty limited and will only really work for cases where we know what we are expecting. However, for cases like event/dlb or net/mlx5, the expected value is (or appears to be) dependent upon some internal device data, and is not constant like in case of net/ixgbe for example.

This can be fixed by passing an opaque pointer, either by storing it in the monitor condition, or by passing it directly to rte_power_monitor at invocation time.

The latter doesn't work well because when we call rte_power_monitor from inside the rte_power library, we lack the context necessary to get said opaque pointer.

Quoted 6 lines

The former doesn't work either, because the only place where we can get this argument is inside get_monitor_addr, but the opaque pointer must persist after we exit that function in order to avoid use-after-free - which means that it either has to be statically allocated (which means it's not thread-safe for a non-trivial case), or dynamically allocated (which a big no-no on a hotpath).

If I get you right, expected_value (and probably mask) can be variable ones. So for callback approach to work we need to pass all this as parameters to PMD comparison callback:
int pmc_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask) Correct?

Quoted 24 lines

Any other suggestions? :)

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

I'm not
opposed to having a callback here, but maybe others have more thoughts on this?

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

--
Thanks,
Anatoly

--
Thanks,
Anatoly

--
Thanks,
Anatoly

Quoted 135 lines

Quoted 128 lines

Quoted 106 lines

Quoted 98 lines

Quoted 82 lines

Quoted 75 lines

Quoted 67 lines

Quoted 43 lines

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 <[email protected]>

---
     lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++
     lib/eal/x86/rte_power_intrinsics.c             | 5 ++++-
     2 files changed, 8 insertions(+), 1 deletion(-)

lib/eal/include/generic/rte_power_intrinsics.h +4 -0

index dddca3d41c..1006c2edfc 100644
@@ -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)
+                       */
     };
 
     /**

lib/eal/x86/rte_power_intrinsics.c +0 -0

index 39ea9fdecd..5d944e9aa4 100644
@@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
                  const uint64_t masked = cur_value & pmc->mask;

lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 126

Quoted 10 lines

lib/eal/x86/rte_power_intrinsics.c +4 -1

 
                  /* 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;
};

Quoted 7 lines

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;

....

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 4 lines

It's going to be especially "fun" to do these indirect function calls from inside transactional region on call to multi-monitor.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

Yep, PMD developer can ignore recommendations and do whatever
he wants in the call-back. We can't control it.
If he touches some memory in it - probably there will be more spurious wakeups and less power saves. In principle it is the same with all other PMD dev-ops - we have to trust that they are doing what they have to.

I did a quick prototype for this, and i don't think it is going to work.

Callbacks with just "current value" as argument will be pretty limited and will only really work for cases where we know what we are expecting. However, for cases like event/dlb or net/mlx5, the expected value is (or appears to be) dependent upon some internal device data, and is not constant like in case of net/ixgbe for example.

This can be fixed by passing an opaque pointer, either by storing it in the monitor condition, or by passing it directly to rte_power_monitor at invocation time.

The latter doesn't work well because when we call rte_power_monitor from inside the rte_power library, we lack the context necessary to get said opaque pointer.

Quoted 6 lines

The former doesn't work either, because the only place where we can get this argument is inside get_monitor_addr, but the opaque pointer must persist after we exit that function in order to avoid use-after-free - which means that it either has to be statically allocated (which means it's not thread-safe for a non-trivial case), or dynamically allocated (which a big no-no on a hotpath).

Quoted 5 lines

If I get you right, expected_value (and probably mask) can be variable ones. So for callback approach to work we need to pass all this as parameters to PMD comparison callback:
int pmc_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask) Correct?

If we have both expected value, mask, and current value, then what's the point of the callback? The point of the callback would be to pass just the current value, and let the callback decide what's the expected value and how to compare it.

So, we can either let callback handle expected values itself by having an opaque callback-specific argument (which means it has to persist between .get_monitor_addr() and rte_power_monitor() calls),

or we do the
comparisons inside rte_power_monitor(), and store the expected/mask values in the monitor condition, and *don't* have any callbacks at all. Are you suggesting an alternative to the above two options?

Quoted 17 lines

Quoted 17 lines

Any other suggestions? :)

Quoted 12 lines

Quoted 12 lines

Quoted 10 lines

Quoted 10 lines

I'm not
opposed to having a callback here, but maybe others have more thoughts on this?

--
Thanks,
Anatoly

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

--
Thanks,
Anatoly

--
Thanks,
Anatoly

--
Thanks,
Anatoly

Quoted 29 lines

Quoted 23 lines

Quoted 16 lines

I did a quick prototype for this, and i don't think it is going to work.

Callbacks with just "current value" as argument will be pretty limited and will only really work for cases where we know what we are expecting. However, for cases like event/dlb or net/mlx5, the expected value is (or appears to be) dependent upon some internal device data, and is not constant like in case of net/ixgbe for example.

This can be fixed by passing an opaque pointer, either by storing it in the monitor condition, or by passing it directly to rte_power_monitor at invocation time.

The latter doesn't work well because when we call rte_power_monitor from inside the rte_power library, we lack the context necessary to get said opaque pointer.

Quoted 6 lines

The former doesn't work either, because the only place where we can get this argument is inside get_monitor_addr, but the opaque pointer must persist after we exit that function in order to avoid use-after-free - which means that it either has to be statically allocated (which means it's not thread-safe for a non-trivial case), or dynamically allocated (which a big no-no on a hotpath).

Quoted 5 lines

If I get you right, expected_value (and probably mask) can be variable ones. So for callback approach to work we need to pass all this as parameters to PMD comparison callback:
int pmc_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask) Correct?

Quoted 4 lines

If we have both expected value, mask, and current value, then what's the point of the callback? The point of the callback would be to pass just the current value, and let the callback decide what's the expected value and how to compare it.

For me the main point of callback is to hide PMD specific comparison semantics. Basically they provide us with some values in struct rte_power_monitor_cond, and then it is up to them how to interpret them in their comparison function. All we'll do for them: will read the value at address provided.
I understand that it looks like an overkill, as majority of these comparison functions will be like:
int cmp_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask)

{
         return ((real_val & mask) == expected_val);
}

Though qsort() and bsearch() work in a similar manner, and everyone seems ok with it.

Quoted 4 lines

So, we can either let callback handle expected values itself by having an opaque callback-specific argument (which means it has to persist between .get_monitor_addr() and rte_power_monitor() calls),

But that's what we doing already - PMD fills rte_power_monitor_cond values for us, we store them somewhere and then use them to decide should we go to sleep or not. All callback does - moves actual values interpretation back to PMD:
Right now:
PMD: provide PMC values
POWER: store PMC values somewhere
read the value at address provided in PMC
interpret PMC values and newly read value and make the decision

With callback:
PMD: provide PMC values
POWER: store PMC values somewhere
read the value at address provided in PMC
PMD: interpret PMC values and newly read value and make the decision

Or did you mean something different here?

Quoted 4 lines

or we do the
comparisons inside rte_power_monitor(), and store the expected/mask values in the monitor condition, and *don't* have any callbacks at all. Are you suggesting an alternative to the above two options?

As I said in my first mail - we can just replace 'inverse' with 'op'. That at least will make this API extendable, if someone will need something different in future.

Another option is

Quoted 75 lines

Quoted 31 lines

Quoted 25 lines

Quoted 18 lines

I did a quick prototype for this, and i don't think it is going to work.

Callbacks with just "current value" as argument will be pretty limited and will only really work for cases where we know what we are expecting. However, for cases like event/dlb or net/mlx5, the expected value is (or appears to be) dependent upon some internal device data, and is not constant like in case of net/ixgbe for example.

This can be fixed by passing an opaque pointer, either by storing it in the monitor condition, or by passing it directly to rte_power_monitor at invocation time.

The latter doesn't work well because when we call rte_power_monitor from inside the rte_power library, we lack the context necessary to get said opaque pointer.

Quoted 6 lines

The former doesn't work either, because the only place where we can get this argument is inside get_monitor_addr, but the opaque pointer must persist after we exit that function in order to avoid use-after-free - which means that it either has to be statically allocated (which means it's not thread-safe for a non-trivial case), or dynamically allocated (which a big no-no on a hotpath).

Quoted 5 lines

If I get you right, expected_value (and probably mask) can be variable ones. So for callback approach to work we need to pass all this as parameters to PMD comparison callback:
int pmc_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask) Correct?

Quoted 4 lines

If we have both expected value, mask, and current value, then what's the point of the callback? The point of the callback would be to pass just the current value, and let the callback decide what's the expected value and how to compare it.

For me the main point of callback is to hide PMD specific comparison semantics. Basically they provide us with some values in struct rte_power_monitor_cond, and then it is up to them how to interpret them in their comparison function. All we'll do for them: will read the value at address provided.
I understand that it looks like an overkill, as majority of these comparison functions will be like:
int cmp_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask)

{
         return ((real_val & mask) == expected_val);
}

Though qsort() and bsearch() work in a similar manner, and everyone seems ok with it.

Quoted 4 lines

So, we can either let callback handle expected values itself by having an opaque callback-specific argument (which means it has to persist between .get_monitor_addr() and rte_power_monitor() calls),

But that's what we doing already - PMD fills rte_power_monitor_cond values for us, we store them somewhere and then use them to decide should we go to sleep or not. All callback does - moves actual values interpretation back to PMD:
Right now:
PMD: provide PMC values
POWER: store PMC values somewhere
read the value at address provided in PMC
interpret PMC values and newly read value and make the decision

With callback:
PMD: provide PMC values
POWER: store PMC values somewhere
read the value at address provided in PMC
PMD: interpret PMC values and newly read value and make the decision

Or did you mean something different here?

Quoted 4 lines

or we do the
comparisons inside rte_power_monitor(), and store the expected/mask values in the monitor condition, and *don't* have any callbacks at all. Are you suggesting an alternative to the above two options?

Quoted 5 lines

As I said in my first mail - we can just replace 'inverse' with 'op'. That at least will make this API extendable, if someone will need something different in future.

Another option is

Right, so the idea is store the PMD-specific data in the monitor condition, and leave it to the callback to interpret it.

The obvious question then is, how many values is enough? Two? Three? Four? This option doesn't really solve the basic issue, it just kicks the can down the road. I guess three values should be enough for everyone (tm) ? :D

I don't like the 'op' thing because if the goal is to be flexible, it's unnecessarily limiting *and* makes the API even more complex to use. I would rather have a number of PMD-specific values and leave it up to the callback to interpret them, because at least that way we're not limited to predefined operations on the monitor condition data.

Quoted 89 lines

Quoted 75 lines

Quoted 31 lines

Quoted 25 lines

Quoted 18 lines

I did a quick prototype for this, and i don't think it is going to work.

Callbacks with just "current value" as argument will be pretty limited and will only really work for cases where we know what we are expecting. However, for cases like event/dlb or net/mlx5, the expected value is (or appears to be) dependent upon some internal device data, and is not constant like in case of net/ixgbe for example.

This can be fixed by passing an opaque pointer, either by storing it in the monitor condition, or by passing it directly to rte_power_monitor at invocation time.

The latter doesn't work well because when we call rte_power_monitor from inside the rte_power library, we lack the context necessary to get said opaque pointer.

Quoted 6 lines

The former doesn't work either, because the only place where we can get this argument is inside get_monitor_addr, but the opaque pointer must persist after we exit that function in order to avoid use-after-free - which means that it either has to be statically allocated (which means it's not thread-safe for a non-trivial case), or dynamically allocated (which a big no-no on a hotpath).

Quoted 5 lines

If I get you right, expected_value (and probably mask) can be variable ones. So for callback approach to work we need to pass all this as parameters to PMD comparison callback:
int pmc_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask) Correct?

Quoted 4 lines

If we have both expected value, mask, and current value, then what's the point of the callback? The point of the callback would be to pass just the current value, and let the callback decide what's the expected value and how to compare it.

For me the main point of callback is to hide PMD specific comparison semantics. Basically they provide us with some values in struct rte_power_monitor_cond, and then it is up to them how to interpret them in their comparison function. All we'll do for them: will read the value at address provided.
I understand that it looks like an overkill, as majority of these comparison functions will be like:
int cmp_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask)

{
          return ((real_val & mask) == expected_val);
}

Though qsort() and bsearch() work in a similar manner, and everyone seems ok with it.

Quoted 4 lines

So, we can either let callback handle expected values itself by having an opaque callback-specific argument (which means it has to persist between .get_monitor_addr() and rte_power_monitor() calls),

But that's what we doing already - PMD fills rte_power_monitor_cond values for us, we store them somewhere and then use them to decide should we go to sleep or not. All callback does - moves actual values interpretation back to PMD:
Right now:
PMD: provide PMC values
POWER: store PMC values somewhere
read the value at address provided in PMC
interpret PMC values and newly read value and make the decision

With callback:
PMD: provide PMC values
POWER: store PMC values somewhere
read the value at address provided in PMC
PMD: interpret PMC values and newly read value and make the decision

Or did you mean something different here?

Quoted 4 lines

or we do the
comparisons inside rte_power_monitor(), and store the expected/mask values in the monitor condition, and *don't* have any callbacks at all. Are you suggesting an alternative to the above two options?

Quoted 5 lines

As I said in my first mail - we can just replace 'inverse' with 'op'. That at least will make this API extendable, if someone will need something different in future.

Another option is

Right, so the idea is store the PMD-specific data in the monitor condition, and leave it to the callback to interpret it.

The obvious question then is, how many values is enough? Two? Three? Four? This option doesn't really solve the basic issue, it just kicks the can down the road. I guess three values should be enough for everyone (tm) ? :D

Quoted 5 lines

I don't like the 'op' thing because if the goal is to be flexible, it's unnecessarily limiting *and* makes the API even more complex to use. I would rather have a number of PMD-specific values and leave it up to the callback to interpret them, because at least that way we're not limited to predefined operations on the monitor condition data.

Just to make sure we are talking about the same, does what you propose looks like that:

struct rte_power_monitor_cond {
volatile void *addr; /**< Address to monitor for changes */
uint8_t size; /**< Data size (in bytes) that will be used to compare
* expected value (val) with data read from the
* monitored memory location (addr). Can be 1, 2,
* 4, or 8. Supplying any other value will result in
* an error.
*/
int (*cmp)(uint64_t real_value, const uint64_t opaque[4]); uint64_t opaque[4]; /*PMD specific data, used by comparison call-back below */
};

And then in rte_power_monitor():
...
uint64_t cur_value = __get_umwait_val(pmc->addr, pmc->size);

if (pmc->cmp(cur_value, pmc->opaque) != 0) {
     /* goto sleep */
}

?

Quoted 116 lines

Quoted 88 lines

Quoted 74 lines

Quoted 30 lines

Quoted 24 lines

Quoted 17 lines

I did a quick prototype for this, and i don't think it is going to work.

Callbacks with just "current value" as argument will be pretty limited and will only really work for cases where we know what we are expecting. However, for cases like event/dlb or net/mlx5, the expected value is (or appears to be) dependent upon some internal device data, and is not constant like in case of net/ixgbe for example.

This can be fixed by passing an opaque pointer, either by storing it in the monitor condition, or by passing it directly to rte_power_monitor at invocation time.

The latter doesn't work well because when we call rte_power_monitor from inside the rte_power library, we lack the context necessary to get said opaque pointer.

Quoted 6 lines

The former doesn't work either, because the only place where we can get this argument is inside get_monitor_addr, but the opaque pointer must persist after we exit that function in order to avoid use-after-free - which means that it either has to be statically allocated (which means it's not thread-safe for a non-trivial case), or dynamically allocated (which a big no-no on a hotpath).

Quoted 5 lines

If I get you right, expected_value (and probably mask) can be variable ones. So for callback approach to work we need to pass all this as parameters to PMD comparison callback:
int pmc_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask) Correct?

Quoted 4 lines

If we have both expected value, mask, and current value, then what's the point of the callback? The point of the callback would be to pass just the current value, and let the callback decide what's the expected value and how to compare it.

For me the main point of callback is to hide PMD specific comparison semantics. Basically they provide us with some values in struct rte_power_monitor_cond, and then it is up to them how to interpret them in their comparison function. All we'll do for them: will read the value at address provided.
I understand that it looks like an overkill, as majority of these comparison functions will be like:
int cmp_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask)

{
          return ((real_val & mask) == expected_val);
}

Though qsort() and bsearch() work in a similar manner, and everyone seems ok with it.

Quoted 4 lines

So, we can either let callback handle expected values itself by having an opaque callback-specific argument (which means it has to persist between .get_monitor_addr() and rte_power_monitor() calls),

But that's what we doing already - PMD fills rte_power_monitor_cond values for us, we store them somewhere and then use them to decide should we go to sleep or not. All callback does - moves actual values interpretation back to PMD:
Right now:
PMD: provide PMC values
POWER: store PMC values somewhere
read the value at address provided in PMC
interpret PMC values and newly read value and make the decision

With callback:
PMD: provide PMC values
POWER: store PMC values somewhere
read the value at address provided in PMC
PMD: interpret PMC values and newly read value and make the decision

Or did you mean something different here?

Quoted 4 lines

or we do the
comparisons inside rte_power_monitor(), and store the expected/mask values in the monitor condition, and *don't* have any callbacks at all. Are you suggesting an alternative to the above two options?

Quoted 5 lines

As I said in my first mail - we can just replace 'inverse' with 'op'. That at least will make this API extendable, if someone will need something different in future.

Another option is

Right, so the idea is store the PMD-specific data in the monitor condition, and leave it to the callback to interpret it.

The obvious question then is, how many values is enough? Two? Three? Four? This option doesn't really solve the basic issue, it just kicks the can down the road. I guess three values should be enough for everyone (tm) ? :D

Quoted 5 lines

I don't like the 'op' thing because if the goal is to be flexible, it's unnecessarily limiting *and* makes the API even more complex to use. I would rather have a number of PMD-specific values and leave it up to the callback to interpret them, because at least that way we're not limited to predefined operations on the monitor condition data.

Just to make sure we are talking about the same, does what you propose looks like that:

struct rte_power_monitor_cond {
volatile void *addr; /**< Address to monitor for changes */
uint8_t size; /**< Data size (in bytes) that will be used to compare
* expected value (val) with data read from the
* monitored memory location (addr). Can be 1, 2,
* 4, or 8. Supplying any other value will result in
* an error.
*/
int (*cmp)(uint64_t real_value, const uint64_t opaque[4]); uint64_t opaque[4]; /*PMD specific data, used by comparison call-back below */
};

And then in rte_power_monitor():
...
uint64_t cur_value = __get_umwait_val(pmc->addr, pmc->size);

if (pmc->cmp(cur_value, pmc->opaque) != 0) {
     /* goto sleep */
}

Quoted 2 lines

?

Something like that, yes.

Quoted 119 lines

Quoted 118 lines

Quoted 88 lines

Quoted 74 lines

Quoted 30 lines

Quoted 24 lines

Quoted 17 lines

I did a quick prototype for this, and i don't think it is going to work.

Callbacks with just "current value" as argument will be pretty limited and will only really work for cases where we know what we are expecting. However, for cases like event/dlb or net/mlx5, the expected value is (or appears to be) dependent upon some internal device data, and is not constant like in case of net/ixgbe for example.

This can be fixed by passing an opaque pointer, either by storing it in the monitor condition, or by passing it directly to rte_power_monitor at invocation time.

The latter doesn't work well because when we call rte_power_monitor from inside the rte_power library, we lack the context necessary to get said opaque pointer.

Quoted 6 lines

The former doesn't work either, because the only place where we can get this argument is inside get_monitor_addr, but the opaque pointer must persist after we exit that function in order to avoid use-after-free - which means that it either has to be statically allocated (which means it's not thread-safe for a non-trivial case), or dynamically allocated (which a big no-no on a hotpath).

Quoted 5 lines

If I get you right, expected_value (and probably mask) can be variable ones. So for callback approach to work we need to pass all this as parameters to PMD comparison callback:
int pmc_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask) Correct?

Quoted 4 lines

If we have both expected value, mask, and current value, then what's the point of the callback? The point of the callback would be to pass just the current value, and let the callback decide what's the expected value and how to compare it.

For me the main point of callback is to hide PMD specific comparison semantics. Basically they provide us with some values in struct rte_power_monitor_cond, and then it is up to them how to interpret them in their comparison function. All we'll do for them: will read the value at address provided.
I understand that it looks like an overkill, as majority of these comparison functions will be like:
int cmp_callback(uint64_t real_val, uint64_t expected_val, uint64_t mask)

{
           return ((real_val & mask) == expected_val);
}

Though qsort() and bsearch() work in a similar manner, and everyone seems ok with it.

Quoted 4 lines

So, we can either let callback handle expected values itself by having an opaque callback-specific argument (which means it has to persist between .get_monitor_addr() and rte_power_monitor() calls),

But that's what we doing already - PMD fills rte_power_monitor_cond values for us, we store them somewhere and then use them to decide should we go to sleep or not. All callback does - moves actual values interpretation back to PMD:
Right now:
PMD: provide PMC values
POWER: store PMC values somewhere
read the value at address provided in PMC
interpret PMC values and newly read value and make the decision

With callback:
PMD: provide PMC values
POWER: store PMC values somewhere
read the value at address provided in PMC
PMD: interpret PMC values and newly read value and make the decision

Or did you mean something different here?

Quoted 4 lines

or we do the
comparisons inside rte_power_monitor(), and store the expected/mask values in the monitor condition, and *don't* have any callbacks at all. Are you suggesting an alternative to the above two options?

Quoted 5 lines

As I said in my first mail - we can just replace 'inverse' with 'op'. That at least will make this API extendable, if someone will need something different in future.

Another option is

Right, so the idea is store the PMD-specific data in the monitor condition, and leave it to the callback to interpret it.

The obvious question then is, how many values is enough? Two? Three? Four? This option doesn't really solve the basic issue, it just kicks the can down the road. I guess three values should be enough for everyone (tm) ? :D

Quoted 5 lines

I don't like the 'op' thing because if the goal is to be flexible, it's unnecessarily limiting *and* makes the API even more complex to use. I would rather have a number of PMD-specific values and leave it up to the callback to interpret them, because at least that way we're not limited to predefined operations on the monitor condition data.

Just to make sure we are talking about the same, does what you propose looks like that:

struct rte_power_monitor_cond {
volatile void *addr; /**< Address to monitor for changes */
uint8_t size; /**< Data size (in bytes) that will be used to compare
* expected value (val) with data read from the
* monitored memory location (addr). Can be 1, 2,
* 4, or 8. Supplying any other value will result in
* an error.
*/
int (*cmp)(uint64_t real_value, const uint64_t opaque[4]); uint64_t opaque[4]; /*PMD specific data, used by comparison call-back below */
};

And then in rte_power_monitor():
...
uint64_t cur_value = __get_umwait_val(pmc->addr, pmc->size);

if (pmc->cmp(cur_value, pmc->opaque) != 0) {
      /* goto sleep */
}

?

Quoted 2 lines

Something like that, yes.

Seems reasonable to me.
Thanks
Konstantin

Right, so the idea is store the PMD-specific data in the monitor condition, and leave it to the callback to interpret it.

The obvious question then is, how many values is enough? Two? Three? Four? This option doesn't really solve the basic issue, it just kicks the can down the road. I guess three values should be enough for everyone (tm) ? :D

Subject: [PATCH v1 2/7] net/af_xdp: add power monitor support

Implement support for .get_monitor_addr in AF_XDP driver.

Signed-off-by Anatoly Burakov <[email protected]>

---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

drivers/net/af_xdp/rte_eth_af_xdp.c +24 -0

index eb5660a3dc..dfbf74ea53 100644
@@ -37,6 +37,7 @@
 #include <rte_malloc.h>
 #include <rte_ring.h>
 #include <rte_spinlock.h>
+#include <rte_power_intrinsics.h>
 
 #include "compat.h"
 

@@ -788,6 +789,29 @@ eth_dev_configure(struct rte_eth_dev *dev)
 	return 0;
 }
 
+static int
+eth_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond
 *pmc)
+{
+	struct pkt_rx_queue *rxq = rx_queue;
+	unsigned int *prod = rxq->rx.producer;
+	const uint32_t cur_val = rxq->rx.cached_prod; /* use cached value */
+
+	/* watch for changes in producer ring */
+	pmc->addr = (void*)prod;
+
+	/* store current value */
+	pmc->val = cur_val;
+	pmc->mask = (uint32_t)~0; /* mask entire uint32_t value */
+
+	/* AF_XDP producer ring index is 32-bit */
+	pmc->size = sizeof(uint32_t);
+
+	/* this requires an inverted check */
+	pmc->invert = 1;
+
+	return 0;
+}
+
 static int
 eth_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)

{

drivers/net/af_xdp/rte_eth_af_xdp.c +1 -1

@@ -1448,6 +1472,7 @@ static const struct eth_dev_ops ops = {
 	.link_update = eth_link_update,
 	.stats_get = eth_stats_get,
 	.stats_reset = eth_stats_reset,
+	.get_monitor_addr = eth_get_monitor_addr
 };
 
 /** parse busy_budget argument */
--

2.25.1

Currently, we expect that only one callback can be active at any given moment, for a particular queue configuration, which is relatively easy to implement in a thread-safe way. However, we're about to add support for multiple queues per lcore, which will greatly increase the possibility of various race conditions.

We could have used something like an RCU for this use case, but absent of a pressing need for thread safety we'll go the easy way and just mandate that the API's are to be called when all affected ports are stopped, and document this limitation. This greatly simplifies the rte_power_monitor-related code.

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/power/meson.build          |   3 +
 lib/power/rte_power_pmd_mgmt.c | 106 ++++++++-------------------------
 lib/power/rte_power_pmd_mgmt.h |   6 ++
 3 files changed, 35 insertions(+), 80 deletions(-)

lib/power/meson.build +3 -0

index c1097d32f1..4f6a242364 100644
@@ -21,4 +21,7 @@ headers = files(
         'rte_power_pmd_mgmt.h',
         'rte_power_guest_channel.h',
 )
+if cc.has_argument('-Wno-cast-qual')
+    cflags += '-Wno-cast-qual'
+endif
 deps += ['timer', 'ethdev']

lib/power/rte_power_pmd_mgmt.c +26 -80

index db03cbf420..0707c60a4f 100644
@@ -40,8 +40,6 @@ struct pmd_queue_cfg {
 	/**< Callback mode for this queue */
 	const struct rte_eth_rxtx_callback *cur_cb;
 	/**< Callback instance */
-	volatile bool umwait_in_progress;
-	/**< are we currently sleeping? */
 	uint64_t empty_poll_stats;
 	/**< Number of empty polls */
 } __rte_cache_aligned;

@@ -92,30 +90,11 @@ clb_umwait(uint16_t port_id, uint16_t qidx, struct rte_mbuf **pkts __rte_unused,
 			struct rte_power_monitor_cond pmc;
 			uint16_t ret;
 
-			/*
-			 * we might get a cancellation request while being
-			 * inside the callback, in which case the wakeup
-			 * wouldn't work because it would've arrived too early.
-			 *
-			 * to get around this, we notify the other thread that
-			 * we're sleeping, so that it can spin until we're done.
-			 * unsolicited wakeups are perfectly safe.
-			 */
-			q_conf->umwait_in_progress = true;
-
-			rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-			/* check if we need to cancel sleep */
-			if (q_conf->pwr_mgmt_state == PMD_MGMT_ENABLED) {
-				/* use monitoring condition to sleep */
-				ret = rte_eth_get_monitor_addr(port_id, qidx,
-						&pmc);
-				if (ret == 0)
-					rte_power_monitor(&pmc, UINT64_MAX);
-			}
-			q_conf->umwait_in_progress = false;
-
-			rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
+			/* use monitoring condition to sleep */
+			ret = rte_eth_get_monitor_addr(port_id, qidx,
+					&pmc);
+			if (ret == 0)
+				rte_power_monitor(&pmc, UINT64_MAX);
 		}
 	} else
 		q_conf->empty_poll_stats = 0;

@@ -183,6 +162,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
 {
 	struct pmd_queue_cfg *queue_cfg;
 	struct rte_eth_dev_info info;
+	rte_rx_callback_fn clb;
 	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);

@@ -232,17 +212,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
 			ret = -ENOTSUP;
 			goto end;
 		}
-		/* initialize data before enabling the callback */
-		queue_cfg->empty_poll_stats = 0;
-		queue_cfg->cb_mode = mode;
-		queue_cfg->umwait_in_progress = false;
-		queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
-
-		/* ensure we update our state before callback starts */
-		rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-		queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,
-				clb_umwait, NULL);
+		clb = clb_umwait;
 		break;
 	}
 	case RTE_POWER_MGMT_TYPE_SCALE:

@@ -269,16 +239,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
 			ret = -ENOTSUP;
 			goto end;
 		}
-		/* initialize data before enabling the callback */
-		queue_cfg->empty_poll_stats = 0;
-		queue_cfg->cb_mode = mode;
-		queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
-
-		/* this is not necessary here, but do it anyway */
-		rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-		queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id,
-				queue_id, clb_scale_freq, NULL);
+		clb = clb_scale_freq;
 		break;
 	}
 	case RTE_POWER_MGMT_TYPE_PAUSE:

@@ -286,18 +247,21 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
 		if (global_data.tsc_per_us == 0)
 			calc_tsc();
 
-		/* initialize data before enabling the callback */
-		queue_cfg->empty_poll_stats = 0;
-		queue_cfg->cb_mode = mode;
-		queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
-
-		/* this is not necessary here, but do it anyway */
-		rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-		queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,
-				clb_pause, NULL);
+		clb = clb_pause;
 		break;
+	default:
+		RTE_LOG(DEBUG, POWER, "Invalid power management type\n");
+		ret = -EINVAL;
+		goto end;
 	}
+
+	/* initialize data before enabling the callback */
+	queue_cfg->empty_poll_stats = 0;
+	queue_cfg->cb_mode = mode;
+	queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
+	queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,
+			clb, NULL);
+
 	ret = 0;
 end:
 	return ret;

@@ -323,27 +287,8 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,
 	/* stop any callbacks from progressing */
 	queue_cfg->pwr_mgmt_state = PMD_MGMT_DISABLED;
 
-	/* ensure we update our state before continuing */
-	rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
 	switch (queue_cfg->cb_mode) {
-	case RTE_POWER_MGMT_TYPE_MONITOR:
-	{
-		bool exit = false;
-		do {
-			/*
-			 * we may request cancellation while the other thread
-			 * has just entered the callback but hasn't started
-			 * sleeping yet, so keep waking it up until we know it's
-			 * done sleeping.
-			 */
-			if (queue_cfg->umwait_in_progress)
-				rte_power_monitor_wakeup(lcore_id);
-			else
-				exit = true;
-		} while (!exit);
-	}
-	/* fall-through */
+	case RTE_POWER_MGMT_TYPE_MONITOR: /* fall-through */
 	case RTE_POWER_MGMT_TYPE_PAUSE:
 		rte_eth_remove_rx_callback(port_id, queue_id,
 				queue_cfg->cur_cb);

@@ -356,10 +301,11 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,
 		break;
 	}
 	/*
-	 * we don't free the RX callback here because it is unsafe to do so
-	 * unless we know for a fact that all data plane threads have stopped.
+	 * the API doc mandates that the user stops all processing on affected
+	 * ports before calling any of these API's, so we can assume that the
+	 * callbacks can be freed. we're intentionally casting away const-ness.
 	 */
-	queue_cfg->cur_cb = NULL;
+	rte_free((void *)queue_cfg->cur_cb);
 
 	return 0;
 }

lib/power/rte_power_pmd_mgmt.h +6 -1

index 7a0ac24625..7557f5d7e1 100644
@@ -43,6 +43,9 @@ enum rte_power_pmd_mgmt_type {
  *
  * @note This function is not thread-safe.
  *
+ * @warning This function must be called when all affected Ethernet ports are
+ *   stopped and no Rx/Tx is in progress!
+ *
  * @param lcore_id
  *   The lcore the Rx queue will be polled from.
  * @param port_id

@@ -69,6 +72,9 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id,
  *
  * @note This function is not thread-safe.
  *
+ * @warning This function must be called when all affected Ethernet ports are
+ *   stopped and no Rx/Tx is in progress!
+ *
  * @param lcore_id
  *   The lcore the Rx queue is polled from.
  * @param port_id
--

2.25.1

Quoted 7 lines

Currently, we expect that only one callback can be active at any given moment, for a particular queue configuration, which is relatively easy to implement in a thread-safe way. However, we're about to add support for multiple queues per lcore, which will greatly increase the possibility of various race conditions.

Quoted 5 lines

We could have used something like an RCU for this use case, but absent of a pressing need for thread safety we'll go the easy way and just mandate that the API's are to be called when all affected ports are stopped, and document this limitation. This greatly simplifies the rte_power_monitor-related code.

I think you need to update RN too with that.

Another thing - do you really need the whole port stopped?
From what I understand - you work on queues, so it is enough for you
that related RX queue is stopped.
So, to make things a bit more robust, in pmgmt_queue_enable/disable you can call rte_eth_rx_queue_info_get() and check queue state.

Quoted 216 lines

Signed-off-by Anatoly Burakov <[email protected]>

---
  lib/power/meson.build          |   3 +
  lib/power/rte_power_pmd_mgmt.c | 106 ++++++++-------------------------
  lib/power/rte_power_pmd_mgmt.h |   6 ++
  3 files changed, 35 insertions(+), 80 deletions(-)

lib/power/meson.build +3 -0

index c1097d32f1..4f6a242364 100644
@@ -21,4 +21,7 @@ headers = files(
          'rte_power_pmd_mgmt.h',
          'rte_power_guest_channel.h',
  )
+if cc.has_argument('-Wno-cast-qual')
+    cflags += '-Wno-cast-qual'
+endif
  deps += ['timer', 'ethdev']

lib/power/rte_power_pmd_mgmt.c +26 -80

index db03cbf420..0707c60a4f 100644
@@ -40,8 +40,6 @@ struct pmd_queue_cfg {
       /**< Callback mode for this queue */
       const struct rte_eth_rxtx_callback *cur_cb;
       /**< Callback instance */
-     volatile bool umwait_in_progress;
-     /**< are we currently sleeping? */
       uint64_t empty_poll_stats;
       /**< Number of empty polls */
  } __rte_cache_aligned;

@@ -92,30 +90,11 @@ clb_umwait(uint16_t port_id, uint16_t qidx, struct rte_mbuf **pkts __rte_unused,
                       struct rte_power_monitor_cond pmc;
                       uint16_t ret;
 
-                     /*
-                      * we might get a cancellation request while being
-                      * inside the callback, in which case the wakeup
-                      * wouldn't work because it would've arrived too early.
-                      *
-                      * to get around this, we notify the other thread that
-                      * we're sleeping, so that it can spin until we're done.
-                      * unsolicited wakeups are perfectly safe.
-                      */
-                     q_conf->umwait_in_progress = true;
-
-                     rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-                     /* check if we need to cancel sleep */
-                     if (q_conf->pwr_mgmt_state == PMD_MGMT_ENABLED) {
-                             /* use monitoring condition to sleep */
-                             ret = rte_eth_get_monitor_addr(port_id, qidx,
-                                             &pmc);
-                             if (ret == 0)
-                                     rte_power_monitor(&pmc, UINT64_MAX);
-                     }
-                     q_conf->umwait_in_progress = false;
-
-                     rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
+                     /* use monitoring condition to sleep */
+                     ret = rte_eth_get_monitor_addr(port_id, qidx,
+                                     &pmc);
+                     if (ret == 0)
+                             rte_power_monitor(&pmc, UINT64_MAX);
               }
       } else
               q_conf->empty_poll_stats = 0;

@@ -183,6 +162,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
  {
       struct pmd_queue_cfg *queue_cfg;
       struct rte_eth_dev_info info;
+     rte_rx_callback_fn clb;
       int ret;
 
       RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);

@@ -232,17 +212,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
                       ret = -ENOTSUP;
                       goto end;
               }
-             /* initialize data before enabling the callback */
-             queue_cfg->empty_poll_stats = 0;
-             queue_cfg->cb_mode = mode;
-             queue_cfg->umwait_in_progress = false;
-             queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
-
-             /* ensure we update our state before callback starts */
-             rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-             queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,
-                             clb_umwait, NULL);
+             clb = clb_umwait;
               break;
       }
       case RTE_POWER_MGMT_TYPE_SCALE:

@@ -269,16 +239,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
                       ret = -ENOTSUP;
                       goto end;
               }
-             /* initialize data before enabling the callback */
-             queue_cfg->empty_poll_stats = 0;
-             queue_cfg->cb_mode = mode;
-             queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
-
-             /* this is not necessary here, but do it anyway */
-             rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-             queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id,
-                             queue_id, clb_scale_freq, NULL);
+             clb = clb_scale_freq;
               break;
       }
       case RTE_POWER_MGMT_TYPE_PAUSE:

@@ -286,18 +247,21 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
               if (global_data.tsc_per_us == 0)
                       calc_tsc();
 
-             /* initialize data before enabling the callback */
-             queue_cfg->empty_poll_stats = 0;
-             queue_cfg->cb_mode = mode;
-             queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
-
-             /* this is not necessary here, but do it anyway */
-             rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-             queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,
-                             clb_pause, NULL);
+             clb = clb_pause;
               break;
+     default:
+             RTE_LOG(DEBUG, POWER, "Invalid power management type\n");
+             ret = -EINVAL;
+             goto end;
       }
+
+     /* initialize data before enabling the callback */
+     queue_cfg->empty_poll_stats = 0;
+     queue_cfg->cb_mode = mode;
+     queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
+     queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,
+                     clb, NULL);
+
       ret = 0;
  end:
       return ret;

@@ -323,27 +287,8 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,
       /* stop any callbacks from progressing */
       queue_cfg->pwr_mgmt_state = PMD_MGMT_DISABLED;
 
-     /* ensure we update our state before continuing */
-     rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
       switch (queue_cfg->cb_mode) {
-     case RTE_POWER_MGMT_TYPE_MONITOR:
-     {
-             bool exit = false;
-             do {
-                     /*
-                      * we may request cancellation while the other thread
-                      * has just entered the callback but hasn't started
-                      * sleeping yet, so keep waking it up until we know it's
-                      * done sleeping.
-                      */
-                     if (queue_cfg->umwait_in_progress)
-                             rte_power_monitor_wakeup(lcore_id);
-                     else
-                             exit = true;
-             } while (!exit);
-     }
-     /* fall-through */
+     case RTE_POWER_MGMT_TYPE_MONITOR: /* fall-through */
       case RTE_POWER_MGMT_TYPE_PAUSE:
               rte_eth_remove_rx_callback(port_id, queue_id,
                               queue_cfg->cur_cb);

@@ -356,10 +301,11 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,
               break;
       }
       /*
-      * we don't free the RX callback here because it is unsafe to do so
-      * unless we know for a fact that all data plane threads have stopped.
+      * the API doc mandates that the user stops all processing on affected
+      * ports before calling any of these API's, so we can assume that the
+      * callbacks can be freed. we're intentionally casting away const-ness.
        */
-     queue_cfg->cur_cb = NULL;
+     rte_free((void *)queue_cfg->cur_cb);
 
       return 0;
  }

lib/power/rte_power_pmd_mgmt.h +6 -1

index 7a0ac24625..7557f5d7e1 100644
@@ -43,6 +43,9 @@ enum rte_power_pmd_mgmt_type {
   *
   * @note This function is not thread-safe.
   *
+ * @warning This function must be called when all affected Ethernet ports are
+ *   stopped and no Rx/Tx is in progress!
+ *
   * @param lcore_id
   *   The lcore the Rx queue will be polled from.
   * @param port_id

@@ -69,6 +72,9 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id,
   *
   * @note This function is not thread-safe.
   *
+ * @warning This function must be called when all affected Ethernet ports are
+ *   stopped and no Rx/Tx is in progress!
+ *
   * @param lcore_id
   *   The lcore the Rx queue is polled from.
   * @param port_id
--

2.25.1

Quoted 7 lines

Quoted 7 lines

Currently, we expect that only one callback can be active at any given moment, for a particular queue configuration, which is relatively easy to implement in a thread-safe way. However, we're about to add support for multiple queues per lcore, which will greatly increase the possibility of various race conditions.

Quoted 7 lines

Quoted 5 lines

We could have used something like an RCU for this use case, but absent of a pressing need for thread safety we'll go the easy way and just mandate that the API's are to be called when all affected ports are stopped, and document this limitation. This greatly simplifies the rte_power_monitor-related code.

I think you need to update RN too with that.

Yep, will fix.

Quoted 5 lines

Another thing - do you really need the whole port stopped?
From what I understand - you work on queues, so it is enough for you
that related RX queue is stopped.
So, to make things a bit more robust, in pmgmt_queue_enable/disable you can call rte_eth_rx_queue_info_get() and check queue state.

We work on queues, but the data is per-lcore not per-queue, and it is potentially used by multiple queues, so checking one specific queue is not going to be enough. We could check all queues that were registered so far with the power library, maybe that'll work better?

Quoted 209 lines

Quoted 209 lines

Signed-off-by Anatoly Burakov <[email protected]>

---
  lib/power/meson.build          |   3 +
  lib/power/rte_power_pmd_mgmt.c | 106 ++++++++-------------------------
  lib/power/rte_power_pmd_mgmt.h |   6 ++
  3 files changed, 35 insertions(+), 80 deletions(-)

lib/power/meson.build +3 -0

index c1097d32f1..4f6a242364 100644
@@ -21,4 +21,7 @@ headers = files(
          'rte_power_pmd_mgmt.h',
          'rte_power_guest_channel.h',
  )
+if cc.has_argument('-Wno-cast-qual')
+    cflags += '-Wno-cast-qual'
+endif
  deps += ['timer', 'ethdev']

lib/power/rte_power_pmd_mgmt.c +26 -80

index db03cbf420..0707c60a4f 100644
@@ -40,8 +40,6 @@ struct pmd_queue_cfg {
       /**< Callback mode for this queue */
       const struct rte_eth_rxtx_callback *cur_cb;
       /**< Callback instance */
-     volatile bool umwait_in_progress;
-     /**< are we currently sleeping? */
       uint64_t empty_poll_stats;
       /**< Number of empty polls */
  } __rte_cache_aligned;

@@ -92,30 +90,11 @@ clb_umwait(uint16_t port_id, uint16_t qidx, struct rte_mbuf **pkts __rte_unused,
                       struct rte_power_monitor_cond pmc;
                       uint16_t ret;
 
-                     /*
-                      * we might get a cancellation request while being
-                      * inside the callback, in which case the wakeup
-                      * wouldn't work because it would've arrived too early.
-                      *
-                      * to get around this, we notify the other thread that
-                      * we're sleeping, so that it can spin until we're done.
-                      * unsolicited wakeups are perfectly safe.
-                      */
-                     q_conf->umwait_in_progress = true;
-
-                     rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-                     /* check if we need to cancel sleep */
-                     if (q_conf->pwr_mgmt_state == PMD_MGMT_ENABLED) {
-                             /* use monitoring condition to sleep */
-                             ret = rte_eth_get_monitor_addr(port_id, qidx,
-                                             &pmc);
-                             if (ret == 0)
-                                     rte_power_monitor(&pmc, UINT64_MAX);
-                     }
-                     q_conf->umwait_in_progress = false;
-
-                     rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
+                     /* use monitoring condition to sleep */
+                     ret = rte_eth_get_monitor_addr(port_id, qidx,
+                                     &pmc);
+                     if (ret == 0)
+                             rte_power_monitor(&pmc, UINT64_MAX);
               }
       } else
               q_conf->empty_poll_stats = 0;

@@ -183,6 +162,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
  {
       struct pmd_queue_cfg *queue_cfg;
       struct rte_eth_dev_info info;
+     rte_rx_callback_fn clb;
       int ret;
 
       RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);

@@ -232,17 +212,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
                       ret = -ENOTSUP;
                       goto end;
               }
-             /* initialize data before enabling the callback */
-             queue_cfg->empty_poll_stats = 0;
-             queue_cfg->cb_mode = mode;
-             queue_cfg->umwait_in_progress = false;
-             queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
-
-             /* ensure we update our state before callback starts */
-             rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-             queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,
-                             clb_umwait, NULL);
+             clb = clb_umwait;
               break;
       }
       case RTE_POWER_MGMT_TYPE_SCALE:

@@ -269,16 +239,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
                       ret = -ENOTSUP;
                       goto end;
               }
-             /* initialize data before enabling the callback */
-             queue_cfg->empty_poll_stats = 0;
-             queue_cfg->cb_mode = mode;
-             queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
-
-             /* this is not necessary here, but do it anyway */
-             rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-             queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id,
-                             queue_id, clb_scale_freq, NULL);
+             clb = clb_scale_freq;
               break;
       }
       case RTE_POWER_MGMT_TYPE_PAUSE:

@@ -286,18 +247,21 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,
               if (global_data.tsc_per_us == 0)
                       calc_tsc();
 
-             /* initialize data before enabling the callback */
-             queue_cfg->empty_poll_stats = 0;
-             queue_cfg->cb_mode = mode;
-             queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
-
-             /* this is not necessary here, but do it anyway */
-             rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
-             queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,
-                             clb_pause, NULL);
+             clb = clb_pause;
               break;
+     default:
+             RTE_LOG(DEBUG, POWER, "Invalid power management type\n");
+             ret = -EINVAL;
+             goto end;
       }
+
+     /* initialize data before enabling the callback */
+     queue_cfg->empty_poll_stats = 0;
+     queue_cfg->cb_mode = mode;
+     queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;
+     queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,
+                     clb, NULL);
+
       ret = 0;
  end:
       return ret;

@@ -323,27 +287,8 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,
       /* stop any callbacks from progressing */
       queue_cfg->pwr_mgmt_state = PMD_MGMT_DISABLED;
 
-     /* ensure we update our state before continuing */
-     rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
-
       switch (queue_cfg->cb_mode) {
-     case RTE_POWER_MGMT_TYPE_MONITOR:
-     {
-             bool exit = false;
-             do {
-                     /*
-                      * we may request cancellation while the other thread
-                      * has just entered the callback but hasn't started
-                      * sleeping yet, so keep waking it up until we know it's
-                      * done sleeping.
-                      */
-                     if (queue_cfg->umwait_in_progress)
-                             rte_power_monitor_wakeup(lcore_id);
-                     else
-                             exit = true;
-             } while (!exit);
-     }
-     /* fall-through */
+     case RTE_POWER_MGMT_TYPE_MONITOR: /* fall-through */
       case RTE_POWER_MGMT_TYPE_PAUSE:
               rte_eth_remove_rx_callback(port_id, queue_id,
                               queue_cfg->cur_cb);

@@ -356,10 +301,11 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,
               break;
       }
       /*
-      * we don't free the RX callback here because it is unsafe to do so
-      * unless we know for a fact that all data plane threads have stopped.
+      * the API doc mandates that the user stops all processing on affected
+      * ports before calling any of these API's, so we can assume that the
+      * callbacks can be freed. we're intentionally casting away const-ness.
        */
-     queue_cfg->cur_cb = NULL;
+     rte_free((void *)queue_cfg->cur_cb);
 
       return 0;
  }

lib/power/rte_power_pmd_mgmt.h +3 -0

index 7a0ac24625..7557f5d7e1 100644
@@ -43,6 +43,9 @@ enum rte_power_pmd_mgmt_type {
   *
   * @note This function is not thread-safe.
   *
+ * @warning This function must be called when all affected Ethernet ports are
+ *   stopped and no Rx/Tx is in progress!
+ *
   * @param lcore_id
   *   The lcore the Rx queue will be polled from.
   * @param port_id

@@ -69,6 +72,9 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id,
   *
   * @note This function is not thread-safe.
   *

Quoted 9 lines

Quoted 8 lines

lib/power/rte_power_pmd_mgmt.h +3 -1

+ * @warning This function must be called when all affected Ethernet ports are
+ *   stopped and no Rx/Tx is in progress!
+ *
   * @param lcore_id
   *   The lcore the Rx queue is polled from.
   * @param port_id
--

2.25.1

--
Thanks,
Anatoly

Quoted 27 lines · Konstantin

Quoted 7 lines

Quoted 7 lines

Currently, we expect that only one callback can be active at any given moment, for a particular queue configuration, which is relatively easy to implement in a thread-safe way. However, we're about to add support for multiple queues per lcore, which will greatly increase the possibility of various race conditions.

Quoted 7 lines

Quoted 5 lines

We could have used something like an RCU for this use case, but absent of a pressing need for thread safety we'll go the easy way and just mandate that the API's are to be called when all affected ports are stopped, and document this limitation. This greatly simplifies the rte_power_monitor-related code.

I think you need to update RN too with that.

Yep, will fix.

Quoted 5 lines

Another thing - do you really need the whole port stopped?
From what I understand - you work on queues, so it is enough for you
that related RX queue is stopped.
So, to make things a bit more robust, in pmgmt_queue_enable/disable you can call rte_eth_rx_queue_info_get() and check queue state.

Quoted 4 lines

We work on queues, but the data is per-lcore not per-queue, and it is potentially used by multiple queues, so checking one specific queue is not going to be enough. We could check all queues that were registered so far with the power library, maybe that'll work better?

Yep, that's what I mean: on queue_enable() check is that queue stopped or not. If not, return -EBUSY/EAGAIN or so/
Sorry if I wasn't clear at first time.

Quoted 32 lines

Quoted 31 lines · Konstantin

Quoted 7 lines

Quoted 7 lines

Currently, we expect that only one callback can be active at any given moment, for a particular queue configuration, which is relatively easy to implement in a thread-safe way. However, we're about to add support for multiple queues per lcore, which will greatly increase the possibility of various race conditions.

Quoted 7 lines

Quoted 5 lines

We could have used something like an RCU for this use case, but absent of a pressing need for thread safety we'll go the easy way and just mandate that the API's are to be called when all affected ports are stopped, and document this limitation. This greatly simplifies the rte_power_monitor-related code.

I think you need to update RN too with that.

Yep, will fix.

Quoted 5 lines

Another thing - do you really need the whole port stopped?
From what I understand - you work on queues, so it is enough for you
that related RX queue is stopped.
So, to make things a bit more robust, in pmgmt_queue_enable/disable you can call rte_eth_rx_queue_info_get() and check queue state.

We work on queues, but the data is per-lcore not per-queue, and it is potentially used by multiple queues, so checking one specific queue is not going to be enough. We could check all queues that were registered so far with the power library, maybe that'll work better?

Quoted 3 lines

Yep, that's what I mean: on queue_enable() check is that queue stopped or not. If not, return -EBUSY/EAGAIN or so/
Sorry if I wasn't clear at first time.

I think it's still better that all queues are stopped, rather than trying to work around the inherently racy implementation. So while i'll add the queue stopped checks, i'll still remove all of the thread safety stuff from here.

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of cores to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when a special designated "power saving" queue is polled. To put it another way, we have no idea which queue the user will poll in what order, so we rely on them telling us that queue X is the last one in the polling loop, so any power management should happen there.
- A new API is added to mark a specific Rx queue as "power saving".

Failing to call this API will result in no power management, however when having only one queue per core it is obvious which queue is the "power saving" one, so things will still work without this new API for use cases that were previously working without it.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/power/rte_power_pmd_mgmt.c | 335 ++++++++++++++++++++++++++-------
 lib/power/rte_power_pmd_mgmt.h |  34 ++++
 lib/power/version.map          |   3 +
 3 files changed, 306 insertions(+), 66 deletions(-)

lib/power/rte_power_pmd_mgmt.c +0 -0

index 0707c60a4f..60dd21a19c 100644

lib/power/rte_power_pmd_mgmt.c +13 -1

+	/**< Which port-queue pairs are associated with this lcore? */
+	struct queue power_save_queue;
+	/**< When polling multiple queues, all but this one will be ignored */
+	bool power_save_queue_set;
+	/**< When polling multiple queues, power save queue must be set */
+	size_t n_queues;
+	/**< How many queues are in the list? */
 	volatile enum pmd_mgmt_state pwr_mgmt_state;
 	/**< State of power management for this queue */
 	enum rte_power_pmd_mgmt_type cb_mode;

@@ -43,8 +55,97 @@ struct pmd_queue_cfg {
 	uint64_t empty_poll_stats;
 	/**< Number of empty polls */
 } __rte_cache_aligned;
+static struct pmd_core_cfg lcore_cfg[RTE_MAX_LCORE];
 
-static struct pmd_queue_cfg port_cfg[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+static inline bool
+queue_equal(const struct queue *l, const struct queue *r)
+{
+	return l->portid == r->portid && l->qid == r->qid;
+}

lib/power/rte_power_pmd_mgmt.c +8 -0

+	const struct queue *pwrsave = &cfg->power_save_queue;
+
+	/* if there's only single queue, no need to check anything */
+	if (cfg->n_queues == 1)
+		return true;
+	return cfg->power_save_queue_set && queue_equal(q, pwrsave);
+}
+

Quoted 12 lines

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

Quoted 12 lines

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of cores to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when a special designated "power saving" queue is polled. To put it another way, we have no idea which queue the user will poll in what order, so we rely on them telling us that queue X is the last one in the polling loop, so any power management should happen there.
- A new API is added to mark a specific Rx queue as "power saving".

Honestly, I don't understand the logic behind that new function. I understand that depending on HW we ca monitor either one or multiple queues. That's ok, but why we now need to mark one queue as a 'very special' one?

Why can't rte_power_ethdev_pmgmt_queue_enable() just:
Check is number of monitored queues exceed HW/SW capabilities, and if so then just return a failure.
Otherwise add queue to the list and treat them all equally, i.e:
go to power save mode when number of sequential empty polls on all monitored queues will exceed EMPTYPOLL_MAX threshold?

Quoted 19 lines

Failing to call this API will result in no power management, however when having only one queue per core it is obvious which queue is the "power saving" one, so things will still work without this new API for use cases that were previously working without it.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Signed-off-by Anatoly Burakov <[email protected]>

---
  lib/power/rte_power_pmd_mgmt.c | 335 ++++++++++++++++++++++++++-------
  lib/power/rte_power_pmd_mgmt.h |  34 ++++
  lib/power/version.map          |   3 +
  3 files changed, 306 insertions(+), 66 deletions(-)

lib/power/rte_power_pmd_mgmt.c +0 -0

index 0707c60a4f..60dd21a19c 100644

lib/power/rte_power_pmd_mgmt.c enum pmd_mgmt_state { · line 39

Quoted 8 lines

lib/power/rte_power_pmd_mgmt.c +4 -1

@@ -33,7 +33,19 @@ enum pmd_mgmt_state {
       PMD_MGMT_ENABLED
  };
 
-struct pmd_queue_cfg {
+struct queue {
+     uint16_t portid;
+     uint16_t qid;
+};

Just a thought: if that would help somehow, it can be changed to:

union queue {
         uint32_t raw;
         struct { uint16_t portid, qid;
         };
};

That way in queue find/cmp functions below you can operate with single raw 32-bt values. Probably not that important, as all these functions are on slow path, but might look nicer.

lib/power/rte_power_pmd_mgmt.c enum pmd_mgmt_state { · line 41

Quoted 3 lines

lib/power/rte_power_pmd_mgmt.c +2 -0

+struct pmd_core_cfg {
+     struct queue queues[RTE_MAX_ETHPORTS];

If we'll have ability to monitor multiple queues per lcore, would it be always enough?
From other side, it is updated on control path only.
Wouldn't normal list with malloc(/rte_malloc) would be more suitable here?

Quoted 27 lines

lib/power/rte_power_pmd_mgmt.c +17 -1

+     /**< Which port-queue pairs are associated with this lcore? */
+     struct queue power_save_queue;
+     /**< When polling multiple queues, all but this one will be ignored */
+     bool power_save_queue_set;
+     /**< When polling multiple queues, power save queue must be set */
+     size_t n_queues;
+     /**< How many queues are in the list? */
       volatile enum pmd_mgmt_state pwr_mgmt_state;
       /**< State of power management for this queue */
       enum rte_power_pmd_mgmt_type cb_mode;

@@ -43,8 +55,97 @@ struct pmd_queue_cfg {
       uint64_t empty_poll_stats;
       /**< Number of empty polls */
  } __rte_cache_aligned;
+static struct pmd_core_cfg lcore_cfg[RTE_MAX_LCORE];
 
-static struct pmd_queue_cfg port_cfg[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+static inline bool
+queue_equal(const struct queue *l, const struct queue *r)
+{
+     return l->portid == r->portid && l->qid == r->qid;
+}
+
+static inline void
+queue_copy(struct queue *dst, const struct queue *src)
+{

lib/power/rte_power_pmd_mgmt.c struct pmd_queue_cfg { · line 74

Quoted 6 lines

lib/power/rte_power_pmd_mgmt.c +6 -0

+     dst->portid = src->portid;
+     dst->qid = src->qid;
+}
+
+static inline bool
+queue_is_power_save(const struct pmd_core_cfg *cfg, const struct queue *q) {

Here and in other places - any reason why standard DPDK coding style is not used?

Quoted 9 lines

lib/power/rte_power_pmd_mgmt.c +8 -0

+     const struct queue *pwrsave = &cfg->power_save_queue;
+
+     /* if there's only single queue, no need to check anything */
+     if (cfg->n_queues == 1)
+             return true;
+     return cfg->power_save_queue_set && queue_equal(q, pwrsave);
+}
+

Quoted 25 lines

Quoted 12 lines

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

Quoted 12 lines

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of cores to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when a special designated "power saving" queue is polled. To put it another way, we have no idea which queue the user will poll in what order, so we rely on them telling us that queue X is the last one in the polling loop, so any power management should happen there.
- A new API is added to mark a specific Rx queue as "power saving".

Quoted 3 lines

Honestly, I don't understand the logic behind that new function. I understand that depending on HW we ca monitor either one or multiple queues. That's ok, but why we now need to mark one queue as a 'very special' one?

Because we don't know which of the queues we are supposed to sleep on.

Imagine a situation where you have 3 queues. What usually happens is you poll them in a loop, so q0, q1, q2, q0, q1, q2... etc. We only want to enter power-optimized state on polling q2, because otherwise we're risking going into power optimized state while q1 or q2 have traffic.

Worst case scenario, we enter sleep after polling q0, then traffic arrives at q2, we wake up, and then attempt to go to sleep on q1 instead of skipping it. Essentially, we will be attempting to sleep at every queue, instead of once in a loop. This *might* be OK for multi-monitor because we'll be aborting sleep due to sleep condition check failure, but for modes like rte_pause()/rte_power_pause()-based sleep, we will be entering sleep unconditionally, and will be risking to sleep at q1 while there's traffic at q2.

So, we need this mechanism to be activated once every *loop*, not per queue.

Quoted 36 lines

Why can't rte_power_ethdev_pmgmt_queue_enable() just:
Check is number of monitored queues exceed HW/SW capabilities, and if so then just return a failure.
Otherwise add queue to the list and treat them all equally, i.e:
go to power save mode when number of sequential empty polls on all monitored queues will exceed EMPTYPOLL_MAX threshold?

Quoted 19 lines

Failing to call this API will result in no power management, however when having only one queue per core it is obvious which queue is the "power saving" one, so things will still work without this new API for use cases that were previously working without it.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Signed-off-by Anatoly Burakov <[email protected]>

---
  lib/power/rte_power_pmd_mgmt.c | 335 ++++++++++++++++++++++++++-------
  lib/power/rte_power_pmd_mgmt.h |  34 ++++
  lib/power/version.map          |   3 +
  3 files changed, 306 insertions(+), 66 deletions(-)

lib/power/rte_power_pmd_mgmt.c +0 -0

index 0707c60a4f..60dd21a19c 100644

lib/power/rte_power_pmd_mgmt.c enum pmd_mgmt_state { · line 39

Quoted 8 lines

lib/power/rte_power_pmd_mgmt.c +4 -1

@@ -33,7 +33,19 @@ enum pmd_mgmt_state {
       PMD_MGMT_ENABLED
  };
 
-struct pmd_queue_cfg {
+struct queue {
+     uint16_t portid;
+     uint16_t qid;
+};

Just a thought: if that would help somehow, it can be changed to:

Quoted 8 lines

union queue {
         uint32_t raw;
         struct { uint16_t portid, qid;
         };
};

That way in queue find/cmp functions below you can operate with single raw 32-bt values. Probably not that important, as all these functions are on slow path, but might look nicer.

Sure, that can work. We actually do comparisons with power save queue on fast path, so maybe that'll help.

Quoted 7 lines

lib/power/rte_power_pmd_mgmt.c enum pmd_mgmt_state { · line 41

Quoted 3 lines

lib/power/rte_power_pmd_mgmt.c +2 -0

+struct pmd_core_cfg {
+     struct queue queues[RTE_MAX_ETHPORTS];

If we'll have ability to monitor multiple queues per lcore, would it be always enough?
From other side, it is updated on control path only.
Wouldn't normal list with malloc(/rte_malloc) would be more suitable here?

You're right, it should be dynamically allocated.

Quoted 27 lines

Quoted 27 lines

lib/power/rte_power_pmd_mgmt.c +17 -1

+     /**< Which port-queue pairs are associated with this lcore? */
+     struct queue power_save_queue;
+     /**< When polling multiple queues, all but this one will be ignored */
+     bool power_save_queue_set;
+     /**< When polling multiple queues, power save queue must be set */
+     size_t n_queues;
+     /**< How many queues are in the list? */
       volatile enum pmd_mgmt_state pwr_mgmt_state;
       /**< State of power management for this queue */
       enum rte_power_pmd_mgmt_type cb_mode;

@@ -43,8 +55,97 @@ struct pmd_queue_cfg {
       uint64_t empty_poll_stats;
       /**< Number of empty polls */
  } __rte_cache_aligned;
+static struct pmd_core_cfg lcore_cfg[RTE_MAX_LCORE];
 
-static struct pmd_queue_cfg port_cfg[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+static inline bool
+queue_equal(const struct queue *l, const struct queue *r)
+{
+     return l->portid == r->portid && l->qid == r->qid;
+}
+
+static inline void
+queue_copy(struct queue *dst, const struct queue *src)
+{

Quoted 8 lines

lib/power/rte_power_pmd_mgmt.c struct pmd_queue_cfg { · line 74

Quoted 6 lines

lib/power/rte_power_pmd_mgmt.c +6 -0

+     dst->portid = src->portid;
+     dst->qid = src->qid;
+}
+
+static inline bool
+queue_is_power_save(const struct pmd_core_cfg *cfg, const struct queue *q) {

Here and in other places - any reason why standard DPDK coding style is not used?

Just accidental :)

Quoted 9 lines

lib/power/rte_power_pmd_mgmt.c struct pmd_queue_cfg { · line 82

Quoted 9 lines

lib/power/rte_power_pmd_mgmt.c +8 -0

+     const struct queue *pwrsave = &cfg->power_save_queue;
+
+     /* if there's only single queue, no need to check anything */
+     if (cfg->n_queues == 1)
+             return true;
+     return cfg->power_save_queue_set && queue_equal(q, pwrsave);
+}
+

--
Thanks,
Anatoly

Quoted 32 lines · Konstantin

Quoted 25 lines

Quoted 12 lines

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

Quoted 12 lines

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of cores to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when a special designated "power saving" queue is polled. To put it another way, we have no idea which queue the user will poll in what order, so we rely on them telling us that queue X is the last one in the polling loop, so any power management should happen there.
- A new API is added to mark a specific Rx queue as "power saving".

Quoted 3 lines

Honestly, I don't understand the logic behind that new function. I understand that depending on HW we ca monitor either one or multiple queues. That's ok, but why we now need to mark one queue as a 'very special' one?

Quoted 6 lines

Because we don't know which of the queues we are supposed to sleep on.

Imagine a situation where you have 3 queues. What usually happens is you poll them in a loop, so q0, q1, q2, q0, q1, q2... etc. We only want to enter power-optimized state on polling q2, because otherwise we're risking going into power optimized state while q1 or q2 have traffic.

That's why before going to sleep we need to make sure that for *all* queues we have at least EMPTYPOLL_MAX empty polls.
Then the order of queue checking wouldn't matter.
With your example it should be:
if (q1.empty_polls > EMPTYPOLL_MAX && q2. empty_polls > EMPTYPOLL_MAX &&
q3.empy_pools > EMPTYPOLL_MAX)
goto_sleep;

Don't take me wrong, I am not suggesting to make *precisely* that checks in the actual code (it could be time consuming if number of checks is big), but the logic needs to remain.

Quoted 122 lines

Worst case scenario, we enter sleep after polling q0, then traffic arrives at q2, we wake up, and then attempt to go to sleep on q1 instead of skipping it. Essentially, we will be attempting to sleep at every queue, instead of once in a loop. This *might* be OK for multi-monitor because we'll be aborting sleep due to sleep condition check failure, but for modes like rte_pause()/rte_power_pause()-based sleep, we will be entering sleep unconditionally, and will be risking to sleep at q1 while there's traffic at q2.

So, we need this mechanism to be activated once every *loop*, not per queue.

Quoted 36 lines

Why can't rte_power_ethdev_pmgmt_queue_enable() just:
Check is number of monitored queues exceed HW/SW capabilities, and if so then just return a failure.
Otherwise add queue to the list and treat them all equally, i.e:
go to power save mode when number of sequential empty polls on all monitored queues will exceed EMPTYPOLL_MAX threshold?

Quoted 19 lines

Failing to call this API will result in no power management, however when having only one queue per core it is obvious which queue is the "power saving" one, so things will still work without this new API for use cases that were previously working without it.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Signed-off-by Anatoly Burakov <[email protected]>

---
   lib/power/rte_power_pmd_mgmt.c | 335 ++++++++++++++++++++++++++-------
   lib/power/rte_power_pmd_mgmt.h |  34 ++++
   lib/power/version.map          |   3 +
   3 files changed, 306 insertions(+), 66 deletions(-)

lib/power/rte_power_pmd_mgmt.c +0 -0

index 0707c60a4f..60dd21a19c 100644

lib/power/rte_power_pmd_mgmt.c enum pmd_mgmt_state { · line 39

Quoted 8 lines

lib/power/rte_power_pmd_mgmt.c +4 -1

@@ -33,7 +33,19 @@ enum pmd_mgmt_state {
        PMD_MGMT_ENABLED
   };
 
-struct pmd_queue_cfg {
+struct queue {
+     uint16_t portid;
+     uint16_t qid;
+};

Just a thought: if that would help somehow, it can be changed to:

Quoted 8 lines

union queue {
          uint32_t raw;
          struct { uint16_t portid, qid;
          };
};

That way in queue find/cmp functions below you can operate with single raw 32-bt values. Probably not that important, as all these functions are on slow path, but might look nicer.

Sure, that can work. We actually do comparisons with power save queue on fast path, so maybe that'll help.

Quoted 7 lines

lib/power/rte_power_pmd_mgmt.c enum pmd_mgmt_state { · line 41

Quoted 3 lines

lib/power/rte_power_pmd_mgmt.c +2 -0

+struct pmd_core_cfg {
+     struct queue queues[RTE_MAX_ETHPORTS];

If we'll have ability to monitor multiple queues per lcore, would it be always enough?
From other side, it is updated on control path only.
Wouldn't normal list with malloc(/rte_malloc) would be more suitable here?

You're right, it should be dynamically allocated.

Quoted 27 lines

Quoted 27 lines

lib/power/rte_power_pmd_mgmt.c +17 -1

+     /**< Which port-queue pairs are associated with this lcore? */
+     struct queue power_save_queue;
+     /**< When polling multiple queues, all but this one will be ignored */
+     bool power_save_queue_set;
+     /**< When polling multiple queues, power save queue must be set */
+     size_t n_queues;
+     /**< How many queues are in the list? */
        volatile enum pmd_mgmt_state pwr_mgmt_state;
        /**< State of power management for this queue */
        enum rte_power_pmd_mgmt_type cb_mode;

@@ -43,8 +55,97 @@ struct pmd_queue_cfg {
        uint64_t empty_poll_stats;
        /**< Number of empty polls */
   } __rte_cache_aligned;
+static struct pmd_core_cfg lcore_cfg[RTE_MAX_LCORE];
 
-static struct pmd_queue_cfg port_cfg[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
+static inline bool
+queue_equal(const struct queue *l, const struct queue *r)
+{
+     return l->portid == r->portid && l->qid == r->qid;
+}
+
+static inline void
+queue_copy(struct queue *dst, const struct queue *src)
+{

Quoted 8 lines

lib/power/rte_power_pmd_mgmt.c struct pmd_queue_cfg { · line 74

Quoted 6 lines

lib/power/rte_power_pmd_mgmt.c +6 -0

+     dst->portid = src->portid;
+     dst->qid = src->qid;
+}
+
+static inline bool
+queue_is_power_save(const struct pmd_core_cfg *cfg, const struct queue *q) {

Here and in other places - any reason why standard DPDK coding style is not used?

Just accidental :)

Quoted 9 lines

lib/power/rte_power_pmd_mgmt.c struct pmd_queue_cfg { · line 82

Quoted 9 lines

lib/power/rte_power_pmd_mgmt.c +8 -0

+     const struct queue *pwrsave = &cfg->power_save_queue;
+
+     /* if there's only single queue, no need to check anything */
+     if (cfg->n_queues == 1)
+             return true;
+     return cfg->power_save_queue_set && queue_equal(q, pwrsave);
+}
+

--
Thanks,
Anatoly