Changes for managarm cancelable syscalls #884
Conversation
|
Yeah there are quite some debug logs left currently 😅 |
There was a problem hiding this comment.
LGTM
I do have a question, since this is not instantly obvious to me: would this code work without the changes in managarm, or are we looking at circular dependencies here? To my knowledge, merging in the managarm side first would result in some weird behavior.
Geertiebear
force-pushed
the
cancelable_syscalls
branch
from
1c76936 to
ac7c83e
Compare
Geertiebear
force-pushed
the
cancelable_syscalls
branch
from
ac7c83e to
506d9e4
Compare
Geertiebear
force-pushed
the
cancelable_syscalls
branch
from
506d9e4 to
922dd20
Compare
| auto element = globalQueue.dequeueSingle(false); | ||
| if (!element) { | ||
| element = globalQueue.dequeueSingle(); | ||
| __ensure(element); | ||
| } |
There was a problem hiding this comment.
This is the only call where we pass false to dequeueSingle() (AFAICT) and we immediately call into dequeueSingle() again. So what is the difference compared to just doing dequeueSingle(true) here?
There was a problem hiding this comment.
Note we use protocol errors to signify cancellation (e.g., in read() you check for managarm::fs::Errors::INTERRUPTED), so maybe the ignoreCancel = false case is not necessary (and we can always re-try if helFutexWait is interrupted)?
There was a problem hiding this comment.
Iirc I did this because some threads were getting some spurious interrupts, but now that I'm thinking about it, that re-entering dequeueSingle should never be an issue in that case.
| auto result = parseSimple(element); | ||
| if (!element) { | ||
| return EINTR; | ||
| } | ||
| auto result = parseSimple(*element); |
There was a problem hiding this comment.
We have this change for sleep but AFAICT sleep() is still not cancelable since we don't integrate it with the cancellation event. We could drop support for sleep() in the initial PR.
There was a problem hiding this comment.
Given the changes done to the return type of deqeueSingle, handling the error seems preferable to __ensure(element).
| @@ -1628,12 +1629,14 @@ int sys_mknodat(int dirfd, const char *path, int mode, int dev) { | |||
| } | |||
|
|
|||
| int sys_read(int fd, void *data, size_t max_size, ssize_t *bytes_read) { | |||
| SignalGuard sguard; | |||
There was a problem hiding this comment.
Why do we need to do this?
This prevents a signal to be raised (= posix-subsystem won't jump to the signal handler), but it should not prevent POSIX from interrupting the thread. I think we still want to keep this.
no92
force-pushed
the
cancelable_syscalls
branch
from
a0a4f8a to
1cf7659
Compare
Since the changes are incomplete and a little unstable, marking it as draft right now.