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

Changes for managarm cancelable syscalls #884

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions sysdeps/managarm/generic/entry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ namespace {
thread_local unsigned __mlibc_gsf_nesting;
thread_local void *__mlibc_cached_thread_page;
thread_local HelHandle *cachedFileTable;
thread_local HelHandle *cancelEvent;

// This construction is a bit weird: Even though the variables above
// are thread_local we still protect their initialization with a pthread_once_t
Expand All @@ -48,6 +49,7 @@ void actuallyCacheInfos() {
__mlibc_cached_thread_page = data.threadPage;
cachedFileTable = data.fileTable;
__mlibc_clk_tracker_page = data.clockTrackerPage;
cancelEvent = data.cancelRequestEvent;
}
} // namespace

Expand Down Expand Up @@ -107,6 +109,11 @@ HelHandle getHandleForFd(int fd) {

void clearCachedInfos() { has_cached_infos = PTHREAD_ONCE_INIT; }

void setCurrentRequestEvent(HelHandle event) {
pthread_once(&has_cached_infos, &actuallyCacheInfos);
__atomic_store_n(cancelEvent, event, __ATOMIC_RELEASE);
}

struct LibraryGuard {
LibraryGuard();
};
Expand Down
5 changes: 4 additions & 1 deletion sysdeps/managarm/generic/fork-exec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,10 @@ int sys_sleep(time_t *secs, long *nanos) {
));

auto element = globalQueue.dequeueSingle();
auto result = parseSimple(element);
if (!element) {
return EINTR;
}
auto result = parseSimple(*element);
Comment on lines -151 to +169
Copy link
Member

Choose a reason for hiding this comment

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

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.

HEL_CHECK(result->error);

*secs = 0;
Expand Down
79 changes: 63 additions & 16 deletions sysdeps/managarm/include/mlibc/posix-pipe.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
#include <hel-syscalls.h>
#include <hel.h>

#include <frg/optional.hpp>
#include <mlibc/debug.hpp>

struct SignalGuard {
SignalGuard();

Expand Down Expand Up @@ -121,24 +124,29 @@ struct Queue {

void trim() {}

ElementHandle dequeueSingle() {
frg::optional<ElementHandle> dequeueSingle(bool ignoreCancel = true) {
while (true) {
__ensure(_retrieveIndex != _nextIndex);

bool done;
_waitProgressFutex(&done);
auto progress = _waitProgressFutex(ignoreCancel);

auto n = _numberOf(_retrieveIndex);
__ensure(_refCount[n]);

if (done) {
if (progress == FutexProgress::DONE) {
retire(n);

_lastProgress = 0;
_retrieveIndex = ((_retrieveIndex + 1) & kHelHeadMask);
continue;
}

if (progress == FutexProgress::CANCELLED) {
__ensure(!ignoreCancel);
mlibc::infoLogger() << "cancel detected" << frg::endlog;
return frg::null_opt;
}

// Dequeue the next element.
auto ptr = (char *)_chunks[n] + sizeof(HelChunk) + _lastProgress;
auto element = reinterpret_cast<HelElement *>(ptr);
Expand Down Expand Up @@ -175,18 +183,21 @@ struct Queue {
HEL_CHECK(helFutexWake(&_queue->headFutex));
}

void _waitProgressFutex(bool *done) {
enum class FutexProgress {
DONE,
IN_PROGRESS,
CANCELLED,
};

FutexProgress _waitProgressFutex(bool ignoreCancel) {
while (true) {
auto futex = __atomic_load_n(&_retrieveChunk()->progressFutex, __ATOMIC_ACQUIRE);
__ensure(!(futex & ~(kHelProgressMask | kHelProgressWaiters | kHelProgressDone)));
do {
if (_lastProgress != (futex & kHelProgressMask)) {
*done = false;
return;
} else if (futex & kHelProgressDone) {
*done = true;
return;
}
if (_lastProgress != (futex & kHelProgressMask))
return FutexProgress::IN_PROGRESS;
else if (futex & kHelProgressDone)
return FutexProgress::DONE;

if (futex & kHelProgressWaiters)
break; // Waiters bit is already set (in a previous iteration).
Expand All @@ -199,9 +210,17 @@ struct Queue {
__ATOMIC_ACQUIRE
));

HEL_CHECK(helFutexWait(
int err = helFutexWait(
&_retrieveChunk()->progressFutex, _lastProgress | kHelProgressWaiters, -1
));
);
if (err == kHelErrCancelled) {
if (ignoreCancel) {
continue;
}

return FutexProgress::CANCELLED;
}
HEL_CHECK(err);
}
}

Expand Down Expand Up @@ -261,6 +280,7 @@ inline HelHandleResult *parseHandle(ElementHandle &element) {
HelHandle getPosixLane();
HelHandle *cacheFileTable();
HelHandle getHandleForFd(int fd);
void setCurrentRequestEvent(HelHandle event);
void clearCachedInfos();

extern thread_local Queue globalQueue;
Expand All @@ -278,12 +298,39 @@ auto exchangeMsgsSync(HelHandle descriptor, Args &&...args) {
);

auto element = globalQueue.dequeueSingle();
void *ptr = element.data();
__ensure(element);
void *ptr = element->data();

[&]<size_t... p>(std::index_sequence<p...>) {
(results.template get<p>().parse(ptr, *element), ...);
}(std::make_index_sequence<std::tuple_size_v<decltype(results)>>{});

return results;
}

template <typename... Args>
auto exchangeMsgsSyncCancellable(HelHandle descriptor, HelHandle event, Args &&...args) {
auto results = helix_ng::createResultsTuple(args...);
auto actions = helix_ng::chainActionArrays(args...);

setCurrentRequestEvent(event);
HEL_CHECK(
helSubmitAsync(descriptor, actions.data(), actions.size(), globalQueue.getQueue(), 0, 0)
);

auto element = globalQueue.dequeueSingle(false);
if (!element) {
element = globalQueue.dequeueSingle();
__ensure(element);
}
Comment on lines +321 to +325
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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)?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

void *ptr = element->data();

[&]<size_t... p>(std::index_sequence<p...>) {
(results.template get<p>().parse(ptr, element), ...);
(results.template get<p>().parse(ptr, *element), ...);
}(std::make_index_sequence<std::tuple_size_v<decltype(results)>>{});

setCurrentRequestEvent(kHelNullHandle);

return results;
}

Expand Down
6 changes: 4 additions & 2 deletions sysdeps/managarm/rtld-generic/support.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,10 @@ struct Queue {
__ATOMIC_ACQUIRE
));

HEL_CHECK(helFutexWait(&_chunk->progressFutex, _lastProgress | kHelProgressWaiters, -1)
);
int err = helFutexWait(&_chunk->progressFutex, _lastProgress | kHelProgressWaiters, -1);
if (err == kHelErrCancelled)
continue;
HEL_CHECK(err);
}
}

Expand Down