Can we take this off line, it is straying from a technical discussion. On 11 March 2015 at 09:29, Ola Liljedahl <[email protected]> wrote:
> On 11 March 2015 at 13:16, Maxim Uvarov <[email protected]> wrote: > >> On 03/10/15 18:46, Ola Liljedahl wrote: >> >>> On 10 March 2015 at 16:11, Maxim Uvarov <[email protected]> wrote: >>> >>>> On 03/10/15 18:08, Ola Liljedahl wrote: >>>> >>>>> On 10 March 2015 at 15:59, Maxim Uvarov <[email protected]> >>>>> 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 <[email protected]> >>>>>>> --- >>>>>>> (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 >>>>>> [email protected] >>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>>>>> >>>>> >>>> >> > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp > > -- Mike Holmes Technical Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
