Nicolas,
On 09.09.15 16:38, Ivan Khoronzhuk wrote:
On 09.09.15 16:27, Ivan Khoronzhuk wrote:
On 09.09.15 15:44, Savolainen, Petri (Nokia - FI/Espoo) wrote:
-----Original Message-----
From: EXT Ivan Khoronzhuk [mailto:ivan.khoronz...@linaro.org]
Sent: Wednesday, September 09, 2015 3:35 PM
To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [Patch] linux-generic: odp_schedule: fix
odp_schdule_wait_time
On 09.09.15 15:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:
-----Original Message-----
From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
ext Ivan Khoronzhuk
Sent: Monday, September 07, 2015 11:51 AM
To: lng-odp@lists.linaro.org
Subject: [lng-odp] [Patch] linux-generic: odp_schedule: fix
odp_schdule_wait_time
The resolution of schedule time can be more than 1ns. As result
can happen that 1ns corresponds 0 ticks of timer counter, but if
passed 1ns it's obvious that user wanted to schedule at least once.
Currently it can lead to wait forever, as 0 corresponds to
ODP_SCHED_WAIT, it can block program flow at all.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronz...@linaro.org>
---
Prerequisit for this patch was taken from:
"[lng-odp] [Patch] validation: scheduler: increase time check"
https://lists.linaro.org/pipermail/lng-odp/2015-August/014738.html
platform/linux-generic/odp_schedule.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-
generic/odp_schedule.c
index c6619e5..05497de 100644
--- a/platform/linux-generic/odp_schedule.c
+++ b/platform/linux-generic/odp_schedule.c
@@ -646,10 +646,13 @@ void odp_schedule_resume(void)
uint64_t odp_schedule_wait_time(uint64_t ns)
{
- if (ns <= ODP_SCHED_NO_WAIT)
- ns = ODP_SCHED_NO_WAIT + 1;
+ uint64_t time;
- return odp_time_ns_to_cycles(ns);
+ time = odp_time_ns_to_cycles(ns);
+ if (!time)
+ time = ODP_SCHED_NO_WAIT;
+
+ return time;
}
I think it's better to change (implementation specific) WAIT/NO_WAIT
values like this.
plat/schedule_types.h
#define ODP_SCHED_WAIT UINT64_MAX
#define ODP_SCHED_NO_WAIT 0
... and avoid any conversion in the common case (WAIT/NO_WAIT). It
will not matter, if 1 ns is rounded to 0 == NO_WAIT.
uint64_t odp_schedule_wait_time(uint64_t ns)
{
if (ns == ODP_SCHED_WAIT || ns == ODP_SCHED_NO_WAIT)
We shouldn't compare sched time and ns, even it's under implementation.
And there is no direct mapping between ns and sched time.
In case of maximum for ns, it doesn't mean pool forever for scheduler.
Even if it has very low possibility. Assigning POOL forever it's rather
an
application responsibility then conversion function.
If application asks for a timeout in 580 years (== UINT64_MAX in nsec), it's
pretty much the same thing than asking for WAIT. It would see the difference
after 580 years. Not a huge problem.
It's not a huge problem, but why?
And we can use the "conversion" function for some calculations,
who knows, what can be in user mind.
Bumc, we return him UINT64_MAX, instead of some UNIT64/10.
(but the same we can say about 1 instead of 0 ...., but it has less impact)
There is can be other things....
This function can be reused by implementations, mostly on transition
stage.
As you mentioned, WAIT/NO_WAIT it's implementation specific (define can
be not from linux-generic,
and function from linux-generic) and better to not rely on WAIT/NO_WAIT
= 0.
Currently it is implementation specified value. Linux-generic can use -1 and 0.
Others can use other values. This as well as other #defines may need to be
fixed for binary compatibility.
Binary compatibility is fine. But why current values cannot be used for that? I
didn't break them.
Adding patch I bear in mind how to not break other implementations,
applications...
I like more small steps in patch then add two ideas in one, if possible.
It allows save time in future.
If we must have proposed values for defines in order to follow binary
compatibility lets
add it in separate patch, that can be easily reverted in case of issue.
But I have no objection to add your proposition here, if you very insist on it,
seems it removes problem with 1 cycle.
But I think, better to rewrite it a little like this, no need to control
ODP_SCHED_WAIT
in this function, and better remove direct comparison ns and sched time.
#define ODP_SCHED_WAIT UINT64_MAX
#define ODP_SCHED_NO_WAIT 0
if (!ns)
return ODP_SCHED_NO_WAIT;
return odp_time_ns_to_cycles(ns);
What do you say?
Even like this:
#define ODP_SCHED_WAIT UINT64_MAX
#define ODP_SCHED_NO_WAIT 0
return odp_time_ns_to_cycles(ns);
Is it OK for you?
It's better to cleanly catch both values (WAIT/NO_WAIT) and use conversion only
for the rest (real nsec values).
-Petri
My variant has no such drawback and is more clear in what's going on.
And your example should be revritten like:
if (!ns)
return ODP_SCHED_NO_WAIT;
return odp_time_ns_to_cycles(ns);
It doesn't differ a lot with proposed patch but still has mentioned
drawback, even
if it's very rarely case.
return ns;
return odp_time_ns_to_cycles(ns);
}
-Petri
--
Regards,
Ivan Khoronzhuk
--
Regards,
Ivan Khoronzhuk
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
