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

Only support set_cpu_affinity with glibc for musl compat #1517

Merged
merged 1 commit into from
Jun 28, 2022

Conversation

aditsachde
Copy link
Contributor

Originating Project/Creator @aditsachde
Affected Component Os/Task
Affected Architectures(s)
Related Issue(s) #1507
Has Unit Tests (y/n) n
Builds Without Errors (y/n) Let CI run
Unit Tests Pass (y/n) Let CI run
Documentation Included (y/n)

Change Description

Only support set_cpu_affinity when using glibc. Detects glibc in a way that will not break due to glibc updates.

Rationale

This PR is meant as a discussion and not necessarily merged in as is. We are aiming to deploy fprime on the Onion Omega2, whose toolchain uses musl instead of glibc. This is one of the two changes (#1508) required to support building fprime with musl.

As mentioned in this discussion (#1507), it would be preferred to be able to set cpu affinity when musl. However, musl only supports setting cpu affinity after starting the thread using the pthread_setaffinity_np function, and not before, using pthread_attr_setaffinity_np.

It is possible to change the ordering, so that fprime sets CPU affinity after starting the thread. This then raises the question of what to do if starting the thread succeeds, but setting CPU affinity fails. If there is a way to acceptably handle this case, then it would make sense to switch this ordering and use pthread_setaffinity_np instead.

I would appreciate any thoughts on the best way this, and on musl compatibility in general.

Testing/Review Recommendations

This PR was opened to as a discussion, as mentioned in rational. For testing, CI should validate the change.

Future Work

Add a CI pipeline to run build and integration tests on a musl based system.

@github-actions
Copy link

github-actions bot commented Jun 20, 2022

@check-spelling-bot Report

Unrecognized words, please review:

  • glibc
Previously acknowledged words that are now absent aadl Accu adoc alignedallocator asciidoctor autodetect autonumbering awt bavail bbd bc bithacks BLSP bootup capout chown cinttypes Classloader classpath compat componentaction compositestructures concat configurator creatingdocsetswithdoxygen deserializer Dinstall dll doall Donatas donsim ecore eps errstr esac fprim FPRIMEPROTOCOL getenv gethostbyname getmtime getppid gettempdir gmail Gnd GNDIF groupadd groupmod hdp HOMEPAGE hostent ifchange IFXML IJET includefile inorder INSTALLDIR instanceof interoperability Inttype isfgen isfpluginexec isfxmlwriter itcl javabuilder javac javanature javax jdt jf JFile jmi JOption junit kevensen magicdraw mcternan mdbasiccomponents mdinternalstructures mdkernel mdports mdprofiles mdxml mdzip memoization memoize mngr mpmcs mscgen nasafprime netdb Netscape's NGAT nh nio nomagic nondetached nroff OMG's OS'es ovrTrace peeker placeholders PLUGINDIR ppid Prepends println propvals PROTOCOLINTERFACE PYPI refman RHEL RPISCHEDCONTEXTS saikiranra seander Simkunas SOCKETIPDRIVERTYPES SQL's sramanan startword strcat strcpy submenu tcl templating textui tkgui tmpd tmpdir toclevels toolbar ubuntu uk ul uml urllib useradd usermod ve virtualenv vmsize vn watney workaround workspaces Xmx Xss Xvfb
Some files were were automatically ignored

These sample patterns would exclude them:

^Drv/BlockDriver/BlockDriver\.hpp$
^Drv/LinuxGpioDriver/LinuxGpioDriver\.hpp$
^Drv/LinuxSpiDriver/LinuxSpiDriver\.hpp$
^Drv/TcpClient/TcpClient\.hpp$
^Fw/Types/Linux/StandardTypes\.hpp$
^Svc/LinuxTime/LinuxTime\.hpp$
^Svc/PrmDb/PrmDb\.hpp$
^Svc/TlmChan/TlmChan\.hpp$
^requirements\.txt$

You should consider adding them to:

.github/actions/spelling/excludes.txt

File matching is via Perl regular expressions.

To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:aditsachde/fprime.git repository
on the patch-2 branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
(cat '.github/actions/spelling/excludes.txt' - <<EOF
$should_exclude_patterns
EOF
) |grep .|
sort -f |
uniq > '.github/actions/spelling/excludes.txt.temp' &&
mv '.github/actions/spelling/excludes.txt.temp' '.github/actions/spelling/excludes.txt'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/nasa/fprime/issues/comments/1160726663" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  

should_exclude_patterns=$(perl -e '$/=undef;
$_=<>;
exit unless s{(?:You should consider excluding directory paths|You should consider adding them to).*}{}s;
s{.*These sample patterns would exclude them:}{}s;
s{.*\`\`\`([^`]*)\`\`\`.*}{$1}m;
print' < "$comment_body" | grep . || true)

update_files
rm $comment_body
git add -u

Copy link
Contributor

@Joshua-Anderson Joshua-Anderson left a comment

Choose a reason for hiding this comment

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

Looks great - this has heritage in how we handle MacOS, where we also don't support thread affinity.

It would be great to support thread affinity on musl, but agree with your judgement call that being deterministic by setting affinity prior to thread creation is more important than portability.

Hopefully musl gains some ability to set thread affinity prior to creation at some point.

Os/Posix/Task.cpp Outdated Show resolved Hide resolved
Os/Posix/Task.cpp Outdated Show resolved Hide resolved
@aditsachde
Copy link
Contributor Author

Including features.h isn't strictly needed, since the __GLIBC__ macro ends up being pulled into scope anyways, but since the macro is need for compilation, it seems right to explicitly include the header file.

@aditsachde aditsachde requested a review from LeStarch June 27, 2022 20:48
Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

Looks good to me. @Joshua-Anderson one more once-over?

Copy link
Contributor

@Joshua-Anderson Joshua-Anderson left a comment

Choose a reason for hiding this comment

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

LGTM!

@LeStarch LeStarch merged commit d5ad6b8 into nasa:devel Jun 28, 2022
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.

3 participants