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 all commits
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
33 changes: 22 additions & 11 deletions sysdeps/managarm/generic/file.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "mlibc/debug.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Is this LSP? In any case, this should be replaced with <>

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should be <>.

#include <asm/ioctls.h>
#include <dirent.h>
#include <errno.h>
Expand Down Expand Up @@ -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;
Copy link
Member

@avdgrinten avdgrinten Dec 30, 2024

Choose a reason for hiding this comment

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

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.


auto handle = getHandleForFd(fd);
if (!handle)
return EBADF;

HelHandle cancel_handle;
HEL_CHECK(helCreateOneshotEvent(&cancel_handle));
helix::UniqueDescriptor cancel_event{cancel_handle};

managarm::fs::CntRequest<MemoryAllocator> req(getSysdepsAllocator());
req.set_req_type(managarm::fs::CntReqType::READ);
req.set_fd(fd);
Expand All @@ -1642,17 +1645,21 @@ int sys_read(int fd, void *data, size_t max_size, ssize_t *bytes_read) {
frg::string<MemoryAllocator> ser(getSysdepsAllocator());
req.SerializeToString(&ser);

auto [offer, send_req, imbue_creds, recv_resp, recv_data] = exchangeMsgsSync(
handle,
helix_ng::offer(
helix_ng::sendBuffer(ser.data(), ser.size()),
helix_ng::imbueCredentials(),
helix_ng::recvInline(),
helix_ng::recvBuffer(data, max_size)
)
);
auto [offer, push_req, send_req, imbue_creds, recv_resp, recv_data] =
exchangeMsgsSyncCancellable(
handle,
cancel_handle,
helix_ng::offer(
helix_ng::sendBuffer(ser.data(), ser.size()),
helix_ng::pushDescriptor(cancel_event),
helix_ng::imbueCredentials(),
helix_ng::recvInline(),
helix_ng::recvBuffer(data, max_size)
)
);

HEL_CHECK(offer.error());
HEL_CHECK(push_req.error());
HEL_CHECK(send_req.error());
HEL_CHECK(imbue_creds.error());
HEL_CHECK(recv_resp.error());
Expand All @@ -1673,6 +1680,10 @@ int sys_read(int fd, void *data, size_t max_size, ssize_t *bytes_read) {
} else if (resp.error() == managarm::fs::Errors::END_OF_FILE) {
*bytes_read = 0;
return 0;
} else if (resp.error() == managarm::fs::Errors::INTERRUPTED) {
HEL_CHECK(recv_data.error());
*bytes_read = recv_data.actualLength();
return EINTR;
} else {
__ensure(resp.error() == managarm::fs::Errors::SUCCESS);
HEL_CHECK(recv_data.error());
Expand Down
26 changes: 22 additions & 4 deletions sysdeps/managarm/generic/fork-exec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,29 +54,44 @@ int sys_futex_wake(int *pointer) {
}

int sys_waitpid(pid_t pid, int *status, int flags, struct rusage *ru, pid_t *ret_pid) {
SignalGuard sguard;
if (ru) {
mlibc::infoLogger() << "mlibc: struct rusage in sys_waitpid is unsupported" << frg::endlog;
return ENOSYS;
}

HelHandle cancel_handle;
HEL_CHECK(helCreateOneshotEvent(&cancel_handle));
helix::UniqueDescriptor cancel_event{cancel_handle};

managarm::posix::CntRequest<MemoryAllocator> req(getSysdepsAllocator());
req.set_request_type(managarm::posix::CntReqType::WAIT);
req.set_pid(pid);
req.set_flags(flags);

auto [offer, send_head, recv_resp] = exchangeMsgsSync(
auto [offer, send_head, push_descriptor, recv_resp] = exchangeMsgsSyncCancellable(
getPosixLane(),
cancel_handle,
helix_ng::offer(
helix_ng::sendBragiHeadOnly(req, getSysdepsAllocator()), helix_ng::recvInline()
helix_ng::sendBragiHeadOnly(req, getSysdepsAllocator()),
helix_ng::pushDescriptor(cancel_event),
helix_ng::recvInline()
)
);

HEL_CHECK(offer.error());
HEL_CHECK(send_head.error());
HEL_CHECK(push_descriptor.error());
HEL_CHECK(recv_resp.error());

managarm::posix::SvrResponse<MemoryAllocator> resp(getSysdepsAllocator());
resp.ParseFromArray(recv_resp.data(), recv_resp.length());
if (resp.error() == managarm::posix::Errors::ILLEGAL_ARGUMENTS) {
return EINVAL;
}
if (resp.error() == managarm::posix::Errors::INTERRUPTED) {
mlibc::infoLogger() << "returning EINT" << frg::endlog;
Copy link
Member

Choose a reason for hiding this comment

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

Should we put this behind some debug flag?

return EINTR;
}
__ensure(resp.error() == managarm::posix::Errors::SUCCESS);
if (status)
*status = resp.mode();
Expand Down Expand Up @@ -148,7 +163,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
32 changes: 21 additions & 11 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 Expand Up @@ -297,7 +299,10 @@ int sys_seek(int fd, off_t offset, int whence, off_t *new_offset) {
int sys_read(int fd, void *data, size_t length, ssize_t *bytes_read) {
cacheFileTable();
auto lane = fileTable[fd];
HelAction actions[5];
HelAction actions[6];

HelHandle cancel_handle;
HEL_CHECK(helCreateOneshotEvent(&cancel_handle));

managarm::fs::CntRequest<MemoryAllocator> req(getAllocator());
req.set_req_type(managarm::fs::CntReqType::READ);
Expand All @@ -314,25 +319,30 @@ int sys_read(int fd, void *data, size_t length, ssize_t *bytes_read) {
actions[1].flags = kHelItemChain;
actions[1].buffer = ser.data();
actions[1].length = ser.size();
actions[2].type = kHelActionImbueCredentials;
actions[2].handle = kHelThisThread;
actions[2].type = kHelActionPushDescriptor;
actions[2].handle = cancel_handle;
actions[2].flags = kHelItemChain;
actions[3].type = kHelActionRecvInline;
actions[3].type = kHelActionImbueCredentials;
actions[3].handle = kHelThisThread;
actions[3].flags = kHelItemChain;
actions[4].type = kHelActionRecvToBuffer;
actions[4].flags = 0;
actions[4].buffer = data;
actions[4].length = length;
HEL_CHECK(helSubmitAsync(lane, actions, 5, globalQueue->getHandle(), 0, 0));
actions[4].type = kHelActionRecvInline;
actions[4].flags = kHelItemChain;
actions[5].type = kHelActionRecvToBuffer;
actions[5].flags = 0;
actions[5].buffer = data;
actions[5].length = length;
HEL_CHECK(helSubmitAsync(lane, actions, 6, globalQueue->getHandle(), 0, 0));

auto element = globalQueue->dequeueSingle();
auto offer = parseHandle(element);
auto send_req = parseSimple(element);
auto push_desc = parseSimple(element);
auto imbue_creds = parseSimple(element);
auto recv_resp = parseInline(element);
auto recv_data = parseLength(element);
HEL_CHECK(offer->error);
HEL_CHECK(send_req->error);
HEL_CHECK(push_desc->error);
HEL_CHECK(imbue_creds->error);
HEL_CHECK(recv_resp->error);
HEL_CHECK(recv_data->error);
Expand Down
Loading