Skip to content

Conversation

@xiaoxiang781216
Copy link
Contributor

Summary

  • libc: Move the common implementation of up_testset to libc/machine

Impact

spinlock

Testing

Pass CI

@Ouss4
Copy link
Member

Ouss4 commented May 17, 2022

@xiaoxiang781216 a fix for the macOS failure on ESP32 configs is here: #6281 however I see other ARM related changes, seem to be build related.

@xiaoxiang781216
Copy link
Contributor Author

@xiaoxiang781216 a fix for the macOS failure on ESP32 configs is here: #6281 however I see other ARM related changes, seem to be build related.

Done.

@xiaoxiang781216 xiaoxiang781216 force-pushed the spinlock branch 2 times, most recently from 53babed to 998bacf Compare May 18, 2022 17:45
Copy link
Member

@Ouss4 Ouss4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good.
I'm just wondering if arch_atomic.c is the best place for the up_testset function.

cc. @gustavonihei

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
@xiaoxiang781216
Copy link
Contributor Author

Change looks good. I'm just wondering if arch_atomic.c is the best place for the up_testset function.

cc. @gustavonihei

up_testset is one of atomic function. Actually, it isn't good to introduce up_testset since atomic is part of C/C++ standard. It's better to use the standard atomic api, and implement them on the old toolchain by hand.

@gustavonihei
Copy link
Contributor

Change looks good. I'm just wondering if arch_atomic.c is the best place for the up_testset function.
cc. @gustavonihei

up_testset is one of atomic function.

The part that makes this new location a bit weird is that arch_atomic.c is under libc, and up_testset is specific for spinlocks.

Actually, it isn't good to introduce up_testset since atomic is part of C/C++ standard. It's better to use the standard atomic api, > and implement them on the old toolchain by hand.

So another option would be to leave up_testset in its place and change it to use the C11 function. And then on arch_atomic.c we could provide the implementation for the GCC/Clang builtins for those archs that don't support.

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented May 19, 2022

Ok, I drop up_testset from PR, let's create a new PR to remove up_testset and call atomic api in spinlock instead.

@xiaoxiang781216
Copy link
Contributor Author

Here is #6283

Copy link
Contributor

@gustavonihei gustavonihei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xiaoxiang781216 xiaoxiang781216 force-pushed the spinlock branch 2 times, most recently from be315dd to 2e99215 Compare May 19, 2022 19:56
@Ouss4
Copy link
Member

Ouss4 commented May 19, 2022

Here is #6283

Recursion. :)

@xiaoxiang781216
Copy link
Contributor Author

should be #6301 :)

@gustavonihei gustavonihei merged commit ba6ee2a into apache:master May 20, 2022
@xiaoxiang781216 xiaoxiang781216 deleted the spinlock branch May 20, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants