| Message ID | 1425998629-21691-1-git-send-email-ola.liljedahl@linaro.org |
|---|---|
| State | Accepted |
| Commit | 0eea70fcff9531743f70e96f64109217e761f4c4 |
| Headers | show |
On 03/10/15 17:43, Ola Liljedahl wrote: > Ensure that the timeout user_ptr and timer fields are set when the > corresponding timer is immediately cancelled. > https://bugs.linaro.org/show_bug.cgi?id=1313 > > Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> > --- > (This document/code contribution attached is provided under the terms of > agreement LES-LTM-21309) > > Passes odp_timer validation with the new odp_timer_cancel() test from Petri. > > platform/linux-generic/odp_timer.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c > index 61a02b6..6b48d2e 100644 > --- a/platform/linux-generic/odp_timer.c > +++ b/platform/linux-generic/odp_timer.c > @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx, > #endif > } else { > /* We have a new timeout buffer which replaces any old one */ > + /* Fill in header fields if timeout event */ > + if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { > + /* Convert from buffer to timeout hdr */ > + odp_timeout_hdr_t *tmo_hdr = > + timeout_hdr_from_buf(*tmo_buf); as we discussed earlier this 2 definitions of tmo_hdr can be one on top of timer_expire(). int succ can be int ret and also defined on top. Maxim. > + tmo_hdr->timer = tp_idx_to_handle(tp, idx); > + tmo_hdr->user_ptr = tp->timers[idx].user_ptr; > + /* expiration field filled in when timer expires */ > + } > + /* Else ignore buffers of other types */ > odp_buffer_t old_buf = ODP_BUFFER_INVALID; > #ifdef ODP_ATOMIC_U128 > tick_buf_t new, old; > @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool *tp, uint32_t idx, uint64_t tick) > _odp_atomic_flag_clear(IDX2LOCK(idx)); > #endif > if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { > - /* Fill in metadata fields in system timeout buffer */ > + /* Fill in expiration tick if timeout event */ > if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { > /* Convert from buffer to timeout hdr */ > odp_timeout_hdr_t *tmo_hdr = > timeout_hdr_from_buf(tmo_buf); > - tmo_hdr->timer = tp_idx_to_handle(tp, idx); > tmo_hdr->expiration = exp_tck; > - tmo_hdr->user_ptr = tim->user_ptr; > + /* timer and user_ptr fields filled in when timer > + * was set */ > } > - /* Else ignore buffers of other types */ > + /* Else ignore events of other types */ > /* Post the timeout to the destination queue */ > int rc = odp_queue_enq(tim->queue, > odp_buffer_to_event(tmo_buf));
On 10 March 2015 at 15:59, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 03/10/15 17:43, Ola Liljedahl wrote: >> >> Ensure that the timeout user_ptr and timer fields are set when the >> corresponding timer is immediately cancelled. >> https://bugs.linaro.org/show_bug.cgi?id=1313 >> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >> --- >> (This document/code contribution attached is provided under the terms of >> agreement LES-LTM-21309) >> >> Passes odp_timer validation with the new odp_timer_cancel() test from >> Petri. >> >> platform/linux-generic/odp_timer.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/platform/linux-generic/odp_timer.c >> b/platform/linux-generic/odp_timer.c >> index 61a02b6..6b48d2e 100644 >> --- a/platform/linux-generic/odp_timer.c >> +++ b/platform/linux-generic/odp_timer.c >> @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx, >> #endif >> } else { >> /* We have a new timeout buffer which replaces any old one >> */ >> + /* Fill in header fields if timeout event */ >> + if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { >> + /* Convert from buffer to timeout hdr */ >> + odp_timeout_hdr_t *tmo_hdr = >> + timeout_hdr_from_buf(*tmo_buf); > > as we discussed earlier this 2 definitions of tmo_hdr can be one on top of > timer_expire(). > int succ can be int ret and also defined on top. Yes they can but that would be a different coding style. It is OK to declare variables at beginning of a block, even Linux does it (see Petri's example). This has been OK since the beginning of time (the start of the Epoch, Jan. 1st 1970). > > Maxim. > >> + tmo_hdr->timer = tp_idx_to_handle(tp, idx); >> + tmo_hdr->user_ptr = tp->timers[idx].user_ptr; >> + /* expiration field filled in when timer expires >> */ >> + } >> + /* Else ignore buffers of other types */ >> odp_buffer_t old_buf = ODP_BUFFER_INVALID; >> #ifdef ODP_ATOMIC_U128 >> tick_buf_t new, old; >> @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool *tp, >> uint32_t idx, uint64_t tick) >> _odp_atomic_flag_clear(IDX2LOCK(idx)); >> #endif >> if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { >> - /* Fill in metadata fields in system timeout buffer */ >> + /* Fill in expiration tick if timeout event */ >> if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { >> /* Convert from buffer to timeout hdr */ >> odp_timeout_hdr_t *tmo_hdr = >> timeout_hdr_from_buf(tmo_buf); >> - tmo_hdr->timer = tp_idx_to_handle(tp, idx); >> tmo_hdr->expiration = exp_tck; >> - tmo_hdr->user_ptr = tim->user_ptr; >> + /* timer and user_ptr fields filled in when timer >> + * was set */ >> } >> - /* Else ignore buffers of other types */ >> + /* Else ignore events of other types */ >> /* Post the timeout to the destination queue */ >> int rc = odp_queue_enq(tim->queue, >> odp_buffer_to_event(tmo_buf)); > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 03/10/15 18:08, Ola Liljedahl wrote: > On 10 March 2015 at 15:59, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> On 03/10/15 17:43, Ola Liljedahl wrote: >>> Ensure that the timeout user_ptr and timer fields are set when the >>> corresponding timer is immediately cancelled. >>> https://bugs.linaro.org/show_bug.cgi?id=1313 >>> >>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >>> --- >>> (This document/code contribution attached is provided under the terms of >>> agreement LES-LTM-21309) >>> >>> Passes odp_timer validation with the new odp_timer_cancel() test from >>> Petri. >>> >>> platform/linux-generic/odp_timer.c | 18 ++++++++++++++---- >>> 1 file changed, 14 insertions(+), 4 deletions(-) >>> >>> diff --git a/platform/linux-generic/odp_timer.c >>> b/platform/linux-generic/odp_timer.c >>> index 61a02b6..6b48d2e 100644 >>> --- a/platform/linux-generic/odp_timer.c >>> +++ b/platform/linux-generic/odp_timer.c >>> @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx, >>> #endif >>> } else { >>> /* We have a new timeout buffer which replaces any old one >>> */ >>> + /* Fill in header fields if timeout event */ >>> + if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { >>> + /* Convert from buffer to timeout hdr */ >>> + odp_timeout_hdr_t *tmo_hdr = >>> + timeout_hdr_from_buf(*tmo_buf); >> as we discussed earlier this 2 definitions of tmo_hdr can be one on top of >> timer_expire(). >> int succ can be int ret and also defined on top. > Yes they can but that would be a different coding style. It is OK to > declare variables at beginning of a block, even Linux does it (see > Petri's example). This has been OK since the beginning of time (the > start of the Epoch, Jan. 1st 1970). My point here is if you use one variable 2 times in one function, than most probably you need define it only once. Do you agree? Maxim. > >> Maxim. >> >>> + tmo_hdr->timer = tp_idx_to_handle(tp, idx); >>> + tmo_hdr->user_ptr = tp->timers[idx].user_ptr; >>> + /* expiration field filled in when timer expires >>> */ >>> + } >>> + /* Else ignore buffers of other types */ >>> odp_buffer_t old_buf = ODP_BUFFER_INVALID; >>> #ifdef ODP_ATOMIC_U128 >>> tick_buf_t new, old; >>> @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool *tp, >>> uint32_t idx, uint64_t tick) >>> _odp_atomic_flag_clear(IDX2LOCK(idx)); >>> #endif >>> if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { >>> - /* Fill in metadata fields in system timeout buffer */ >>> + /* Fill in expiration tick if timeout event */ >>> if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { >>> /* Convert from buffer to timeout hdr */ >>> odp_timeout_hdr_t *tmo_hdr = >>> timeout_hdr_from_buf(tmo_buf); >>> - tmo_hdr->timer = tp_idx_to_handle(tp, idx); >>> tmo_hdr->expiration = exp_tck; >>> - tmo_hdr->user_ptr = tim->user_ptr; >>> + /* timer and user_ptr fields filled in when timer >>> + * was set */ >>> } >>> - /* Else ignore buffers of other types */ >>> + /* Else ignore events of other types */ >>> /* Post the timeout to the destination queue */ >>> int rc = odp_queue_enq(tim->queue, >>> odp_buffer_to_event(tmo_buf)); >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp
On 10 March 2015 at 16:11, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 03/10/15 18:08, Ola Liljedahl wrote: >> >> On 10 March 2015 at 15:59, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>> >>> On 03/10/15 17:43, Ola Liljedahl wrote: >>>> >>>> Ensure that the timeout user_ptr and timer fields are set when the >>>> corresponding timer is immediately cancelled. >>>> https://bugs.linaro.org/show_bug.cgi?id=1313 >>>> >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >>>> --- >>>> (This document/code contribution attached is provided under the terms of >>>> agreement LES-LTM-21309) >>>> >>>> Passes odp_timer validation with the new odp_timer_cancel() test from >>>> Petri. >>>> >>>> platform/linux-generic/odp_timer.c | 18 ++++++++++++++---- >>>> 1 file changed, 14 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/platform/linux-generic/odp_timer.c >>>> b/platform/linux-generic/odp_timer.c >>>> index 61a02b6..6b48d2e 100644 >>>> --- a/platform/linux-generic/odp_timer.c >>>> +++ b/platform/linux-generic/odp_timer.c >>>> @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx, >>>> #endif >>>> } else { >>>> /* We have a new timeout buffer which replaces any old >>>> one >>>> */ >>>> + /* Fill in header fields if timeout event */ >>>> + if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { >>>> + /* Convert from buffer to timeout hdr */ >>>> + odp_timeout_hdr_t *tmo_hdr = >>>> + timeout_hdr_from_buf(*tmo_buf); >>> >>> as we discussed earlier this 2 definitions of tmo_hdr can be one on top >>> of >>> timer_expire(). >>> int succ can be int ret and also defined on top. >> >> Yes they can but that would be a different coding style. It is OK to >> declare variables at beginning of a block, even Linux does it (see >> Petri's example). This has been OK since the beginning of time (the >> start of the Epoch, Jan. 1st 1970). > > > My point here is if you use one variable 2 times in one function, than most > probably you need define it only once. > Do you agree? No. There is no intrinsic value in having as few variable declarations as possible. If the scope becomes larger, this could be confusing to the programmers that try to understand the code. A larger scope means that the variables are valid longer (from a compiler perspective) even if the variables are not really valid from a runtime perspective. If I only need to use a variable in a certain block (because the corresponding value is only alive there), I want to declare it in that block and have it go out of scope when the block ends. Possibly another block in the same function needs a variable of the same type for some short-lived purpose. Then I will declare that variable in that block and not try to combine in with an unrelated variable in another block just because they are of the same type and used for similar purposes. > > Maxim. > >> >>> Maxim. >>> >>>> + tmo_hdr->timer = tp_idx_to_handle(tp, idx); >>>> + tmo_hdr->user_ptr = tp->timers[idx].user_ptr; >>>> + /* expiration field filled in when timer expires >>>> */ >>>> + } >>>> + /* Else ignore buffers of other types */ >>>> odp_buffer_t old_buf = ODP_BUFFER_INVALID; >>>> #ifdef ODP_ATOMIC_U128 >>>> tick_buf_t new, old; >>>> @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool *tp, >>>> uint32_t idx, uint64_t tick) >>>> _odp_atomic_flag_clear(IDX2LOCK(idx)); >>>> #endif >>>> if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { >>>> - /* Fill in metadata fields in system timeout buffer */ >>>> + /* Fill in expiration tick if timeout event */ >>>> if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { >>>> /* Convert from buffer to timeout hdr */ >>>> odp_timeout_hdr_t *tmo_hdr = >>>> timeout_hdr_from_buf(tmo_buf); >>>> - tmo_hdr->timer = tp_idx_to_handle(tp, idx); >>>> tmo_hdr->expiration = exp_tck; >>>> - tmo_hdr->user_ptr = tim->user_ptr; >>>> + /* timer and user_ptr fields filled in when >>>> timer >>>> + * was set */ >>>> } >>>> - /* Else ignore buffers of other types */ >>>> + /* Else ignore events of other types */ >>>> /* Post the timeout to the destination queue */ >>>> int rc = odp_queue_enq(tim->queue, >>>> odp_buffer_to_event(tmo_buf)); >>> >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/lng-odp > >
On 03/10/15 18:46, Ola Liljedahl wrote: > On 10 March 2015 at 16:11, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> On 03/10/15 18:08, Ola Liljedahl wrote: >>> On 10 March 2015 at 15:59, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>>> On 03/10/15 17:43, Ola Liljedahl wrote: >>>>> Ensure that the timeout user_ptr and timer fields are set when the >>>>> corresponding timer is immediately cancelled. >>>>> https://bugs.linaro.org/show_bug.cgi?id=1313 >>>>> >>>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >>>>> --- >>>>> (This document/code contribution attached is provided under the terms of >>>>> agreement LES-LTM-21309) >>>>> >>>>> Passes odp_timer validation with the new odp_timer_cancel() test from >>>>> Petri. >>>>> >>>>> platform/linux-generic/odp_timer.c | 18 ++++++++++++++---- >>>>> 1 file changed, 14 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/platform/linux-generic/odp_timer.c >>>>> b/platform/linux-generic/odp_timer.c >>>>> index 61a02b6..6b48d2e 100644 >>>>> --- a/platform/linux-generic/odp_timer.c >>>>> +++ b/platform/linux-generic/odp_timer.c >>>>> @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx, >>>>> #endif >>>>> } else { >>>>> /* We have a new timeout buffer which replaces any old >>>>> one >>>>> */ >>>>> + /* Fill in header fields if timeout event */ >>>>> + if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { >>>>> + /* Convert from buffer to timeout hdr */ >>>>> + odp_timeout_hdr_t *tmo_hdr = >>>>> + timeout_hdr_from_buf(*tmo_buf); >>>> as we discussed earlier this 2 definitions of tmo_hdr can be one on top >>>> of >>>> timer_expire(). >>>> int succ can be int ret and also defined on top. >>> Yes they can but that would be a different coding style. It is OK to >>> declare variables at beginning of a block, even Linux does it (see >>> Petri's example). This has been OK since the beginning of time (the >>> start of the Epoch, Jan. 1st 1970). >> >> My point here is if you use one variable 2 times in one function, than most >> probably you need define it only once. >> Do you agree? > No. > > There is no intrinsic value in having as few variable declarations as > possible. If the scope becomes larger, this could be confusing to the > programmers that try to understand the code. A larger scope means that > the variables are valid longer (from a compiler perspective) even if > the variables are not really valid from a runtime perspective. > > If I only need to use a variable in a certain block (because the > corresponding value is only alive there), I want to declare it in that > block and have it go out of scope when the block ends. Possibly > another block in the same function needs a variable of the same type > for some short-lived purpose. Then I will declare that variable in > that block and not try to combine in with an unrelated variable in > another block just because they are of the same type and used for > similar purposes. If you need to do this that means that this function is too long, and probably we need rework it to make more clear to everybody. Current timer_reset() is actual 4 functions depending on provided defines. Where ODP_ATOMIC_U128 is defined? How is it related to _ARM_ARCH? I did some simplifying of that timer functions. Please take a look. Thanks, Maxim. >> Maxim. >> >>>> Maxim. >>>> >>>>> + tmo_hdr->timer = tp_idx_to_handle(tp, idx); >>>>> + tmo_hdr->user_ptr = tp->timers[idx].user_ptr; >>>>> + /* expiration field filled in when timer expires >>>>> */ >>>>> + } >>>>> + /* Else ignore buffers of other types */ >>>>> odp_buffer_t old_buf = ODP_BUFFER_INVALID; >>>>> #ifdef ODP_ATOMIC_U128 >>>>> tick_buf_t new, old; >>>>> @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool *tp, >>>>> uint32_t idx, uint64_t tick) >>>>> _odp_atomic_flag_clear(IDX2LOCK(idx)); >>>>> #endif >>>>> if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { >>>>> - /* Fill in metadata fields in system timeout buffer */ >>>>> + /* Fill in expiration tick if timeout event */ >>>>> if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { >>>>> /* Convert from buffer to timeout hdr */ >>>>> odp_timeout_hdr_t *tmo_hdr = >>>>> timeout_hdr_from_buf(tmo_buf); >>>>> - tmo_hdr->timer = tp_idx_to_handle(tp, idx); >>>>> tmo_hdr->expiration = exp_tck; >>>>> - tmo_hdr->user_ptr = tim->user_ptr; >>>>> + /* timer and user_ptr fields filled in when >>>>> timer >>>>> + * was set */ >>>>> } >>>>> - /* Else ignore buffers of other types */ >>>>> + /* Else ignore events of other types */ >>>>> /* Post the timeout to the destination queue */ >>>>> int rc = odp_queue_enq(tim->queue, >>>>> odp_buffer_to_event(tmo_buf)); >>>> >>>> >>>> _______________________________________________ >>>> lng-odp mailing list >>>> lng-odp@lists.linaro.org >>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>
On 11 March 2015 at 13:16, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 03/10/15 18:46, Ola Liljedahl wrote: > >> On 10 March 2015 at 16:11, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> >>> On 03/10/15 18:08, Ola Liljedahl wrote: >>> >>>> On 10 March 2015 at 15:59, Maxim Uvarov <maxim.uvarov@linaro.org> >>>> wrote: >>>> >>>>> On 03/10/15 17:43, Ola Liljedahl wrote: >>>>> >>>>>> Ensure that the timeout user_ptr and timer fields are set when the >>>>>> corresponding timer is immediately cancelled. >>>>>> https://bugs.linaro.org/show_bug.cgi?id=1313 >>>>>> >>>>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >>>>>> --- >>>>>> (This document/code contribution attached is provided under the terms >>>>>> of >>>>>> agreement LES-LTM-21309) >>>>>> >>>>>> Passes odp_timer validation with the new odp_timer_cancel() test from >>>>>> Petri. >>>>>> >>>>>> platform/linux-generic/odp_timer.c | 18 ++++++++++++++---- >>>>>> 1 file changed, 14 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/platform/linux-generic/odp_timer.c >>>>>> b/platform/linux-generic/odp_timer.c >>>>>> index 61a02b6..6b48d2e 100644 >>>>>> --- a/platform/linux-generic/odp_timer.c >>>>>> +++ b/platform/linux-generic/odp_timer.c >>>>>> @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx, >>>>>> #endif >>>>>> } else { >>>>>> /* We have a new timeout buffer which replaces any >>>>>> old >>>>>> one >>>>>> */ >>>>>> + /* Fill in header fields if timeout event */ >>>>>> + if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { >>>>>> + /* Convert from buffer to timeout hdr */ >>>>>> + odp_timeout_hdr_t *tmo_hdr = >>>>>> + timeout_hdr_from_buf(*tmo_buf); >>>>>> >>>>> as we discussed earlier this 2 definitions of tmo_hdr can be one on >>>>> top >>>>> of >>>>> timer_expire(). >>>>> int succ can be int ret and also defined on top. >>>>> >>>> Yes they can but that would be a different coding style. It is OK to >>>> declare variables at beginning of a block, even Linux does it (see >>>> Petri's example). This has been OK since the beginning of time (the >>>> start of the Epoch, Jan. 1st 1970). >>>> >>> >>> My point here is if you use one variable 2 times in one function, than >>> most >>> probably you need define it only once. >>> Do you agree? >>> >> No. >> >> There is no intrinsic value in having as few variable declarations as >> possible. If the scope becomes larger, this could be confusing to the >> programmers that try to understand the code. A larger scope means that >> the variables are valid longer (from a compiler perspective) even if >> the variables are not really valid from a runtime perspective. >> >> If I only need to use a variable in a certain block (because the >> corresponding value is only alive there), I want to declare it in that >> block and have it go out of scope when the block ends. Possibly >> another block in the same function needs a variable of the same type >> for some short-lived purpose. Then I will declare that variable in >> that block and not try to combine in with an unrelated variable in >> another block just because they are of the same type and used for >> similar purposes. >> > > If you need to do this that means that this function is too long, and > probably we need > rework it to make more clear to everybody. I describe situations where it is better to have shorter-lived (shorter than the whole function) variable declarations and you just parrot "your function is too long". Variables in a function have different regions of aliveness and it make sense for the programmer to be explicit about this so that the compiler can warn if you are trying to use a variable when it does not have a valid value (think how useful it could be if you could make a pointer variable go out of scope after you have called free(), perhaps have you heard of those "use after free" bugs?). http://cwe.mitre.org/data/definitions/416.html Whether a functions is longer (or shorter) than some subjective optimum is a different question. Me thinks you just haven't programmed enough to appreciate such language features. But why should your inexperience limit the rest of us from writing better code? > Current timer_reset() is actual 4 functions depending on provided defines. > > Where ODP_ATOMIC_U128 is defined? How is it related to _ARM_ARCH? > https://patches.linaro.org/42855/ Not merged yet. _ARM_ARCH and ODP_ATOMIC_U128 are not related to each other. x86-64 supports 128-bit atomic operations (e.g. CMPXHG16) when compiled with -mcx16. A compiler flag is required since some early x86-64 implementations did not support CMPXHG16. ARMv8/AArch64 does support 128-bit atomic operations (load/store exclusive pair) but the compiler (e.g. GCC) does not generate those instructions yet. Because some targets don't support 128-bit SWAP and CAS is why I added some limited support for lock-less operations (e.g. reset reusing an existing timeout event, you only need to reset the expiration tick). On ARMv7, this requires fewer barriers (DMB) compared to using locks (barriers are best avoided on e.g. ARM and PPC for performance reasons). Resetting a timer is a really performance critical operation, you might be doing this multiple times per packet. > I did some simplifying of that timer functions. Please take a look. > Yes I can have a look. > Thanks, > Maxim. > > > > Maxim. >>> >>> Maxim. >>>>> >>>>> + tmo_hdr->timer = tp_idx_to_handle(tp, idx); >>>>>> + tmo_hdr->user_ptr = tp->timers[idx].user_ptr; >>>>>> + /* expiration field filled in when timer >>>>>> expires >>>>>> */ >>>>>> + } >>>>>> + /* Else ignore buffers of other types */ >>>>>> odp_buffer_t old_buf = ODP_BUFFER_INVALID; >>>>>> #ifdef ODP_ATOMIC_U128 >>>>>> tick_buf_t new, old; >>>>>> @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool *tp, >>>>>> uint32_t idx, uint64_t tick) >>>>>> _odp_atomic_flag_clear(IDX2LOCK(idx)); >>>>>> #endif >>>>>> if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { >>>>>> - /* Fill in metadata fields in system timeout buffer */ >>>>>> + /* Fill in expiration tick if timeout event */ >>>>>> if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) >>>>>> { >>>>>> /* Convert from buffer to timeout hdr */ >>>>>> odp_timeout_hdr_t *tmo_hdr = >>>>>> timeout_hdr_from_buf(tmo_buf); >>>>>> - tmo_hdr->timer = tp_idx_to_handle(tp, idx); >>>>>> tmo_hdr->expiration = exp_tck; >>>>>> - tmo_hdr->user_ptr = tim->user_ptr; >>>>>> + /* timer and user_ptr fields filled in when >>>>>> timer >>>>>> + * was set */ >>>>>> } >>>>>> - /* Else ignore buffers of other types */ >>>>>> + /* Else ignore events of other types */ >>>>>> /* Post the timeout to the destination queue */ >>>>>> int rc = odp_queue_enq(tim->queue, >>>>>> >>>>>> odp_buffer_to_event(tmo_buf)); >>>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> lng-odp mailing list >>>>> lng-odp@lists.linaro.org >>>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>>>> >>>> >>> >
Can we take this off line, it is straying from a technical discussion. On 11 March 2015 at 09:29, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 11 March 2015 at 13:16, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > >> On 03/10/15 18:46, Ola Liljedahl wrote: >> >>> On 10 March 2015 at 16:11, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>> >>>> On 03/10/15 18:08, Ola Liljedahl wrote: >>>> >>>>> On 10 March 2015 at 15:59, Maxim Uvarov <maxim.uvarov@linaro.org> >>>>> wrote: >>>>> >>>>>> On 03/10/15 17:43, Ola Liljedahl wrote: >>>>>> >>>>>>> Ensure that the timeout user_ptr and timer fields are set when the >>>>>>> corresponding timer is immediately cancelled. >>>>>>> https://bugs.linaro.org/show_bug.cgi?id=1313 >>>>>>> >>>>>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >>>>>>> --- >>>>>>> (This document/code contribution attached is provided under the >>>>>>> terms of >>>>>>> agreement LES-LTM-21309) >>>>>>> >>>>>>> Passes odp_timer validation with the new odp_timer_cancel() test from >>>>>>> Petri. >>>>>>> >>>>>>> platform/linux-generic/odp_timer.c | 18 ++++++++++++++---- >>>>>>> 1 file changed, 14 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/platform/linux-generic/odp_timer.c >>>>>>> b/platform/linux-generic/odp_timer.c >>>>>>> index 61a02b6..6b48d2e 100644 >>>>>>> --- a/platform/linux-generic/odp_timer.c >>>>>>> +++ b/platform/linux-generic/odp_timer.c >>>>>>> @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx, >>>>>>> #endif >>>>>>> } else { >>>>>>> /* We have a new timeout buffer which replaces any >>>>>>> old >>>>>>> one >>>>>>> */ >>>>>>> + /* Fill in header fields if timeout event */ >>>>>>> + if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) >>>>>>> { >>>>>>> + /* Convert from buffer to timeout hdr */ >>>>>>> + odp_timeout_hdr_t *tmo_hdr = >>>>>>> + timeout_hdr_from_buf(*tmo_buf); >>>>>>> >>>>>> as we discussed earlier this 2 definitions of tmo_hdr can be one on >>>>>> top >>>>>> of >>>>>> timer_expire(). >>>>>> int succ can be int ret and also defined on top. >>>>>> >>>>> Yes they can but that would be a different coding style. It is OK to >>>>> declare variables at beginning of a block, even Linux does it (see >>>>> Petri's example). This has been OK since the beginning of time (the >>>>> start of the Epoch, Jan. 1st 1970). >>>>> >>>> >>>> My point here is if you use one variable 2 times in one function, than >>>> most >>>> probably you need define it only once. >>>> Do you agree? >>>> >>> No. >>> >>> There is no intrinsic value in having as few variable declarations as >>> possible. If the scope becomes larger, this could be confusing to the >>> programmers that try to understand the code. A larger scope means that >>> the variables are valid longer (from a compiler perspective) even if >>> the variables are not really valid from a runtime perspective. >>> >>> If I only need to use a variable in a certain block (because the >>> corresponding value is only alive there), I want to declare it in that >>> block and have it go out of scope when the block ends. Possibly >>> another block in the same function needs a variable of the same type >>> for some short-lived purpose. Then I will declare that variable in >>> that block and not try to combine in with an unrelated variable in >>> another block just because they are of the same type and used for >>> similar purposes. >>> >> >> If you need to do this that means that this function is too long, and >> probably we need >> rework it to make more clear to everybody. > > I describe situations where it is better to have shorter-lived (shorter > than the whole function) variable declarations and you just parrot "your > function is too long". Variables in a function have different regions of > aliveness and it make sense for the programmer to be explicit about this so > that the compiler can warn if you are trying to use a variable when it does > not have a valid value (think how useful it could be if you could make a > pointer variable go out of scope after you have called free(), perhaps have > you heard of those "use after free" bugs?). > http://cwe.mitre.org/data/definitions/416.html > > Whether a functions is longer (or shorter) than some subjective optimum is > a different question. > > Me thinks you just haven't programmed enough to appreciate such language > features. But why should your inexperience limit the rest of us from > writing better code? > > > > >> Current timer_reset() is actual 4 functions depending on provided defines. >> >> Where ODP_ATOMIC_U128 is defined? How is it related to _ARM_ARCH? >> > https://patches.linaro.org/42855/ > Not merged yet. > > _ARM_ARCH and ODP_ATOMIC_U128 are not related to each other. > x86-64 supports 128-bit atomic operations (e.g. CMPXHG16) when compiled > with -mcx16. A compiler flag is required since some early x86-64 > implementations did not support CMPXHG16. > ARMv8/AArch64 does support 128-bit atomic operations (load/store exclusive > pair) but the compiler (e.g. GCC) does not generate those instructions yet. > > Because some targets don't support 128-bit SWAP and CAS is why I added > some limited support for lock-less operations (e.g. reset reusing an > existing timeout event, you only need to reset the expiration tick). On > ARMv7, this requires fewer barriers (DMB) compared to using locks (barriers > are best avoided on e.g. ARM and PPC for performance reasons). Resetting a > timer is a really performance critical operation, you might be doing this > multiple times per packet. > > > >> I did some simplifying of that timer functions. Please take a look. >> > Yes I can have a look. > > >> Thanks, >> Maxim. >> >> >> >> Maxim. >>>> >>>> Maxim. >>>>>> >>>>>> + tmo_hdr->timer = tp_idx_to_handle(tp, idx); >>>>>>> + tmo_hdr->user_ptr = tp->timers[idx].user_ptr; >>>>>>> + /* expiration field filled in when timer >>>>>>> expires >>>>>>> */ >>>>>>> + } >>>>>>> + /* Else ignore buffers of other types */ >>>>>>> odp_buffer_t old_buf = ODP_BUFFER_INVALID; >>>>>>> #ifdef ODP_ATOMIC_U128 >>>>>>> tick_buf_t new, old; >>>>>>> @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool >>>>>>> *tp, >>>>>>> uint32_t idx, uint64_t tick) >>>>>>> _odp_atomic_flag_clear(IDX2LOCK(idx)); >>>>>>> #endif >>>>>>> if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { >>>>>>> - /* Fill in metadata fields in system timeout buffer >>>>>>> */ >>>>>>> + /* Fill in expiration tick if timeout event */ >>>>>>> if (_odp_buffer_type(tmo_buf) == >>>>>>> ODP_EVENT_TIMEOUT) { >>>>>>> /* Convert from buffer to timeout hdr */ >>>>>>> odp_timeout_hdr_t *tmo_hdr = >>>>>>> timeout_hdr_from_buf(tmo_buf); >>>>>>> - tmo_hdr->timer = tp_idx_to_handle(tp, idx); >>>>>>> tmo_hdr->expiration = exp_tck; >>>>>>> - tmo_hdr->user_ptr = tim->user_ptr; >>>>>>> + /* timer and user_ptr fields filled in when >>>>>>> timer >>>>>>> + * was set */ >>>>>>> } >>>>>>> - /* Else ignore buffers of other types */ >>>>>>> + /* Else ignore events of other types */ >>>>>>> /* Post the timeout to the destination queue */ >>>>>>> int rc = odp_queue_enq(tim->queue, >>>>>>> >>>>>>> odp_buffer_to_event(tmo_buf)); >>>>>>> >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> lng-odp mailing list >>>>>> lng-odp@lists.linaro.org >>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>>>>> >>>>> >>>> >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp > >
On 11 March 2015 at 14:49, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > > > -----Original Message----- > > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > > bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl > > Sent: Tuesday, March 10, 2015 4:44 PM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH] linux-generic: odp_timer: set user_ptr for > > cancelled timeout > > > > Ensure that the timeout user_ptr and timer fields are set when the > > corresponding timer is immediately cancelled. > > https://bugs.linaro.org/show_bug.cgi?id=1313 > > > > Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> > > --- > > (This document/code contribution attached is provided under the terms of > > agreement LES-LTM-21309) > > > > Passes odp_timer validation with the new odp_timer_cancel() test from > > Petri. > > > > platform/linux-generic/odp_timer.c | 18 ++++++++++++++---- > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/platform/linux-generic/odp_timer.c b/platform/linux- > > generic/odp_timer.c > > index 61a02b6..6b48d2e 100644 > > --- a/platform/linux-generic/odp_timer.c > > +++ b/platform/linux-generic/odp_timer.c > > @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx, > > #endif > > } else { > > /* We have a new timeout buffer which replaces any old one > */ > > + /* Fill in header fields if timeout event */ > > Typo: "... if timeout event" => "... in timeout event" > Will fix. BTW should the timer implementation still be using buffers as the base class for timeouts? I think there is some more event-ification work to do here. > > + if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { > > + /* Convert from buffer to timeout hdr */ > > + odp_timeout_hdr_t *tmo_hdr = > > + timeout_hdr_from_buf(*tmo_buf); > > + tmo_hdr->timer = tp_idx_to_handle(tp, idx); > > + tmo_hdr->user_ptr = tp->timers[idx].user_ptr; > > + /* expiration field filled in when timer expires */ > > + } > > + /* Else ignore buffers of other types */ > > odp_buffer_t old_buf = ODP_BUFFER_INVALID; > > #ifdef ODP_ATOMIC_U128 > > tick_buf_t new, old; > > @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool *tp, > > uint32_t idx, uint64_t tick) > > _odp_atomic_flag_clear(IDX2LOCK(idx)); > > #endif > > if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { > > - /* Fill in metadata fields in system timeout buffer */ > > + /* Fill in expiration tick if timeout event */ > > Same typo. > Will fix. > > -Petri > > > if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { > > /* Convert from buffer to timeout hdr */ > > odp_timeout_hdr_t *tmo_hdr = > > timeout_hdr_from_buf(tmo_buf); > > - tmo_hdr->timer = tp_idx_to_handle(tp, idx); > > tmo_hdr->expiration = exp_tck; > > - tmo_hdr->user_ptr = tim->user_ptr; > > + /* timer and user_ptr fields filled in when timer > > + * was set */ > > } > > - /* Else ignore buffers of other types */ > > + /* Else ignore events of other types */ > > /* Post the timeout to the destination queue */ > > int rc = odp_queue_enq(tim->queue, > > odp_buffer_to_event(tmo_buf)); > > -- > > 1.9.1 > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 11 March 2015 at 15:26, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 11 March 2015 at 14:49, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > >> >> >> > -----Original Message----- >> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> > bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl >> > Sent: Tuesday, March 10, 2015 4:44 PM >> > To: lng-odp@lists.linaro.org >> > Subject: [lng-odp] [PATCH] linux-generic: odp_timer: set user_ptr for >> > cancelled timeout >> > >> > Ensure that the timeout user_ptr and timer fields are set when the >> > corresponding timer is immediately cancelled. >> > https://bugs.linaro.org/show_bug.cgi?id=1313 >> > >> > Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >> > --- >> > (This document/code contribution attached is provided under the terms of >> > agreement LES-LTM-21309) >> > >> > Passes odp_timer validation with the new odp_timer_cancel() test from >> > Petri. >> > >> > platform/linux-generic/odp_timer.c | 18 ++++++++++++++---- >> > 1 file changed, 14 insertions(+), 4 deletions(-) >> > >> > diff --git a/platform/linux-generic/odp_timer.c b/platform/linux- >> > generic/odp_timer.c >> > index 61a02b6..6b48d2e 100644 >> > --- a/platform/linux-generic/odp_timer.c >> > +++ b/platform/linux-generic/odp_timer.c >> > @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx, >> > #endif >> > } else { >> > /* We have a new timeout buffer which replaces any old >> one */ >> > + /* Fill in header fields if timeout event */ >> >> Typo: "... if timeout event" => "... in timeout event" >> > Will fix. > Now looking at what I wrote I actually wrote what I meant. But I should probably have phrased it like this: "If timeout event then fill in headers" or "When timeout event fill in headers". I hope this would be clearer. The specified event does not have to be a timeout event. > > BTW should the timer implementation still be using buffers as the base > class for timeouts? I think there is some more event-ification work to do > here. > > >> > + if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { >> > + /* Convert from buffer to timeout hdr */ >> > + odp_timeout_hdr_t *tmo_hdr = >> > + timeout_hdr_from_buf(*tmo_buf); >> > + tmo_hdr->timer = tp_idx_to_handle(tp, idx); >> > + tmo_hdr->user_ptr = tp->timers[idx].user_ptr; >> > + /* expiration field filled in when timer expires >> */ >> > + } >> > + /* Else ignore buffers of other types */ >> > odp_buffer_t old_buf = ODP_BUFFER_INVALID; >> > #ifdef ODP_ATOMIC_U128 >> > tick_buf_t new, old; >> > @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool *tp, >> > uint32_t idx, uint64_t tick) >> > _odp_atomic_flag_clear(IDX2LOCK(idx)); >> > #endif >> > if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { >> > - /* Fill in metadata fields in system timeout buffer */ >> > + /* Fill in expiration tick if timeout event */ >> >> Same typo. >> > Will fix. > > >> >> -Petri >> >> > if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { >> > /* Convert from buffer to timeout hdr */ >> > odp_timeout_hdr_t *tmo_hdr = >> > timeout_hdr_from_buf(tmo_buf); >> > - tmo_hdr->timer = tp_idx_to_handle(tp, idx); >> > tmo_hdr->expiration = exp_tck; >> > - tmo_hdr->user_ptr = tim->user_ptr; >> > + /* timer and user_ptr fields filled in when timer >> > + * was set */ >> > } >> > - /* Else ignore buffers of other types */ >> > + /* Else ignore events of other types */ >> > /* Post the timeout to the destination queue */ >> > int rc = odp_queue_enq(tim->queue, >> > odp_buffer_to_event(tmo_buf)); >> > -- >> > 1.9.1 >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> > >
diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c index 61a02b6..6b48d2e 100644 --- a/platform/linux-generic/odp_timer.c +++ b/platform/linux-generic/odp_timer.c @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx, #endif } else { /* We have a new timeout buffer which replaces any old one */ + /* Fill in header fields if timeout event */ + if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { + /* Convert from buffer to timeout hdr */ + odp_timeout_hdr_t *tmo_hdr = + timeout_hdr_from_buf(*tmo_buf); + tmo_hdr->timer = tp_idx_to_handle(tp, idx); + tmo_hdr->user_ptr = tp->timers[idx].user_ptr; + /* expiration field filled in when timer expires */ + } + /* Else ignore buffers of other types */ odp_buffer_t old_buf = ODP_BUFFER_INVALID; #ifdef ODP_ATOMIC_U128 tick_buf_t new, old; @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool *tp, uint32_t idx, uint64_t tick) _odp_atomic_flag_clear(IDX2LOCK(idx)); #endif if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { - /* Fill in metadata fields in system timeout buffer */ + /* Fill in expiration tick if timeout event */ if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { /* Convert from buffer to timeout hdr */ odp_timeout_hdr_t *tmo_hdr = timeout_hdr_from_buf(tmo_buf); - tmo_hdr->timer = tp_idx_to_handle(tp, idx); tmo_hdr->expiration = exp_tck; - tmo_hdr->user_ptr = tim->user_ptr; + /* timer and user_ptr fields filled in when timer + * was set */ } - /* Else ignore buffers of other types */ + /* Else ignore events of other types */ /* Post the timeout to the destination queue */ int rc = odp_queue_enq(tim->queue, odp_buffer_to_event(tmo_buf));
Ensure that the timeout user_ptr and timer fields are set when the corresponding timer is immediately cancelled. https://bugs.linaro.org/show_bug.cgi?id=1313 Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> --- (This document/code contribution attached is provided under the terms of agreement LES-LTM-21309) Passes odp_timer validation with the new odp_timer_cancel() test from Petri. platform/linux-generic/odp_timer.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)