On 09/10/2015 09:23 AM, Ivan Khoronzhuk wrote: > 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? > Yes, sounds allright.
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
