Skip to content
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

Mac allow system ucontext #90

Merged
merged 2 commits into from Sep 8, 2021
Merged

Mac allow system ucontext #90

merged 2 commits into from Sep 8, 2021

Conversation

olivier-snl
Copy link
Collaborator

No description provided.

@olivier-snl
Copy link
Collaborator Author

This still needs some cleanup, but the intent here is to enable initial support for m1 macs using the system ucontext implementation while we work on support for assembly-based fast context switching for arm64 (primarily being developed on our ThunderX2 Linux arm systems for now). The initial support given here requires the changes in this pull request, along with the following build recipe:

env CFLAGS=-D_XOPEN_SOURCE ./configure --disable-fastcontext --with-default-stack-size=65536 

Additionally, you'll see warnings about using deprecated functions, which you can suppress via compiler flags if they bother you.

@ronawho was wondering about why we had disallowed the use of the system ucontext on mac systems in the first place. I don't have an answer to that, except that I have found that our preferred default small stack sizes are not viable for it: signal.h specifies a minimum of 32k and a maximum of 128k. All tests pass for me using 64k. Not sure if @m3m0ryh0l3 had a particular reason in mind.

@ronawho
Copy link
Contributor

ronawho commented Aug 19, 2021

@olivier-snl I don't have access to an m1 mac to test on, but I'm wondering if we can automate this detection instead of forcing users to reconfigure.

We have a lot of users coming from a pure python world and many of them are not comfortable or familiar with reconfiguring.

@janciesko
Copy link
Collaborator

janciesko commented Aug 25, 2021

Tested on x86 Mac that ./configure && make check works for both setting --disable-fastcontext and not setting --disable-fastcontext. M1 Mac requires the use of --disable-fastcontext. With setting this option on M1 Mac both, configure and make check succeed. We are working on the fast context implementation on M1 Mac hardware.

@janciesko
Copy link
Collaborator

We could add a autoconf check for M1 MAC and set --disable-fastcontext in that case.

ronawho added a commit to chapel-lang/chapel that referenced this pull request Sep 1, 2021
Default to fifo tasking layer on arm based macs

[reviewed by @bradcray and @gbtitus]

Qthreads doesn't currently support arm based macs. Configure fails with
`error: What kind of a Mac is this?` while trying to figure out what
native assembly to use for context switching. We started looking at
using system `ucontext` in sandialabs/qthreads#90, but given how close our
release is we want to default to fifo since we know some users have had
success with that.

Resolves Cray/chapel-private#2424
Part of #17911
@janciesko janciesko marked this pull request as ready for review September 7, 2021 15:45
@janciesko janciesko self-requested a review September 7, 2021 15:45
*-darwin*)
qthread_cv_swapcontext_works=no
;;
# *-darwin*)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since current x86 and M1 Macs seemingly support system ucontext, I think it is ok to drop this case statement.

Copy link
Collaborator

@janciesko janciesko Sep 7, 2021

Choose a reason for hiding this comment

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

Consider adding something like
AS_IF([test "$host" in *-darvin*], [qt_host_based_enable_fastcontext=no], [qt_host_based_enable_fastcontext=yes] ;;
to L37 to disable fast context automatically on M1 Macs while the support is not available yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

darvin -> darwin. But really, it's the combo of darwin and aarch64 (however that parses). We don't want to disable fast context on earlier macs.

Copy link
Collaborator

@janciesko janciesko Sep 7, 2021

Choose a reason for hiding this comment

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

Yes, that combo should come together in L37 if I see this correctly when adding that additional check for Darwin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/janciesko/qthreads/tree/mac-allow-system-ucontext-2

This is not correct. m1 macs are arm v8 not arm v7. The arch to AC will be something like arm64 or aarch64.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this was more an idea / place holder. We'd need to add the right case depending on what config gives us.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can match on aarch64-*-darwin* | arm64*-*-darwin*

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, since it shouldn't really match any of the architectures in the case, it should fall through without any change. I just tried from the qthreads repo branch (without the m4 change and without the disable fast context switch on configure) and it worked just fine.

@janciesko
Copy link
Collaborator

Superseded by #92

@janciesko janciesko merged commit ee4a774 into main Sep 8, 2021
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.

None yet

3 participants