New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On Linux, ARMv5tel does not have all functions available #3

Closed
nixpanic opened this Issue Mar 3, 2012 · 7 comments

Comments

Projects
None yet
2 participants
@nixpanic

nixpanic commented Mar 3, 2012

It seems that libatomic_ops provides an optimized implementation for ARMv6 and newer architectures. Unfortunately there are no asm functions for ARMv5, which should fallback automatically to the pthread implementation.

This proposed patch is already available in the Fedora package. This change should provide the pthread implementation when atomic_ops.h is included on ARMv5 and earlier architectures.

Please review and include this in the next release if it make sense. Thanks!

@ivmai

This comment has been minimized.

Show comment
Hide comment
@ivmai

ivmai Mar 3, 2012

Owner

Hi Niels,

Thank you for pointing us to this patch but IMHO this is a logically incorrect patch.
By libatomic_ops contract:

  1. if some operation is not available then AO_HAVE_x just not defined;
  2. for CAS ops, if we strictly need it then we should define AO_REQUIRE_CAS causing automatic fall-back on demand.
  3. fetch_and_add is derived from CAS in the worth case, so you should define AO_REQUIRE_CAS as well if you need atomic inc (this seems to be not explicitly specified in the docs but should be).

Looking into arm.h (of master branch), the deference between, roughly speaking, ARMv5 and ARMv6 is in presence of CAS.

Please point me at if I misunderstand something of your post.

Regards.

03 03 2012, 19:02 Niels de Vos reply@reply.github.com:

It seems that libatomic_ops provides an optimized implementation for ARMv6 and newer architectures. Unfortunately there are no asm functions for ARMv5, which should fallback automatically to the pthread implementation.

This proposed patch is already available in the Fedora package. This change should provide the pthread implementation when atomic_ops.h is included on ARMv5 and earlier architectures.

Please review and include this in the next release if it make sense. Thanks!


Reply to this email directly or view it on GitHub:
#3

Owner

ivmai commented Mar 3, 2012

Hi Niels,

Thank you for pointing us to this patch but IMHO this is a logically incorrect patch.
By libatomic_ops contract:

  1. if some operation is not available then AO_HAVE_x just not defined;
  2. for CAS ops, if we strictly need it then we should define AO_REQUIRE_CAS causing automatic fall-back on demand.
  3. fetch_and_add is derived from CAS in the worth case, so you should define AO_REQUIRE_CAS as well if you need atomic inc (this seems to be not explicitly specified in the docs but should be).

Looking into arm.h (of master branch), the deference between, roughly speaking, ARMv5 and ARMv6 is in presence of CAS.

Please point me at if I misunderstand something of your post.

Regards.

03 03 2012, 19:02 Niels de Vos reply@reply.github.com:

It seems that libatomic_ops provides an optimized implementation for ARMv6 and newer architectures. Unfortunately there are no asm functions for ARMv5, which should fallback automatically to the pthread implementation.

This proposed patch is already available in the Fedora package. This change should provide the pthread implementation when atomic_ops.h is included on ARMv5 and earlier architectures.

Please review and include this in the next release if it make sense. Thanks!


Reply to this email directly or view it on GitHub:
#3

@nixpanic

This comment has been minimized.

Show comment
Hide comment
@nixpanic

nixpanic Mar 5, 2012

Thanks for the reply!

I will try to compile the sources when defining AO_REQUIRE_CAS and see if that works and report later again.

nixpanic commented Mar 5, 2012

Thanks for the reply!

I will try to compile the sources when defining AO_REQUIRE_CAS and see if that works and report later again.

@nixpanic

This comment has been minimized.

Show comment
Hide comment
@nixpanic

nixpanic Mar 7, 2012

Hi Ivan,

unfortunately does not seem to be sufficient to define AO_REQUIRE_CAS on ARMv5tel. The Fedora Bug 799153 contains a little test-case that can not be compiled without defining AO_USE_PTHREAD_DEFS.

Do you think this could be a bug in the path when AO_REQUIRE_CAS is defined?

Thanks,
Niels

nixpanic commented Mar 7, 2012

Hi Ivan,

unfortunately does not seem to be sufficient to define AO_REQUIRE_CAS on ARMv5tel. The Fedora Bug 799153 contains a little test-case that can not be compiled without defining AO_USE_PTHREAD_DEFS.

Do you think this could be a bug in the path when AO_REQUIRE_CAS is defined?

Thanks,
Niels

@ivmai

This comment has been minimized.

Show comment
Hide comment
@ivmai

ivmai Mar 7, 2012

Owner

Hi Niels,

07 03 2012, 12:32 Niels de Vos reply@reply.github.com:

Hi Ivan,

unfortunately does not seem to be sufficient to define AO_REQUIRE_CAS on ARMv5tel. The Fedora Bug 799153 contains a little test-case that can not be compiled without defining AO_USE_PTHREAD_DEFS.

Do you think this could be a bug in the path when AO_REQUIRE_CAS is defined?

What do you mean by "path"?

The test case is:

// test.c
#include <atomic_ops.h> // I'd rather use here: #include "atomic_ops.h>"
#include <sys/types.h>

int main(void)
{
volatile size_t i = 1234; // I'd rather use AO_t here
return AO_fetch_and_add1(&i);
}

I tried both current release, current master branch and libatomic_ops-7_2alpha6.

First try without AO_REQUIRE_CAS:
~/arm-2010.09/bin/arm-none-linux-gnueabi-gcc --sysroot ~/arm-2010.09/sysroot -I src/ -march=armv5te test.c src/atomic_ops.c

/tmp/cca1N8ZK.o: In function main': test.c:(.text+0x1c): undefined reference toAO_fetch_and_add1'
collect2: ld returned 1 exit status

Second try:
~/arm-2010.09/bin/arm-none-linux-gnueabi-gcc --sysroot ~/arm-2010.09/sysroot -I src/ -DAO_REQUIRE_CAS -march=armv5te test.c src/atomic_ops.c

Success.
So, I can't reproduce Fedora Bug 799153 (i.e. you don't need AO_USE_PTHREAD_DEFS here)

Regards.

Thanks,
Niels


Reply to this email directly or view it on GitHub:
#3 (comment)

Owner

ivmai commented Mar 7, 2012

Hi Niels,

07 03 2012, 12:32 Niels de Vos reply@reply.github.com:

Hi Ivan,

unfortunately does not seem to be sufficient to define AO_REQUIRE_CAS on ARMv5tel. The Fedora Bug 799153 contains a little test-case that can not be compiled without defining AO_USE_PTHREAD_DEFS.

Do you think this could be a bug in the path when AO_REQUIRE_CAS is defined?

What do you mean by "path"?

The test case is:

// test.c
#include <atomic_ops.h> // I'd rather use here: #include "atomic_ops.h>"
#include <sys/types.h>

int main(void)
{
volatile size_t i = 1234; // I'd rather use AO_t here
return AO_fetch_and_add1(&i);
}

I tried both current release, current master branch and libatomic_ops-7_2alpha6.

First try without AO_REQUIRE_CAS:
~/arm-2010.09/bin/arm-none-linux-gnueabi-gcc --sysroot ~/arm-2010.09/sysroot -I src/ -march=armv5te test.c src/atomic_ops.c

/tmp/cca1N8ZK.o: In function main': test.c:(.text+0x1c): undefined reference toAO_fetch_and_add1'
collect2: ld returned 1 exit status

Second try:
~/arm-2010.09/bin/arm-none-linux-gnueabi-gcc --sysroot ~/arm-2010.09/sysroot -I src/ -DAO_REQUIRE_CAS -march=armv5te test.c src/atomic_ops.c

Success.
So, I can't reproduce Fedora Bug 799153 (i.e. you don't need AO_USE_PTHREAD_DEFS here)

Regards.

Thanks,
Niels


Reply to this email directly or view it on GitHub:
#3 (comment)

@nixpanic

This comment has been minimized.

Show comment
Hide comment
@nixpanic

nixpanic Mar 7, 2012

Many thanks for looking into this, Ivan!

I'll have to dig a little further what causes this build to fail. Maybe defining AO_USE_PTHREAD_DEFS triggers some side effect somewhere...

When I have found an acceptable solution, I'll inform you again.

nixpanic commented Mar 7, 2012

Many thanks for looking into this, Ivan!

I'll have to dig a little further what causes this build to fail. Maybe defining AO_USE_PTHREAD_DEFS triggers some side effect somewhere...

When I have found an acceptable solution, I'll inform you again.

@ivmai

This comment has been minimized.

Show comment
Hide comment
@ivmai

ivmai Mar 12, 2012

Owner

Hi Neils,

Any news? (We're close to next libatomic_ops release, so it would be good to fix this issue if you know the proper solution.)

07 03 2012, 14:00 Niels de Vos reply@reply.github.com:

Many thanks for looking into this, Ivan!

I'll have to dig a little further what causes this build to fail. Maybe defining AO_USE_PTHREAD_DEFS triggers some side effect somewhere...

When I have found an acceptable solution, I'll inform you again.


Reply to this email directly or view it on GitHub:
#3 (comment)

Owner

ivmai commented Mar 12, 2012

Hi Neils,

Any news? (We're close to next libatomic_ops release, so it would be good to fix this issue if you know the proper solution.)

07 03 2012, 14:00 Niels de Vos reply@reply.github.com:

Many thanks for looking into this, Ivan!

I'll have to dig a little further what causes this build to fail. Maybe defining AO_USE_PTHREAD_DEFS triggers some side effect somewhere...

When I have found an acceptable solution, I'll inform you again.


Reply to this email directly or view it on GitHub:
#3 (comment)

@nixpanic

This comment has been minimized.

Show comment
Hide comment
@nixpanic

nixpanic Mar 12, 2012

Hi Ivan,

no sorry. I do not have any news yet, and will not be able to test in more detail on a real ARMv5 within the next few weeks.
No need to wait for this issue to be solved within libatomic_ops, as it may well be something else.

Thanks,
Niels

nixpanic commented Mar 12, 2012

Hi Ivan,

no sorry. I do not have any news yet, and will not be able to test in more detail on a real ARMv5 within the next few weeks.
No need to wait for this issue to be solved within libatomic_ops, as it may well be something else.

Thanks,
Niels

@ivmai ivmai closed this Apr 4, 2012

ivmai added a commit that referenced this issue Aug 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment