On 5 November 2014 20:12, Victor Kamensky <[email protected]> wrote:
> Hi Ola,
>
> Below is an example of one issue I noticed.
>
> If you would post arm counter implementation in separate
> patch, I would be able to comment on all relevant places,
> but with current huge patch I just give one example. Also if it
> would be separate patch, we would be able to work on it in
> separate thread, till we get it right. But if you would continue
> to repost big whole patch with each iteration working on this
> issue, it would be quite a pain for me to find out what has
> changed, although I would just had one thing in mind - counter
> ARM V7 implementation.
I could send preliminary patches with just the ARMv7 implementation
but the complete patch should have support for all architectures me
think. At least ARMv7, ARMv8/AArch64, MIPS64/OCTEON and x86(-64)
because those are already supported in the existing code and I don't want
to be accused of removing functionality.
>
>> +/**
>> + * Atomic add to 64-bit counter variable
>> + *
>> + * @param ptr Pointer to a 64-bit counter variable
>> + * @param incr The value to be added to the counter variable
>> + */
>> +static inline void odp_counter64_add(odp_counter64_t *ptr, uint64_t incr)
>> +{
>> +#if defined __arm__ /* A32/T32 ISA */
>> + uint64_t old_val;
>> + int status;
>> + do {
>> + __asm __volatile("ldrexd %0, %H0, [%2]\t\n"
>> + "adds %0, %0, %3\t\n"
>> + "adc %H0, %H3\t\n"
>> + "strexd %1, %0, %H0, [%2]"
>
> Above looks very wrong to me. Did you test that on BE
> system? Please see
No I did not test on BE system as I don't any such available. I was reading a
page describing GCC inline assembler for ARM. Either it did not mention BE
(quite possible as few people have used ARM in BE mode before) or I missed it.
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2245f92498b216b50e744423bde17626287409d8
>
> You should use %Q %R instead of %H.
Thanks. Just to be sure, using %Q %R is endian neutral so I won't need any
ifdef for big and little endian?
>
> The same issue exist other ARM V7 counter64
> functions.
OK.
>
> Thanks,
> Victor
>
>> + : "=&r"(old_val), "=&r"(status)
>> + : "r"(&ptr->v), "r"(incr)
>> + : );
>> + } while (odp_unlikely(status != 0)); /* Retry until write succeeds */
>> +#elif defined __OCTEON__
>> + __asm __volatile("saad %[inc], (%[base])"
>> + : "+m" (*ptr)
>> + : [inc] "r" (incr), [base] "r" (ptr)
>> + : );
>> +#elif defined __x86_64__
>> + /* Generates good code on x86_64 */
>> + (void)__sync_fetch_and_add(&ptr->v, incr);
>> +#else
>> +/* Warning odp_counter64_add() may not be efficiently implemented */
>> + (void)__sync_fetch_and_add(&ptr->v, incr);
>> +#endif
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
