| Message ID | 1441878522-19505-3-git-send-email-ivan.khoronzhuk@linaro.org |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] > Sent: Thursday, September 10, 2015 12:49 PM > To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo); > nmorey@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.khoronzhuk@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. > > - wait_time = odp_schedule_wait_time(1); This is OK. > - CU_ASSERT(wait_time > 0); This is not. The value returned is implementation specific. > - > 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. So, both asserts should be removed. In addition, wait time should be tested with a schedule call ... but that's for another patch. -Petri > } > -- > 1.9.1
On 10.09.15 15:21, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >> Sent: Thursday, September 10, 2015 12:49 PM >> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo); >> nmorey@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.khoronzhuk@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?
> -----Original Message----- > From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] > Sent: Thursday, September 10, 2015 4:00 PM > To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org; > nmorey@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.khoronzhuk@linaro.org] > >> Sent: Thursday, September 10, 2015 12:49 PM > >> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo); > >> nmorey@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.khoronzhuk@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: // calls don't crash odp_schedule_wait_time(0); odp_schedule_wait_time(1); ... odp_schedule_wait_time(-1); // waits odp_schedule(NULL, ODP_SCHED_WAIT); // doesn't wait odp_schedule(NULL, ODP_SCHED_NO_WAIT); // wait at least 'ns' nsec wait_time = odp_schedule_wait_time(ns); odp_schedule(NULL, wait_time); Note that ODP_SCHED_WAIT and ODP_SCHED_NO_WAIT - are inputs to odp_schedule() - are not inputs to odp_schedule_wait_time() - are not outputs from odp_schedule_wait_time() From application (API spec) point of view output from odp_schedule_wait_time() is a random value that may or may not match ODP_SCHED_WAIT or ODP_SCHED_NO_WAIT. Application uses odp_schedule_wait_time() only when it needs to wait for a certain time. -Petri > > > -- > Regards, > Ivan Khoronzhuk
All, On 10.09.15 16:15, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >> Sent: Thursday, September 10, 2015 4:00 PM >> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org; >> nmorey@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.khoronzhuk@linaro.org] >>>> Sent: Thursday, September 10, 2015 12:49 PM >>>> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo); >>>> nmorey@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.khoronzhuk@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
> -----Original Message----- > From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] > Sent: Thursday, September 10, 2015 5:13 PM > To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org; > nmorey@kalray.eu > Subject: Re: [lng-odp] [Patch 2/2] validation: schedule: don't check > schedule time on 0 > > All, > > On 10.09.15 16:15, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > > > > >> -----Original Message----- > >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] > >> Sent: Thursday, September 10, 2015 4:00 PM > >> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org; > >> nmorey@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.khoronzhuk@linaro.org] > >>>> Sent: Thursday, September 10, 2015 12:49 PM > >>>> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - > FI/Espoo); > >>>> nmorey@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.khoronzhuk@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); > /** * Schedule * * Schedules all queues created with ODP_QUEUE_TYPE_SCHED type. Returns * next highest priority event which is available for the calling thread. * Outputs the source queue of the event. If there's no event available, waits * for an event according to the wait parameter setting. Returns * ODP_EVENT_INVALID if reaches end of the wait period. * * When returns an event, the thread holds the queue synchronization context * (atomic or ordered) until the next odp_schedule() or odp_schedule_multi() * call. The next call implicitly releases the current context and potentially * returns with a new context. User can allow early context release (e.g. see * odp_schedule_release_atomic()) for performance optimization. * * @param from Output parameter for the source queue (where the event was * dequeued from). Ignored if NULL. * @param wait Minimum time to wait for an event. Waits infinitely, if set to * ODP_SCHED_WAIT. Does not wait, if set to ODP_SCHED_NO_WAIT. * Use odp_schedule_wait_time() to convert time to other wait * values. * * @return Next highest priority event * @retval ODP_EVENT_INVALID on timeout and no events available * * @see odp_schedule_multi(), odp_schedule_release_atomic() */ odp_event_t odp_schedule(odp_queue_t *from, uint64_t wait); wait == Minimum time to wait for an event. Waits infinitely, if set to ODP_SCHED_WAIT. Does not wait, if set to ODP_SCHED_NO_WAIT. Use odp_schedule_wait_time() to convert time to other wait values. /** * Schedule wait time * * Converts nanoseconds to wait values for other schedule functions. * * @param ns Nanoseconds * * @return Value for the wait parameter in schedule functions */ uint64_t odp_schedule_wait_time(uint64_t ns); Implementation decides if ODP_SCHED_WAIT/NO_WAIT belong to the range returned by odp_schedule_wait_time(). Application must not expect that e.g. odp_schedule_wait_time(0) == ODP_SCHED_NO_WAIT, or any other value. This would be a valid implementation: ODP_SCHED_NO_WAIT == 0 odp_schedule_wait_time(0) == 7 odp_schedule_wait_time(1000) == 17 odp_schedule_wait_time(UINT64_MAX) == 17000000 ODP_SCHED_WAIT == UINT64_MAX -Petri
Petri, On 10.09.15 17:35, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >> Sent: Thursday, September 10, 2015 5:13 PM >> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org; >> nmorey@kalray.eu >> Subject: Re: [lng-odp] [Patch 2/2] validation: schedule: don't check >> schedule time on 0 >> >> All, >> >> On 10.09.15 16:15, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>> >>> >>>> -----Original Message----- >>>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >>>> Sent: Thursday, September 10, 2015 4:00 PM >>>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org; >>>> nmorey@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.khoronzhuk@linaro.org] >>>>>> Sent: Thursday, September 10, 2015 12:49 PM >>>>>> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - >> FI/Espoo); >>>>>> nmorey@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.khoronzhuk@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); >> > > > /** > * Schedule > * > * Schedules all queues created with ODP_QUEUE_TYPE_SCHED type. Returns > * next highest priority event which is available for the calling thread. > * Outputs the source queue of the event. If there's no event available, waits > * for an event according to the wait parameter setting. Returns > * ODP_EVENT_INVALID if reaches end of the wait period. > * > * When returns an event, the thread holds the queue synchronization context > * (atomic or ordered) until the next odp_schedule() or odp_schedule_multi() > * call. The next call implicitly releases the current context and potentially > * returns with a new context. User can allow early context release (e.g. see > * odp_schedule_release_atomic()) for performance optimization. > * > * @param from Output parameter for the source queue (where the event was > * dequeued from). Ignored if NULL. > * @param wait Minimum time to wait for an event. Waits infinitely, if set to > * ODP_SCHED_WAIT. Does not wait, if set to ODP_SCHED_NO_WAIT. > * Use odp_schedule_wait_time() to convert time to other wait > * values. > * > * @return Next highest priority event > * @retval ODP_EVENT_INVALID on timeout and no events available > * > * @see odp_schedule_multi(), odp_schedule_release_atomic() > */ > odp_event_t odp_schedule(odp_queue_t *from, uint64_t wait); > > > wait == Minimum time to wait for an event. Waits infinitely, if set to ODP_SCHED_WAIT. Does not wait, if set to ODP_SCHED_NO_WAIT. Use odp_schedule_wait_time() to convert time to other wait values. Sorry, but where did you found here that output from odp_schedule_wait_time() may or may not match ODP_SCHED_WAIT or ODP_SCHED_NO_WAIT? "to convert time to other wait values" rather say that it cannot. Minimum time can be guaranteed every time, don't forget it polls, no problem to wait even more than some counter can allow. No need to return ODP_SCHED_WAIT from odp_schedule_wait_time() at all. It's bad approach, even more it should be checked in validation test. > > /** > * Schedule wait time > * > * Converts nanoseconds to wait values for other schedule functions. > * > * @param ns Nanoseconds > * > * @return Value for the wait parameter in schedule functions > */ > uint64_t odp_schedule_wait_time(uint64_t ns); > > > Implementation decides if ODP_SCHED_WAIT/NO_WAIT belong to the range returned by odp_schedule_wait_time(). > Application must not expect that e.g. odp_schedule_wait_time(0) == ODP_SCHED_NO_WAIT, or any other value. On my opinion it's better to add this requirement. If user asks 0ns, why it cannot be "no wait"? It can never be less that 0ns. It can be some algorithm that generate wait time with analytical expression, that expects 0ns to be NO_WAIT. In regard of ODP_SCHED_NO_WAIT, I agree it can be more, why not. But odp_schedule_wait_time(0) == ODP_SCHED_NO_WAIT - it's very convenient. Anyway it's supposed to follow some ABI, as you sad. > > This would be a valid implementation: > > ODP_SCHED_NO_WAIT == 0 > > odp_schedule_wait_time(0) == 7 > odp_schedule_wait_time(1000) == 17 > odp_schedule_wait_time(UINT64_MAX) == 17000000 > > ODP_SCHED_WAIT == UINT64_MAX > > > -Petri > > > >
Petri, On 10.09.15 18:16, Ivan Khoronzhuk wrote: > Petri, > > On 10.09.15 17:35, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >> >>> -----Original Message----- >>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >>> Sent: Thursday, September 10, 2015 5:13 PM >>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org; >>> nmorey@kalray.eu >>> Subject: Re: [lng-odp] [Patch 2/2] validation: schedule: don't check >>> schedule time on 0 >>> >>> All, >>> >>> On 10.09.15 16:15, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >>>>> Sent: Thursday, September 10, 2015 4:00 PM >>>>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org; >>>>> nmorey@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.khoronzhuk@linaro.org] >>>>>>> Sent: Thursday, September 10, 2015 12:49 PM >>>>>>> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - >>> FI/Espoo); >>>>>>> nmorey@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.khoronzhuk@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); >>> >> >> >> /** >> * Schedule >> * >> * Schedules all queues created with ODP_QUEUE_TYPE_SCHED type. Returns >> * next highest priority event which is available for the calling thread. >> * Outputs the source queue of the event. If there's no event available, waits >> * for an event according to the wait parameter setting. Returns >> * ODP_EVENT_INVALID if reaches end of the wait period. >> * >> * When returns an event, the thread holds the queue synchronization context >> * (atomic or ordered) until the next odp_schedule() or odp_schedule_multi() >> * call. The next call implicitly releases the current context and potentially >> * returns with a new context. User can allow early context release (e.g. see >> * odp_schedule_release_atomic()) for performance optimization. >> * >> * @param from Output parameter for the source queue (where the event was >> * dequeued from). Ignored if NULL. >> * @param wait Minimum time to wait for an event. Waits infinitely, if set to >> * ODP_SCHED_WAIT. Does not wait, if set to ODP_SCHED_NO_WAIT. >> * Use odp_schedule_wait_time() to convert time to other wait >> * values. >> * >> * @return Next highest priority event >> * @retval ODP_EVENT_INVALID on timeout and no events available >> * >> * @see odp_schedule_multi(), odp_schedule_release_atomic() >> */ >> odp_event_t odp_schedule(odp_queue_t *from, uint64_t wait); >> >> >> wait == Minimum time to wait for an event. Waits infinitely, if set to ODP_SCHED_WAIT. Does not wait, if set to ODP_SCHED_NO_WAIT. Use odp_schedule_wait_time() to convert time to other wait values. > Sorry, but > where did you found here that output from odp_schedule_wait_time() may or may not match ODP_SCHED_WAIT or ODP_SCHED_NO_WAIT? > "to convert time to other wait values" rather say that it cannot. Logically that's true. If odp_schedule_wait_time() can generate other values it doesn't mean it cannot generate these values. But our patch fixes exactly one of such cases. It generated ODP_SCHED_WAIT, but we didn't expect it. We simply linearize it. There is should be randomness. I've sent v2, please review it. > Minimum time can be guaranteed every time, don't forget it polls, no > problem to wait even more than some counter can allow. > No need to return ODP_SCHED_WAIT from odp_schedule_wait_time() at all. > It's bad approach, even more it should be checked in validation test. > >> >> /** >> * Schedule wait time >> * >> * Converts nanoseconds to wait values for other schedule functions. >> * >> * @param ns Nanoseconds >> * >> * @return Value for the wait parameter in schedule functions >> */ >> uint64_t odp_schedule_wait_time(uint64_t ns); >> >> >> Implementation decides if ODP_SCHED_WAIT/NO_WAIT belong to the range returned by odp_schedule_wait_time(). >> Application must not expect that e.g. odp_schedule_wait_time(0) == ODP_SCHED_NO_WAIT, or any other value. > > On my opinion it's better to add this requirement. > If user asks 0ns, why it cannot be "no wait"? It can never be less that 0ns. > > It can be some algorithm that generate wait time with analytical expression, > that expects 0ns to be NO_WAIT. > > In regard of ODP_SCHED_NO_WAIT, I agree it can be more, why not. But > odp_schedule_wait_time(0) == ODP_SCHED_NO_WAIT - it's very convenient. > Anyway it's supposed to follow some ABI, as you sad. > >> >> This would be a valid implementation: >> >> ODP_SCHED_NO_WAIT == 0 >> >> odp_schedule_wait_time(0) == 7 >> odp_schedule_wait_time(1000) == 17 >> odp_schedule_wait_time(UINT64_MAX) == 17000000 >> >> ODP_SCHED_WAIT == UINT64_MAX >> >> >> -Petri >> >> >> >> >
On 10.09.15 21:08, Ivan Khoronzhuk wrote: > Petri, > > On 10.09.15 18:16, Ivan Khoronzhuk wrote: >> Petri, >> >> On 10.09.15 17:35, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>> >>> >>>> -----Original Message----- >>>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >>>> Sent: Thursday, September 10, 2015 5:13 PM >>>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org; >>>> nmorey@kalray.eu >>>> Subject: Re: [lng-odp] [Patch 2/2] validation: schedule: don't check >>>> schedule time on 0 >>>> >>>> All, >>>> >>>> On 10.09.15 16:15, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org] >>>>>> Sent: Thursday, September 10, 2015 4:00 PM >>>>>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org; >>>>>> nmorey@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.khoronzhuk@linaro.org] >>>>>>>> Sent: Thursday, September 10, 2015 12:49 PM >>>>>>>> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - >>>> FI/Espoo); >>>>>>>> nmorey@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.khoronzhuk@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); >>>> >>> >>> >>> /** >>> * Schedule >>> * >>> * Schedules all queues created with ODP_QUEUE_TYPE_SCHED type. Returns >>> * next highest priority event which is available for the calling thread. >>> * Outputs the source queue of the event. If there's no event available, waits >>> * for an event according to the wait parameter setting. Returns >>> * ODP_EVENT_INVALID if reaches end of the wait period. >>> * >>> * When returns an event, the thread holds the queue synchronization context >>> * (atomic or ordered) until the next odp_schedule() or odp_schedule_multi() >>> * call. The next call implicitly releases the current context and potentially >>> * returns with a new context. User can allow early context release (e.g. see >>> * odp_schedule_release_atomic()) for performance optimization. >>> * >>> * @param from Output parameter for the source queue (where the event was >>> * dequeued from). Ignored if NULL. >>> * @param wait Minimum time to wait for an event. Waits infinitely, if set to >>> * ODP_SCHED_WAIT. Does not wait, if set to ODP_SCHED_NO_WAIT. >>> * Use odp_schedule_wait_time() to convert time to other wait >>> * values. >>> * >>> * @return Next highest priority event >>> * @retval ODP_EVENT_INVALID on timeout and no events available >>> * >>> * @see odp_schedule_multi(), odp_schedule_release_atomic() >>> */ >>> odp_event_t odp_schedule(odp_queue_t *from, uint64_t wait); >>> >>> >>> wait == Minimum time to wait for an event. Waits infinitely, if set to ODP_SCHED_WAIT. Does not wait, if set to ODP_SCHED_NO_WAIT. Use odp_schedule_wait_time() to convert time to other wait values. >> Sorry, but >> where did you found here that output from odp_schedule_wait_time() may or may not match ODP_SCHED_WAIT or ODP_SCHED_NO_WAIT? >> "to convert time to other wait values" rather say that it cannot. > Logically that's true. If odp_schedule_wait_time() can generate other values > it doesn't mean it cannot generate these values. But our patch > fixes exactly one of such cases. It generated ODP_SCHED_WAIT, but we didn't expect it. > We simply linearize it. There is should be randomness. *should -> shouldn't be > I've sent v2, please review it. > >> Minimum time can be guaranteed every time, don't forget it polls, no >> problem to wait even more than some counter can allow. >> No need to return ODP_SCHED_WAIT from odp_schedule_wait_time() at all. >> It's bad approach, even more it should be checked in validation test. >> >>> >>> /** >>> * Schedule wait time >>> * >>> * Converts nanoseconds to wait values for other schedule functions. >>> * >>> * @param ns Nanoseconds >>> * >>> * @return Value for the wait parameter in schedule functions >>> */ >>> uint64_t odp_schedule_wait_time(uint64_t ns); >>> >>> >>> Implementation decides if ODP_SCHED_WAIT/NO_WAIT belong to the range returned by odp_schedule_wait_time(). >>> Application must not expect that e.g. odp_schedule_wait_time(0) == ODP_SCHED_NO_WAIT, or any other value. >> >> On my opinion it's better to add this requirement. >> If user asks 0ns, why it cannot be "no wait"? It can never be less that 0ns. >> >> It can be some algorithm that generate wait time with analytical expression, >> that expects 0ns to be NO_WAIT. >> >> In regard of ODP_SCHED_NO_WAIT, I agree it can be more, why not. But >> odp_schedule_wait_time(0) == ODP_SCHED_NO_WAIT - it's very convenient. >> Anyway it's supposed to follow some ABI, as you sad. >> >>> >>> This would be a valid implementation: >>> >>> ODP_SCHED_NO_WAIT == 0 >>> >>> odp_schedule_wait_time(0) == 7 >>> odp_schedule_wait_time(1000) == 17 >>> odp_schedule_wait_time(UINT64_MAX) == 17000000 >>> >>> ODP_SCHED_WAIT == UINT64_MAX >>> >>> >>> -Petri >>> >>> >>> >>> >> >
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); - wait_time = odp_schedule_wait_time(1); - CU_ASSERT(wait_time > 0); - wait_time = odp_schedule_wait_time((uint64_t)-1LL); CU_ASSERT(wait_time > 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.khoronzhuk@linaro.org> --- test/validation/scheduler/scheduler.c | 3 --- 1 file changed, 3 deletions(-)