> -----Original Message----- > From: EXT Ivan Khoronzhuk [mailto:[email protected]] > Sent: Thursday, September 10, 2015 5:13 PM > To: Savolainen, Petri (Nokia - FI/Espoo); [email protected]; > [email protected] > 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:[email protected]] > >> Sent: Thursday, September 10, 2015 4:00 PM > >> To: Savolainen, Petri (Nokia - FI/Espoo); [email protected]; > >> [email protected] > >> 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:[email protected]] > >>>> Sent: Thursday, September 10, 2015 12:49 PM > >>>> To: [email protected]; Savolainen, Petri (Nokia - > FI/Espoo); > >>>> [email protected] > >>>> 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 <[email protected]> > >>>> --- > >>>> 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 _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
