-
Notifications
You must be signed in to change notification settings - Fork 30
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
[WIP] OCaml 5.0 support #122
Conversation
For the ocaml-solo5 devs: Feel free to ask questions, rebase the branch, add changes to this branch (you should have the rights to, though don't forget to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice some minor things and had some questions, also wrote a mini strerror_r.
*OBJ = *OBJ | ARG; \ | ||
tmp; \ | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be easier to just call into the compiler atomics (__atomic_bla
) no ?
STUB_ABORT(pthread_cond_broadcast); | ||
STUB_ABORT(pthread_self); | ||
STUB_ABORT(pthread_detach); | ||
STUB_ABORT(sigfillset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could probably be STUB_IGNORE, we'll never have signals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it can be STUB_IGNORE
, during the small experiences I've done so far it doesn't seems to be an issue as I never saw any abort exception raised. Could it worth to keep it as STUB_ABORT
and wait for the time an unikernel tells us when we need to add signal to solo5? :)
STUB_ABORT(pthread_self); | ||
STUB_ABORT(pthread_detach); | ||
STUB_ABORT(sigfillset); | ||
STUB_ABORT(usleep); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be something like calling solo5_yield(deadline, emptyset)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we currently can sleep
with Solo5 since we have the monotonic clock (it's just matter that we will block the only CPU we have).
STUB_ABORT(pthread_detach); | ||
STUB_ABORT(sigfillset); | ||
STUB_ABORT(usleep); | ||
STUB_ABORT(strerror_r); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is one:
#include <assert.h>
#include <errno.h>
#include <stdio.h>
#include <string.h>
int
mystrerror_r(int num, char *buf, size_t buflen)
{
static const char *errlist[] = {
"Undefined error: 0", /* 0 - ENOERROR */
"Bad file descriptor", /* 1 - EBADF */
"Result too large", /* 2 - ERANGE */
"Function not implemented", /* 3 - ENOSYS */
"Value too large to be stored in data type", /* 4 - OVERFLOW */
"No such file or directory", /* 5 - ENOENT */
"Invalid argument", /* 6 - EINVAL */
"Cannot allocate memory", /* 7 - ENOMEM */
"Too many open files", /* 8 - EMFILE */
"Device busy", /* 9 - EBUSY */
};
int nerr;
nerr = sizeof(errlist) / sizeof(errlist[0]);
if (num >= nerr || num < 0) {
errno = EINVAL;
return (EINVAL);
}
if (snprintf(buf, buflen, "%s", errlist[num]) >= buflen) {
errno = ERANGE;
return (ERANGE);
}
return (0);
}
int
main(void)
{
char buf[256];
int i;
assert(mystrerror_r(0, buf, strlen("Undefined error: 0")) == ERANGE);
assert(mystrerror_r(-1, buf, sizeof(buf)) == EINVAL);
assert(mystrerror_r(10, buf, sizeof(buf)) == EINVAL);
for (i = 0; i < 10; i++) {
assert (mystrerror_r(i, buf, sizeof(buf)) == 0);
printf("strerror_r[%d] = %s\n", i, buf);
}
return (0);
}
sam:tmp: cc -o strerror strerror.c -Wall && ./strerror
strerror_r[0] = Undefined error: 0
strerror_r[1] = Bad file descriptor
strerror_r[2] = Result too large
strerror_r[3] = Function not implemented
strerror_r[4] = Value too large to be stored in data type
strerror_r[5] = No such file or directory
strerror_r[6] = Invalid argument
strerror_r[7] = Cannot allocate memory
strerror_r[8] = Too many open files
strerror_r[9] = Device busy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh forgot to store errno
on the error cases, fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I agree it should be added, but as for sigfillset
, if no exception raised, maybe it's not needed?
I can't really see how &d->interruptor would be bad, but I'd try to dereference |
Here are some printing (sorry, this will be a bit long :/), we probably segfault before accessing
EDIT:
Here is how I read the assembly for
|
I edited the asm output, to my understanding, we fail before the test As |
I'm now sure that the failing line is |
Nice find ! For sure that is the problem. Interesting that the TLS is kept in For now you can cheat, just change the runtime for |
Ok, I tried and had to fight a bit with the |
I guess there are moar cases:
Writing a userland pthread scheduler is not an insane amount of work, especially because we need a limited subset of it. At any rate, if you're up to it we can do it at some point, I'm on holidays now contemplating the end of the universe and playing dwarf fortress, so can't really commit to anything. |
Update: I tried the dumb approach by allocating some memory for TLS and running the thing:
And now I have the following:
The caller for I'll be afk for the next week, but I think we're on good track for resolving that PR. Enjoy the end of the world and be safe ;) |
Update: The
The next step is to have a proper threads implementation and eventually checks why I can have with not enough memory (128MB is not sufficient :/):
|
Can't you just point to a static buffer though ? no need to malloc, but anyway, good find ! |
Yes, done in an update branch, I'll PR it at some point. And in the meantime, I discovered why the unikernel now needs a lot more memory. This is due to https://github.com/ocaml/ocaml/blob/ac52c3ab4a545d77eea034993a0947ba5e79762b/runtime/domain.c#L218 and https://github.com/ocaml/ocaml/blob/ac52c3ab4a545d77eea034993a0947ba5e79762b/runtime/domain.c#L724. The heap reservation is done once for the upper limit of possible domains (here I got the traces:
|
At this stage it's probably easier to make a compiler patch like '--with-max-domains=1'. |
It's probably a task that @shindere should be aware 👍. Any requests like this one deserves special attention in order to improve the toolchain compiling OCaml for our specific use. However, I'm not sure how to organize that in the middle/long term - and we still are on the experiment stage. |
mv ocaml/Makefile.sed ocaml/Makefile | ||
echo -e "\$$(ocamlyacc_PROGRAM)\$$(EXE):\n\tcp $(shell which ocamlyacc) yacc/\n" >> ocaml/Makefile | ||
# patch ocaml 5.0.0 runtime for single domain/thread solo5 | ||
sed -e 's/#define Max_domains 128/#define Max_domains 1/' ocaml/runtime/caml/domain.h > ocaml/runtime/caml/domain.h.sed && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it can be useful to be more robust here and use 's/#define Max_domains .+$/#define Max_domains 1/'
? It will modify both paths for Max_domains
definition (https://github.com/ocaml/ocaml/blob/021619f25d84dc0161537366f01c92f6a3980851/runtime/caml/domain.h#L34) and be resilient if in the future Max_domains
is set to anything else than 128.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, just remove the original domain.h after moving to .sed, if sed -e
fails, then domain.h will never be produced and compilation will fail. This also makes sure all the steps in && will have to succeed.
Just commenting on the `Max_domain` aspect here but let me know whether
it was a wider comment which was expected.
I quickly went through the code and read a comment in
`runtime/caml/comain.h` saying that the hard limit on the maximum number
of domains may go away in the future. This makes me unsure that upstream
will be willing to introduce an interface to specify it, because it
means that one will have to handle its deprecation if one day the hard
limit indeed goes away. But that's just my uninformed opinion, so if
being able to specify a max doamin is important to you it may be worth
discussiing with developers more involved than me in this particular
area of domains.
|
Hi @shindere, thanks for your feedback! I think you're right here :)
As @haesbaert said in #122 (comment) it's probably best at some point to change the allocation system in solo5 and to allow some memory to be requested and only reserve it when it's really used (the hint should be |
I understand #124 superseeds this PR to some degree, but in this PR there are still some review comments left as far as I can tell. Someone wants to drive the comments from here over to the other PR, so we can close this one and re-review the other one? |
I think, most of what we said on this PR was integrated into #124. The MacOS compilation is missing I think - but Solo5 already has an issue on such platform. I think we can safely close this PR. |
What about e.g. #122 (comment) ? |
For now this branch will only work withEDIT: ocaml-src from opam-repository works fine now.ocaml-src
pointing to https://github.com/kit-ty-kate/ocaml/tree/solo5-500 (all changes are waiting to be backported to the 5.0 branch)[copying my first comment for a more visible disclaimer]
For the ocaml-solo5 devs: Feel free to ask questions, rebase the branch, add changes to this branch (you should have the rights to, though don't forget to use git push --force-with-lease if you force-push in case i also add some changes at the same time). I might have not time to finish the work so please do continue it.
joint work with @TheLortex at the MirageOS retreat 2022 with some help of all the people present at the event.