[v2,1/4] power: fix non thread-safe power env modification
Checks
Commit Message
From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
Due to lack of thread safety in exisiting solution
use spinlock mechanism for atomic
modification of power environment related data.
Fixes: 445c6528b5 ("power: common interface for guest and host")
Cc: stable@dpdk.org
Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
doc/guides/rel_notes/release_19_05.rst | 2 ++
lib/librte_power/rte_power.c | 30 ++++++++++++++++++--------
lib/librte_power/rte_power.h | 2 +-
3 files changed, 24 insertions(+), 10 deletions(-)
Comments
On Mon, 18 Mar 2019 12:56:44 +0100
Hajkowski <marcinx.hajkowski@intel.com> wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
>
> Due to lack of thread safety in exisiting solution
> use spinlock mechanism for atomic
> modification of power environment related data.
>
> Fixes: 445c6528b5 ("power: common interface for guest and host")
> Cc: stable@dpdk.org
>
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
Thanks for making this library better.
When I did the dpdk-metrics investigation at the US DPDK summit, the power
library was one of the DPDK libraries that no upstream open source project
uses.
You might want to consider:
* why is no project using it? what is wrong?
* does it not fit any use case in any project should it continue?
* if it is for a non-open source customer, then why is it part of DPDK?
On 18-Mar-19 11:56 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
>
> Due to lack of thread safety in exisiting solution
> use spinlock mechanism for atomic
> modification of power environment related data.
>
> Fixes: 445c6528b5 ("power: common interface for guest and host")
> Cc: stable@dpdk.org
>
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
18/03/2019 12:56, Hajkowski:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> --- a/doc/guides/rel_notes/release_19_05.rst
> +++ b/doc/guides/rel_notes/release_19_05.rst
> @@ -120,6 +120,8 @@ API Changes
> + * power: ``rte_power_set_env`` and ``rte_power_unset_env`` functions
> + have been modified to be thread safe.
The deprecation notice was recently sent,
so I guess this patch is for DPDK 19.08.
Review from the maintainer (David) may help.
Thanks
On 29-Mar-19 2:14 PM, Thomas Monjalon wrote:
> 18/03/2019 12:56, Hajkowski:
>> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
>> --- a/doc/guides/rel_notes/release_19_05.rst
>> +++ b/doc/guides/rel_notes/release_19_05.rst
>> @@ -120,6 +120,8 @@ API Changes
>> + * power: ``rte_power_set_env`` and ``rte_power_unset_env`` functions
>> + have been modified to be thread safe.
>
> The deprecation notice was recently sent,
> so I guess this patch is for DPDK 19.08.
Yes, this is changing API so the target was 19.08. However, first patch
is a fix and can be applied to 19.05 as well. The API documentation
stated that the function was not thread safe, but the code itself was
thread safe (it wasn't because it was buggy, but the intention of being
thread safe was there), so this could be considered fixing docs to match
the intended behavior of the code.
>
> Review from the maintainer (David) may help.
> Thanks
>
>
>
29/03/2019 16:09, Burakov, Anatoly:
> On 29-Mar-19 2:14 PM, Thomas Monjalon wrote:
> > 18/03/2019 12:56, Hajkowski:
> >> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> >> --- a/doc/guides/rel_notes/release_19_05.rst
> >> +++ b/doc/guides/rel_notes/release_19_05.rst
> >> @@ -120,6 +120,8 @@ API Changes
> >> + * power: ``rte_power_set_env`` and ``rte_power_unset_env`` functions
> >> + have been modified to be thread safe.
> >
> > The deprecation notice was recently sent,
> > so I guess this patch is for DPDK 19.08.
>
> Yes, this is changing API so the target was 19.08. However, first patch
> is a fix and can be applied to 19.05 as well. The API documentation
> stated that the function was not thread safe, but the code itself was
> thread safe (it wasn't because it was buggy, but the intention of being
> thread safe was there), so this could be considered fixing docs to match
> the intended behavior of the code.
>
> > Review from the maintainer (David) may help.
> > Thanks
What is the follow-up here?
We still have an old deprecation notice:
http://git.dpdk.org/dpdk/commit/?id=3477b7a2cc
I wonder how such things can be forgotten.
I feel some help is needed in prioritization,
so let's consider this deprecation as the priority #1
gating any other change in the power library.
Priority #2: cleaning up API which are secretly exported
for example convenience. It is an old design issue never fixed:
http://inbox.dpdk.org/dev/6046120.mQ0ExDuKPD@thomas/
Priority #3: request feedbacks from other maintainers
to add a generic API in ethdev to get a hook for power management.
On 25/10/2020 6:22 PM, Thomas Monjalon wrote:
> 29/03/2019 16:09, Burakov, Anatoly:
>> On 29-Mar-19 2:14 PM, Thomas Monjalon wrote:
>>> 18/03/2019 12:56, Hajkowski:
>>>> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
>>>> --- a/doc/guides/rel_notes/release_19_05.rst
>>>> +++ b/doc/guides/rel_notes/release_19_05.rst
>>>> @@ -120,6 +120,8 @@ API Changes
>>>> + * power: ``rte_power_set_env`` and ``rte_power_unset_env`` functions
>>>> + have been modified to be thread safe.
>>> The deprecation notice was recently sent,
>>> so I guess this patch is for DPDK 19.08.
>> Yes, this is changing API so the target was 19.08. However, first patch
>> is a fix and can be applied to 19.05 as well. The API documentation
>> stated that the function was not thread safe, but the code itself was
>> thread safe (it wasn't because it was buggy, but the intention of being
>> thread safe was there), so this could be considered fixing docs to match
>> the intended behavior of the code.
>>
>>> Review from the maintainer (David) may help.
>>> Thanks
> What is the follow-up here?
> We still have an old deprecation notice:
> http://git.dpdk.org/dpdk/commit/?id=3477b7a2cc
>
> I wonder how such things can be forgotten.
> I feel some help is needed in prioritization,
> so let's consider this deprecation as the priority #1
> gating any other change in the power library.
Hi Thomas,
#1 is now done, I've pushed a patch removing the deprication notice to
the mailing list, as the change it describes had previously been applied.
Patch here: http://patches.dpdk.org/patch/82327/
> Priority #2: cleaning up API which are secretly exported
> for example convenience. It is an old design issue never fixed:
> http://inbox.dpdk.org/dev/6046120.mQ0ExDuKPD@thomas/
Regarding the virtio channel API, Bruce and I had a look at this, and I
think I need to do some more research into it. I'd prefer not to make
that API public, as it was intended to be mainly for the
vm_power_manager app and the companion guest_cli.
So I'll look into this, and look at the best way to proceed with
cleaning this up so that these apps can be build using meson/ninja as
part of DPDK, as well as using 'make' extrernal to DPDK.
I hope to push up an RFC next week so we can get agreement on the best
path forward on this item.
>
> Priority #3: request feedbacks from other maintainers
> to add a generic API in ethdev to get a hook for power management.
>
Would it be possible to look at #2 and #3 in parallel? I'm not sure I'd
have #2 done fully in time for this release, and, if not, I will make
sure it's done for 21.02.
Rgds,
Dave.
28/10/2020 14:53, David Hunt:
> On 25/10/2020 6:22 PM, Thomas Monjalon wrote:
> > 29/03/2019 16:09, Burakov, Anatoly:
> >> On 29-Mar-19 2:14 PM, Thomas Monjalon wrote:
> >>> 18/03/2019 12:56, Hajkowski:
> >>>> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> >>>> --- a/doc/guides/rel_notes/release_19_05.rst
> >>>> +++ b/doc/guides/rel_notes/release_19_05.rst
> >>>> @@ -120,6 +120,8 @@ API Changes
> >>>> + * power: ``rte_power_set_env`` and ``rte_power_unset_env`` functions
> >>>> + have been modified to be thread safe.
> >>> The deprecation notice was recently sent,
> >>> so I guess this patch is for DPDK 19.08.
> >> Yes, this is changing API so the target was 19.08. However, first patch
> >> is a fix and can be applied to 19.05 as well. The API documentation
> >> stated that the function was not thread safe, but the code itself was
> >> thread safe (it wasn't because it was buggy, but the intention of being
> >> thread safe was there), so this could be considered fixing docs to match
> >> the intended behavior of the code.
> >>
> >>> Review from the maintainer (David) may help.
> >>> Thanks
> > What is the follow-up here?
> > We still have an old deprecation notice:
> > http://git.dpdk.org/dpdk/commit/?id=3477b7a2cc
> >
> > I wonder how such things can be forgotten.
> > I feel some help is needed in prioritization,
> > so let's consider this deprecation as the priority #1
> > gating any other change in the power library.
>
>
> Hi Thomas,
>
> #1 is now done, I've pushed a patch removing the deprication notice to
> the mailing list, as the change it describes had previously been applied.
> Patch here: http://patches.dpdk.org/patch/82327/
>
>
> > Priority #2: cleaning up API which are secretly exported
> > for example convenience. It is an old design issue never fixed:
> > http://inbox.dpdk.org/dev/6046120.mQ0ExDuKPD@thomas/
>
>
> Regarding the virtio channel API, Bruce and I had a look at this, and I
> think I need to do some more research into it. I'd prefer not to make
> that API public, as it was intended to be mainly for the
> vm_power_manager app and the companion guest_cli.
>
> So I'll look into this, and look at the best way to proceed with
> cleaning this up so that these apps can be build using meson/ninja as
> part of DPDK, as well as using 'make' extrernal to DPDK.
>
> I hope to push up an RFC next week so we can get agreement on the best
> path forward on this item.
>
>
> >
> > Priority #3: request feedbacks from other maintainers
> > to add a generic API in ethdev to get a hook for power management.
> >
>
> Would it be possible to look at #2 and #3 in parallel? I'm not sure I'd
> have #2 done fully in time for this release, and, if not, I will make
> sure it's done for 21.02.
Yes I'm looking at #3 in parallel but the ethdev API is really
not clear enough.
@@ -120,6 +120,8 @@ API Changes
Also, make sure to start the actual text at the margin.
=========================================================
+ * power: ``rte_power_set_env`` and ``rte_power_unset_env`` functions
+ have been modified to be thread safe.
ABI Changes
-----------
@@ -2,7 +2,7 @@
* Copyright(c) 2010-2014 Intel Corporation
*/
-#include <rte_atomic.h>
+#include <rte_spinlock.h>
#include "rte_power.h"
#include "power_acpi_cpufreq.h"
@@ -12,7 +12,7 @@
enum power_management_env global_default_env = PM_ENV_NOT_SET;
-volatile uint32_t global_env_cfg_status = 0;
+static rte_spinlock_t global_env_cfg_lock = RTE_SPINLOCK_INITIALIZER;
/* function pointers */
rte_power_freqs_t rte_power_freqs = NULL;
@@ -30,9 +30,15 @@ rte_power_get_capabilities_t rte_power_get_capabilities;
int
rte_power_set_env(enum power_management_env env)
{
- if (rte_atomic32_cmpset(&global_env_cfg_status, 0, 1) == 0) {
+ rte_spinlock_lock(&global_env_cfg_lock);
+
+ if (global_default_env != PM_ENV_NOT_SET) {
+ rte_spinlock_unlock(&global_env_cfg_lock);
return 0;
}
+
+ int ret = 0;
+
if (env == PM_ENV_ACPI_CPUFREQ) {
rte_power_freqs = power_acpi_cpufreq_freqs;
rte_power_get_freq = power_acpi_cpufreq_get_freq;
@@ -73,18 +79,24 @@ rte_power_set_env(enum power_management_env env)
} else {
RTE_LOG(ERR, POWER, "Invalid Power Management Environment(%d) set\n",
env);
- rte_power_unset_env();
- return -1;
+ ret = -1;
}
- global_default_env = env;
- return 0;
+
+ if (ret == 0)
+ global_default_env = env;
+ else
+ global_default_env = PM_ENV_NOT_SET;
+
+ rte_spinlock_unlock(&global_env_cfg_lock);
+ return ret;
}
void
rte_power_unset_env(void)
{
- if (rte_atomic32_cmpset(&global_env_cfg_status, 1, 0) != 0)
- global_default_env = PM_ENV_NOT_SET;
+ rte_spinlock_lock(&global_env_cfg_lock);
+ global_default_env = PM_ENV_NOT_SET;
+ rte_spinlock_unlock(&global_env_cfg_lock);
}
enum power_management_env
@@ -26,7 +26,7 @@ enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
/**
* Set the default power management implementation. If this is not called prior
* to rte_power_init(), then auto-detect of the environment will take place.
- * It is not thread safe.
+ * It is thread safe.
*
* @param env
* env. The environment in which to initialise Power Management for.