power: add wakeup log
PATCH 1/1 power: add wakeup log
This patch adds a log in rte_power_monitor to show the core has been waked up.
--- lib/eal/x86/rte_power_intrinsics.c | 8 ++++++++ 1 file changed, 8 insertions(+)
lib/eal/x86/rte_power_intrinsics.c +8 -0
@@ -128,6 +128,14 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc, : "D"(0), /* enter C0.2 */ "a"(tsc_l), "d"(tsc_h)); + cur_value =__get_umwait_val (pmc->addr, pmc->size); + +/* check if core has been waked up by changing monitoring value */ +if (pmc->fn (cur_value, pmc->opaque) !=0 ) +RTE_LOG (INFO ,EAL , +"lcore %u is waked up from value change\n" , +rte_lcore_id ()); + end:/* erase sleep address */ rte_spinlock_lock (&s->lock);
Signature · 1 line
2.25.1
lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 136
Quoted 1 line · On Tue, 22 Feb 2022 13:52:27 +0000 Miao Li <[email protected]> wrote:
lib/eal/x86/rte_power_intrinsics.c +1
-0+ "lcore %u is waked up from value change\n",
That is awkward phrasing in English and should be DEBUG not INFO level because it may happen often.
Maybe:
"lcore %u awoke because value changed\n"
or something better.
PS: sometimes it is good idea to using grammar tools when wording commit or messages.
Hi,
Quoted 8 lines
-----Original Message-----
From: Stephen Hemminger <[email protected]>
Sent: Wednesday, February 23, 2022 12:07 AM
To: Li, Miao <[email protected]>
Cc: [email protected]; Hunt, David <[email protected]>; Wang, Yinan <[email protected]>
Subject: Re: [PATCH v1] power: add wakeup log
Quoted 7 lines
lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 136
Quoted 1 line · On Tue, 22 Feb 2022 13:52:27 +0000 Miao Li <[email protected]> wrote:
lib/eal/x86/rte_power_intrinsics.c +1
-0+ "lcore %u is waked up from value change\n",That is awkward phrasing in English and should be DEBUG not INFO level because it may happen often.
I will fix it in the next version.
Quoted 4 lines
Maybe:
"lcore %u awoke because value changed\n"
or something better.
I will change the log content in next version.
Quoted 3 lines
PS: sometimes it is good idea to using grammar tools when wording commit or messages.
Thanks,
Miao
Quoted 17 lines · Miao Li
This patch adds a log in
rte_power_monitorto show the core has been waked up.Signed-off-by Miao Li <[email protected]>--- lib/eal/x86/rte_power_intrinsics.c | 8 ++++++++ 1 file changed, 8 insertions(+)lib/eal/x86/rte_power_intrinsics.c +1
-0@@ -128,6 +128,14 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc, : "D"(0), /* enter C0.2 */ "a"(tsc_l), "d"(tsc_h)); + cur_value =__get_umwait_val (pmc->addr, pmc->size);
lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 141
Quoted 10 lines
lib/eal/x86/rte_power_intrinsics.c +7
-0+ +/* check if core has been waked up by changing monitoring value */ +if (pmc->fn (cur_value, pmc->opaque) !=0 ) +RTE_LOG (INFO ,EAL , +"lcore %u is waked up from value change\n" , +rte_lcore_id ()); + end:/* erase sleep address */ rte_spinlock_lock (&s->lock);
Hi Li,
If I'm not mistaken, a similar patch was added to a previous DPDK release and then removed because of the enormous performance impact.
This looks to be something similar, and it's adding a log message to a low-level function. Also, as mentioned before, the intention in the future is to call this function much more agressively, so there would be hundreds of thousands of messages every second.
We cannot add an RTE_LOG here. Please rework and put the log in the test case instead.
Also, regarding the wording, I would suggest "lcore %u awoke due to monitor address value change\n"
Rgds,
Dave.
Hi,
Quoted 45 lines
-----Original Message-----
From: Hunt, David <[email protected]>
Sent: Wednesday, February 23, 2022 12:32 AM
To: Li, Miao <[email protected]>; [email protected]
Cc: Wang, Yinan <[email protected]>; [email protected]
Subject: Re: [PATCH v1] power: add wakeup logReview
Quoted 9 lines · Miao Li
This patch adds a log in
rte_power_monitorto show the core has been waked up.Signed-off-by Miao Li <[email protected]>--- lib/eal/x86/rte_power_intrinsics.c | 8 ++++++++ 1 file changed, 8 insertions(+)(quoted diff) +0
-0
b/lib/eal/x86/rte_power_intrinsics.clib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct
Quoted 4 lines
index
f749da9b85..dd63e2b6eb100644lib/eal/x86/rte_power_intrinsics.c +0
-0@@ -128,6 +128,14 @@ rte_power_monitor(const struct
rte_power_monitor_cond*pmc,Quoted 4 lines
lib/eal/x86/rte_power_intrinsics.c +1
-0: "D"(0), /* enter C0.2 */ "a"(tsc_l), "d"(tsc_h)); + cur_value =__get_umwait_val (pmc->addr, pmc->size);lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 141
Quoted 10 lines
lib/eal/x86/rte_power_intrinsics.c +7
-0+ +/* check if core has been waked up by changing monitoring value */ +if (pmc->fn (cur_value, pmc->opaque) !=0 ) +RTE_LOG (INFO ,EAL , +"lcore %u is waked up from value change\n" , +rte_lcore_id ()); + end:/* erase sleep address */ rte_spinlock_lock (&s->lock);Hi Li,
If I'm not mistaken, a similar patch was added to a previous DPDK release and then removed because of the enormous performance impact.
Quoted 7 lines
This looks to be something similar, and it's adding a log message to a low-level function. Also, as mentioned before, the intention in the future is to call this function much more agressively, so there would be hundreds of thousands of messages every second.
We cannot add an
RTE_LOGhere. Please rework and put the log in the test case instead.
I add a judgment before the output. When no packet arriver, the log will not be printed. When a lot of packets arriver, the rte_power_monitor will not be called. So I think the performance impact is small.
Quoted 3 lines
Also, regarding the wording, I would suggest "lcore %u awoke due to monitor address value change\n"
I will change the log content in next version.
Quoted 5 lines
Rgds,
Dave.
Thanks,
Miao
Quoted 64 lines · Miao
Hi,
Quoted 45 lines
-----Original Message-----
From: Hunt, David <[email protected]>
Sent: Wednesday, February 23, 2022 12:32 AM
To: Li, Miao <[email protected]>; [email protected]
Cc: Wang, Yinan <[email protected]>; [email protected]
Subject: Re: [PATCH v1] power: add wakeup logReview
Quoted 9 lines · Miao Li
This patch adds a log in
rte_power_monitorto show the core has been waked up.Signed-off-by Miao Li <[email protected]>--- lib/eal/x86/rte_power_intrinsics.c | 8 ++++++++ 1 file changed, 8 insertions(+)(quoted diff) +0
-0
b/lib/eal/x86/rte_power_intrinsics.clib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct
Quoted 4 lines
index
f749da9b85..dd63e2b6eb100644lib/eal/x86/rte_power_intrinsics.c +0
-0@@ -128,6 +128,14 @@ rte_power_monitor(const struct
rte_power_monitor_cond*pmc,Quoted 4 lines
lib/eal/x86/rte_power_intrinsics.c +1
-0: "D"(0), /* enter C0.2 */ "a"(tsc_l), "d"(tsc_h)); + cur_value =__get_umwait_val (pmc->addr, pmc->size);lib/eal/x86/rte_power_intrinsics.c rte_power_monitor(const struct rte_power_monitor_cond *pmc, · line 141
Quoted 10 lines
lib/eal/x86/rte_power_intrinsics.c +7
-0+ +/* check if core has been waked up by changing monitoring value */ +if (pmc->fn (cur_value, pmc->opaque) !=0 ) +RTE_LOG (INFO ,EAL , +"lcore %u is waked up from value change\n" , +rte_lcore_id ()); + end:/* erase sleep address */ rte_spinlock_lock (&s->lock);Hi Li,
If I'm not mistaken, a similar patch was added to a previous DPDK release and then removed because of the enormous performance impact.
Quoted 7 lines
This looks to be something similar, and it's adding a log message to a low-level function. Also, as mentioned before, the intention in the future is to call this function much more agressively, so there would be hundreds of thousands of messages every second.
We cannot add an
RTE_LOGhere. Please rework and put the log in the test case instead.I add a judgment before the output. When no packet arriver, the log will not be printed. When a lot of packets arriver, the
rte_power_monitorwill not be called. So I think the performance impact is small.Quoted 3 lines
Also, regarding the wording, I would suggest "lcore %u awoke due to monitor address value change\n"
I will change the log content in next version.
Quoted 1 line
Quoted 9 lines
Quoted 4 lines
Rgds,
Dave.
Thanks,
Miao
Hi Miao,
I have to agree with Dave here, I don't think this patch should be merged, as there are several problems with it.
First of all, the result might be inaccurate in practice, because there is a race condition between reading value first and second time - UMONITOR protects us from that before we sleep, but not after. So, the information we get with __get_umwait_val() and calling a callback might be out of date by the time we reach that code. So, this code is *provably* vulnerable to race conditions.
More importantly, I do not see how this is a useful addition to DPDK. I have to ask: what is the problem the patch is trying to solve? Because if the problem being solved is validating full path from l3fwd-power down to rte_power_monitor, then it could be solved in other ways that are more agreeable.
For example, I believe we have logging inside l3fwd-power, so we can validate that it wakes up correctly. We *don't* have logging inside rte_power for these particular code paths, but IMO it would be unnecessary because l3fwd-power already effectively does that anyway. We *don't* have logging inside rte_power_monitor, but we can still validate it with unit tests, if the concern here is validating that the functionality works correctly.
So, we can verify rte_power_monitor works with unit tests. We *could* introspect rte_power callback code with more logging if that's required (although IMO unnecessary, given that l3fwd-power already does that), and we can validate that l3fwd-power wakes up correctly with existing logging. So, with the addition of unit tests, the entire stack would be validated.
Thus, to me, it seems like the patch is adding a (conditional) log message to a low level function, supported by an unnecessary and racy read/callback call for the sole purpose of checking information that does not get used anywhere else in the function other than in a log message, to solve a problem that could be solved in another way (with unit tests).
Is there anything that I'm missing here?
Signature · 2 lines
Thanks, Anatoly