[RFC PATCH v3 9/9] libcamera: process: Remove `ProcessManager` singleton
Barnabás Pőcze
barnabas.pocze at ideasonboard.com
Thu Mar 27 17:13:05 CET 2025
Hi
2025. 03. 26. 16:03 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
>
> Thank you for the patch.
>
> On Tue, Mar 25, 2025 at 07:08:21PM +0100, Barnabás Pőcze wrote:
>> The `ProcessManager` is a singleton class to handle `SIGCHLD` signals
>> and report the exit status to the particular `Process` instances.
>>
>> However, having a singleton in a library is not favourable and it is
>> even less favourable if it installs a signal handler.
>>
>> Using pidfd it is possible to avoid the need for the signal handler;
>> and the `Process` objects can watch their pidfd themselves, eliminating
>> the need for the `ProcessManager` class altogether.
>>
>> `P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change
>> raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,
>> `pidfd_send_signal()` were all introduced earlier.
>>
>> Furthermore, the call to the `unshare()` system call can be removed
>> as those options can be passed to `clone3()` directly.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
>> ---
>> include/libcamera/internal/camera_manager.h | 1 -
>> include/libcamera/internal/process.h | 34 +--
>> meson.build | 2 +-
>> src/libcamera/process.cpp | 241 +++++---------------
>> test/ipc/unixsocket_ipc.cpp | 2 -
>> test/log/log_process.cpp | 2 -
>> test/process/process_test.cpp | 2 -
>> 7 files changed, 55 insertions(+), 229 deletions(-)
>>
>> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
>> index 0150ca61f..5dfbe1f65 100644
>> --- a/include/libcamera/internal/camera_manager.h
>> +++ b/include/libcamera/internal/camera_manager.h
>> @@ -65,7 +65,6 @@ private:
>> std::unique_ptr<DeviceEnumerator> enumerator_;
>>
>> std::unique_ptr<IPAManager> ipaManager_;
>> - ProcessManager processManager_;
>> };
>>
>> } /* namespace libcamera */
>> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h
>> index 14391d60a..e600a49f8 100644
>> --- a/include/libcamera/internal/process.h
>> +++ b/include/libcamera/internal/process.h
>> @@ -7,7 +7,6 @@
>>
>> #pragma once
>>
>> -#include <signal.h>
>> #include <string>
>>
>> #include <libcamera/base/signal.h>
>> @@ -44,41 +43,14 @@ public:
>> private:
>> LIBCAMERA_DISABLE_COPY_AND_MOVE(Process)
>>
>> - int isolate();
>> - void died(int wstatus);
>> -
>> pid_t pid_;
>> enum ExitStatus exitStatus_;
>> int exitCode_;
>>
>> - friend class ProcessManager;
>> -};
>> -
>> -class ProcessManager
>> -{
>> -public:
>> - ProcessManager();
>> - ~ProcessManager();
>> -
>> - void registerProcess(Process *proc);
>> -
>> - static ProcessManager *instance();
>> -
>> - int writePipe() const;
>> -
>> - const struct sigaction &oldsa() const;
>> -
>> -private:
>> - static ProcessManager *self_;
>> -
>> - void sighandler();
>> -
>> - std::list<Process *> processes_;
>> -
>> - struct sigaction oldsa_;
>> + UniqueFD pidfd_;
>> + std::unique_ptr<EventNotifier> pidfdNotify_;
>>
>> - EventNotifier *sigEvent_;
>> - UniqueFD pipe_[2];
>> + void onPidfdNotify();
>
> We declare member functions before member variables.
>
>> };
>>
>> } /* namespace libcamera */
>> diff --git a/meson.build b/meson.build
>> index 00291d628..bd2c88dfa 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -265,7 +265,7 @@ subdir('Documentation')
>> subdir('test')
>>
>> if not meson.is_cross_build()
>> - kernel_version_req = '>= 5.0.0'
>> + kernel_version_req = '>= 5.4.0'
>> kernel_version = run_command('uname', '-r', check : true).stdout().strip()
>> if not kernel_version.version_compare(kernel_version_req)
>> warning('The current running kernel version @0@ is too old to run libcamera.'
>> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
>> index 5fa813300..cadf5c43e 100644
>> --- a/src/libcamera/process.cpp
>> +++ b/src/libcamera/process.cpp
>> @@ -10,15 +10,19 @@
>> #include <algorithm>
>> #include <dirent.h>
>> #include <fcntl.h>
>> -#include <list>
>> +#include <sched.h>
>> #include <signal.h>
>> #include <string.h>
>> #include <sys/socket.h>
>> +#include <sys/syscall.h>
>> #include <sys/types.h>
>> #include <sys/wait.h>
>> #include <unistd.h>
>> #include <vector>
>>
>> +#include <linux/sched.h>
>> +#include <linux/wait.h> /* glibc only provides `P_PIDFD` in `sys/wait.h` since 2.36 */
>> +
>> #include <libcamera/base/event_notifier.h>
>> #include <libcamera/base/log.h>
>> #include <libcamera/base/utils.h>
>> @@ -32,37 +36,8 @@ namespace libcamera {
>>
>> LOG_DEFINE_CATEGORY(Process)
>>
>> -/**
>> - * \class ProcessManager
>> - * \brief Manager of processes
>> - *
>> - * The ProcessManager singleton keeps track of all created Process instances,
>> - * and manages the signal handling involved in terminating processes.
>> - */
>> -
>> namespace {
>>
>> -void sigact(int signal, siginfo_t *info, void *ucontext)
>> -{
>> -#pragma GCC diagnostic push
>> -#pragma GCC diagnostic ignored "-Wunused-result"
>> - /*
>> - * We're in a signal handler so we can't log any message, and we need
>> - * to continue anyway.
>> - */
>> - char data = 0;
>> - write(ProcessManager::instance()->writePipe(), &data, sizeof(data));
>> -#pragma GCC diagnostic pop
>> -
>> - const struct sigaction &oldsa = ProcessManager::instance()->oldsa();
>> - if (oldsa.sa_flags & SA_SIGINFO) {
>> - oldsa.sa_sigaction(signal, info, ucontext);
>> - } else {
>> - if (oldsa.sa_handler != SIG_IGN && oldsa.sa_handler != SIG_DFL)
>> - oldsa.sa_handler(signal);
>> - }
>> -}
>> -
>> void closeAllFdsExcept(Span<const int> fds)
>> {
>> std::vector<int> v(fds.begin(), fds.end());
>> @@ -113,129 +88,6 @@ void closeAllFdsExcept(Span<const int> fds)
>>
>> } /* namespace */
>>
>> -void ProcessManager::sighandler()
>> -{
>> - char data;
>> - ssize_t ret = read(pipe_[0].get(), &data, sizeof(data));
>> - if (ret < 0) {
>> - LOG(Process, Error)
>> - << "Failed to read byte from signal handler pipe";
>> - return;
>> - }
>> -
>> - for (auto it = processes_.begin(); it != processes_.end();) {
>> - Process *process = *it;
>> -
>> - int wstatus;
>> - pid_t pid = waitpid(process->pid_, &wstatus, WNOHANG);
>> - if (process->pid_ != pid) {
>> - ++it;
>> - continue;
>> - }
>> -
>> - it = processes_.erase(it);
>> - process->died(wstatus);
>> - }
>> -}
>> -
>> -/**
>> - * \brief Register process with process manager
>> - * \param[in] proc Process to register
>> - *
>> - * This function registers the \a proc with the process manager. It
>> - * shall be called by the parent process after successfully forking, in
>> - * order to let the parent signal process termination.
>> - */
>> -void ProcessManager::registerProcess(Process *proc)
>> -{
>> - processes_.push_back(proc);
>> -}
>> -
>> -ProcessManager *ProcessManager::self_ = nullptr;
>> -
>> -/**
>> - * \brief Construct a ProcessManager instance
>> - *
>> - * The ProcessManager class is meant to only be instantiated once, by the
>> - * CameraManager.
>> - */
>> -ProcessManager::ProcessManager()
>> -{
>> - if (self_)
>> - LOG(Process, Fatal)
>> - << "Multiple ProcessManager objects are not allowed";
>> -
>> - sigaction(SIGCHLD, NULL, &oldsa_);
>> -
>> - struct sigaction sa;
>> - memset(&sa, 0, sizeof(sa));
>> - sa.sa_sigaction = &sigact;
>> - memcpy(&sa.sa_mask, &oldsa_.sa_mask, sizeof(sa.sa_mask));
>> - sigaddset(&sa.sa_mask, SIGCHLD);
>> - sa.sa_flags = oldsa_.sa_flags | SA_SIGINFO;
>> -
>> - sigaction(SIGCHLD, &sa, NULL);
>> -
>> - int pipe[2];
>> - if (pipe2(pipe, O_CLOEXEC | O_DIRECT | O_NONBLOCK))
>> - LOG(Process, Fatal)
>> - << "Failed to initialize pipe for signal handling";
>> -
>> - pipe_[0] = UniqueFD(pipe[0]);
>> - pipe_[1] = UniqueFD(pipe[1]);
>> -
>> - sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);
>> - sigEvent_->activated.connect(this, &ProcessManager::sighandler);
>> -
>> - self_ = this;
>> -}
>> -
>> -ProcessManager::~ProcessManager()
>> -{
>> - sigaction(SIGCHLD, &oldsa_, NULL);
>> -
>> - delete sigEvent_;
>> -
>> - self_ = nullptr;
>> -}
>> -
>> -/**
>> - * \brief Retrieve the Process manager instance
>> - *
>> - * The ProcessManager is constructed by the CameraManager. This function shall
>> - * be used to retrieve the single instance of the manager.
>> - *
>> - * \return The Process manager instance
>> - */
>> -ProcessManager *ProcessManager::instance()
>> -{
>> - return self_;
>> -}
>> -
>> -/**
>> - * \brief Retrieve the Process manager's write pipe
>> - *
>> - * This function is meant only to be used by the static signal handler.
>> - *
>> - * \return Pipe for writing
>> - */
>> -int ProcessManager::writePipe() const
>> -{
>> - return pipe_[1].get();
>> -}
>> -
>> -/**
>> - * \brief Retrive the old signal action data
>> - *
>> - * This function is meant only to be used by the static signal handler.
>> - *
>> - * \return The old signal action data
>> - */
>> -const struct sigaction &ProcessManager::oldsa() const
>> -{
>> - return oldsa_;
>> -}
>> -
>> /**
>> * \class Process
>> * \brief Process object
>> @@ -286,8 +138,6 @@ int Process::start(const std::string &path,
>> Span<const std::string> args,
>> Span<const int> fds)
>> {
>> - int ret;
>> -
>> if (pid_ > 0)
>> return -EBUSY;
>>
>> @@ -296,20 +146,26 @@ int Process::start(const std::string &path,
>> return -EINVAL;
>> }
>>
>> - int childPid = fork();
>> - if (childPid == -1) {
>> - ret = -errno;
>> + int pidfd;
>> + clone_args cargs = {};
>> +
>> + cargs.flags = CLONE_CLEAR_SIGHAND | CLONE_PIDFD | CLONE_NEWUSER | CLONE_NEWNET;
>> + cargs.pidfd = reinterpret_cast<uintptr_t>(&pidfd);
>> + cargs.exit_signal = SIGCHLD;
>> +
>> + long childPid = syscall(SYS_clone3, &cargs, sizeof(cargs));
>> + if (childPid < 0) {
>> + int ret = -errno;
>> LOG(Process, Error) << "Failed to fork: " << strerror(-ret);
>> return ret;
>> - } else if (childPid) {
>> - pid_ = childPid;
>> - ProcessManager::instance()->registerProcess(this);
>> + }
>>
>> - return 0;
>> + if (childPid) {
>> + pid_ = childPid;
>> + pidfd_ = UniqueFD(pidfd);
>> + pidfdNotify_ = std::make_unique<EventNotifier>(pidfd_.get(), EventNotifier::Type::Read);
>> + pidfdNotify_->activated.connect(this, &Process::onPidfdNotify);
>> } else {
>> - if (isolate())
>> - _exit(EXIT_FAILURE);
>> -
>> closeAllFdsExcept(fds);
>>
>> const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
>> @@ -328,35 +184,40 @@ int Process::start(const std::string &path,
>>
>> exit(EXIT_FAILURE);
>> }
>> -}
>> -
>> -int Process::isolate()
>> -{
>> - int ret = unshare(CLONE_NEWUSER | CLONE_NEWNET);
>> - if (ret) {
>> - ret = -errno;
>> - LOG(Process, Error) << "Failed to unshare execution context: "
>> - << strerror(-ret);
>> - return ret;
>> - }
>>
>> return 0;
>> }
>>
>> -/**
>> - * \brief SIGCHLD handler
>> - * \param[in] wstatus The status as output by waitpid()
>> - *
>> - * This function is called when the process associated with Process terminates.
>> - * It emits the Process::finished signal.
>> - */
>> -void Process::died(int wstatus)
>> +void Process::onPidfdNotify()
>> {
>> - pid_ = -1;
>> - exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;
>> - exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;
>> + auto pidfdNotify = std::exchange(pidfdNotify_, {});
>> + auto pidfd = std::exchange(pidfd_, {});
>> + auto pid = std::exchange(pid_, -1);
>> +
>> + ASSERT(pidfdNotify);
>
> Do we have to guard against this, or could we just write
>
> pidfdNotify_.reset();
>
> ?
>
>> + ASSERT(pidfd.isValid());
>> + ASSERT(pid > 0);
>> +
>> + siginfo_t info;
>> +
>> + if (waitid(static_cast<idtype_t>(P_PIDFD), pidfd.get(), &info, WNOHANG | WEXITED) >= 0) {
>> + ASSERT(info.si_pid == pid);
>>
>> - finished.emit(exitStatus_, exitCode_);
>> + LOG(Process, Debug)
>> + << this << "[" << pid << ":" << pidfd.get() << "]"
>
> Sounds like a candidate to inherit from Loggable and avoid duplication
> of this in log message. You would need to reset pid_ to -1 at the end of
> the function though. Except it should be done before emitting the
> finished signal, as there's no running_ variable anymore. Annoying... I
> wonder if we should keep running_, or if there's a cleaner way.
I'll give it some thought because this indeed does not look ideal.
Also, regrettably it seems chromeos does not have new enough linux headers.
I'm wondering if it would be possible to update the CI image to something newer?
I have tested release-R135-16209.B locally, and that appears to have the
necessary definitions.
Regards,
Barnabás Pőcze
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> + << " code: " << info.si_code
>> + << " status: " << info.si_status;
>> +
>> + exitStatus_ = info.si_code == CLD_EXITED ? Process::NormalExit : Process::SignalExit;
>> + exitCode_ = info.si_code == CLD_EXITED ? info.si_status : -1;
>> +
>> + finished.emit(exitStatus_, exitCode_);
>> + } else {
>> + int err = errno;
>> + LOG(Process, Warning)
>> + << this << "[" << pid << ":" << pidfd.get() << "]"
>> + << " waitid() failed: " << strerror(err);
>> + }
>> }
>>
>> /**
>> @@ -394,8 +255,8 @@ void Process::died(int wstatus)
>> */
>> void Process::kill()
>> {
>> - if (pid_ > 0)
>> - ::kill(pid_, SIGKILL);
>> + if (pidfd_.isValid())
>> + syscall(SYS_pidfd_send_signal, pidfd_.get(), SIGKILL, nullptr, 0);
>> }
>>
>> } /* namespace libcamera */
>> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
>> index df7d9c2b4..f3e3c09ef 100644
>> --- a/test/ipc/unixsocket_ipc.cpp
>> +++ b/test/ipc/unixsocket_ipc.cpp
>> @@ -209,8 +209,6 @@ protected:
>> }
>>
>> private:
>> - ProcessManager processManager_;
>> -
>> unique_ptr<IPCPipeUnixSocket> ipc_;
>> };
>>
>> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
>> index 9609e23d5..367b78803 100644
>> --- a/test/log/log_process.cpp
>> +++ b/test/log/log_process.cpp
>> @@ -137,8 +137,6 @@ private:
>> exitCode_ = exitCode;
>> }
>>
>> - ProcessManager processManager_;
>> -
>> Process proc_;
>> Process::ExitStatus exitStatus_ = Process::NotExited;
>> string logPath_;
>> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
>> index a88d8fef1..fe76666e6 100644
>> --- a/test/process/process_test.cpp
>> +++ b/test/process/process_test.cpp
>> @@ -86,8 +86,6 @@ private:
>> exitCode_ = exitCode;
>> }
>>
>> - ProcessManager processManager_;
>> -
>> Process proc_;
>> enum Process::ExitStatus exitStatus_;
>> int exitCode_;
>
More information about the libcamera-devel
mailing list