| Message ID | 1441615859-23305-1-git-send-email-ivan.khoronzhuk@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 09/07/2015 10:50 AM, Ivan Khoronzhuk wrote: > 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.khoronzhuk@linaro.org> Reviewed-by: Nicolas Morey-Chaisemartin <nmorey@kalray.eu> > --- > > 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; > } > >
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@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.khoronzhuk@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) return ns; return odp_time_ns_to_cycles(ns); } -Petri
On 09.09.15 15:00, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@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.khoronzhuk@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. 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. 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 >
On 09.09.15 15:34, Ivan Khoronzhuk wrote: > > On 09.09.15 15:00, Savolainen, Petri (Nokia - FI/Espoo) wrote: >> >> >>> -----Original Message----- >>> From: lng-odp [mailto:lng-odp-bounces@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.khoronzhuk@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 *pool -> poll > application responsibility then conversion function. > > 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. > 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 >> >
> -----Original Message----- > From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@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-bounces@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.khoronzhuk@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. > > 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. 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
On 09.09.15 15:44, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@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-bounces@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.khoronzhuk@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? > > 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
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.khoronzhuk@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-bounces@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.khoronzhuk@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); > >> >> 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 >
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.khoronzhuk@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-bounces@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.khoronzhuk@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 >> >
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.khoronzhuk@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-bounces@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.khoronzhuk@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.
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; }
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.khoronzhuk@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(-)