All,
On 10.09.15 16:15, Savolainen, Petri (Nokia - FI/Espoo) wrote:
-----Original Message-----
From: EXT Ivan Khoronzhuk [mailto:ivan.khoronz...@linaro.org]
Sent: Thursday, September 10, 2015 4:00 PM
To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org;
nmo...@kalray.eu
Subject: Re: [lng-odp] [Patch 2/2] validation: schedule: don't check
schedule time on 0
On 10.09.15 15:21, Savolainen, Petri (Nokia - FI/Espoo) wrote:
-----Original Message-----
From: EXT Ivan Khoronzhuk [mailto:ivan.khoronz...@linaro.org]
Sent: Thursday, September 10, 2015 12:49 PM
To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo);
nmo...@kalray.eu
Cc: Ivan Khoronzhuk
Subject: [lng-odp] [Patch 2/2] validation: schedule: don't check
schedule time on 0
The ODP_SCHED_NO_WAIT now corresponds to 0, not 1.
So no need to check it anymore.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronz...@linaro.org>
---
test/validation/scheduler/scheduler.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/test/validation/scheduler/scheduler.c
b/test/validation/scheduler/scheduler.c
index 1874889..94facea 100644
--- a/test/validation/scheduler/scheduler.c
+++ b/test/validation/scheduler/scheduler.c
@@ -96,9 +96,6 @@ void scheduler_test_wait_time(void)
wait_time = odp_schedule_wait_time(0);
This test is OK.
It's even not tested. But I don't touch it in my series.
I can push it with separate patch.
But I tend to add it in this patch like:
CU_ASSERT(wait_time == ODP_SCHED_NO_WAIT);
And rename patch on "correct wait time test"
is it OK for you?
- wait_time = odp_schedule_wait_time(1);
This is OK.
- CU_ASSERT(wait_time > 0);
This is not. The value returned is implementation specific.
That's why it's deleted.
-
wait_time = odp_schedule_wait_time((uint64_t)-1LL);
This is OK.
CU_ASSERT(wait_time > 0);
This is not. The value returned is implementation specific.
Probably better test it here like:
CU_ASSERT(wait_time != ODP_SCHED_NO_WAIT);
CU_ASSERT(wait_time != ODP_SCHED_WAIT);
I'll add it along with proposed test improvements.
Is it OK for you?
So, both asserts should be removed. In addition, wait time should be
tested with a schedule call ... but that's for another patch.
Right. But not here.
Probably with next test like "schedule_wait_time_check"
and test it with time API?
These are correct test cases:
I don't think so.
// calls don't crash
odp_schedule_wait_time(0);
odp_schedule_wait_time(1);
Let it be.
Don't forget, I'm not going to add it in this series.
...
odp_schedule_wait_time(-1);
Why do we need this at all?
// waits
odp_schedule(NULL, ODP_SCHED_WAIT);
How long are you going to wait on it?
Don't forget, I'm not going to add it in this series.
My intention here fix simple bug, not more.
// doesn't wait
odp_schedule(NULL, ODP_SCHED_NO_WAIT);
I dislike dependency on time API, but
How are you going to test it here w/o time API?
You can check that it was returned say within 5ns.
Don't forget, I'm not going to add it in this series.
// wait at least 'ns' nsec
wait_time = odp_schedule_wait_time(ns);
odp_schedule(NULL, wait_time);
Same here, + time API, another case your conversion function can produce
completely wrong result.
And don't forget, I'm not going to add it in this series.
Note that ODP_SCHED_WAIT and ODP_SCHED_NO_WAIT
- are inputs to odp_schedule()
right
- are not inputs to odp_schedule_wait_time()
rignt, that's why I deleted it in this patch (remember ns == ODP_SCHED_WAIT)
- are not outputs from odp_schedule_wait_time()
right and not.
It must return ODP_SCHED_NO_WAIT if ns = 0.
Seems we agreed on this.
Also it shouldn't produce ODP_SCHED_WAIT, never. We don't need surprises in the
code, especially wait forever.
From application (API spec) point of view output from odp_schedule_wait_time()
is a random value
I started to scare this call at all.
that may or may not match ODP_SCHED_WAIT or ODP_SCHED_NO_WAIT.
Really? Seems we extract information from different specs.
If you really see it, please, it should be corrected, even if it's obvious that
it must to not return ODP_SCHED_WAIT.
Application uses odp_schedule_wait_time() only when it needs to wait for a
certain time.
Ohh. Thanks.
So, according to my view on that:
In this concrete test, we should have:
wait_time = odp_schedule_wait_time(0);
CU_ASSERT(wait_time == ODP_SCHED_NO_WAIT);
wait_time = odp_schedule_wait_time((uint64_t)-1LL);
CU_ASSERT(wait_time != ODP_SCHED_NO_WAIT);
CU_ASSERT(wait_time != ODP_SCHED_WAIT);
Other changes can be added sometime later.
If you disagree,
I better won't add it here at all and limit this patch to simple fix,
and remove simply CU_ASSERT(wait_time > 0) for wait_time =
odp_schedule_wait_time(1);
-Petri
--
Regards,
Ivan Khoronzhuk
--
Regards,
Ivan Khoronzhuk
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
