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

pd-mapper.service: don't start if /sys/class/remoteproc is empty #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AdamWill
Copy link

If there are no maps, pd-mapper always errors out with message "no pd maps available". This means the service fails and shows up as failed in systemctl --failed, which is ugly and annoying. For Fedora, it's also against the release criteria - there should be no failed services on startup of a default install, but on a default aarch64 install, this service usually fails.

To avoid this, let's add a condition saying not to try and start the service if /sys/class/remoteproc is not empty. Note this condition was added in systemd 244, but zbyzsek tells me that older systemd would not error out, it would just print a warning and ignore it, so that should be fine.

If there are no maps, pd-mapper always errors out with message
"no pd maps available". This means the service fails and shows
up as failed in systemctl --failed, which is ugly and annoying.
For Fedora, it's also against the release criteria - there should
be no failed services on startup of a default install, but on
a default aarch64 install, this service usually fails.

To avoid this, let's add a condition saying not to try and start
the service if /sys/class/remoteproc is not empty. Note this
condition was added in systemd 244, but zbyzsek tells me that
older systemd would not error out, it would just print a warning
and ignore it, so that should be fine.

Signed-off-by: Adam Williamson <[email protected]>
@konradybcio konradybcio requested a review from lumag August 30, 2024 12:11
@lumag
Copy link

lumag commented Aug 30, 2024

@AdamWill what if the remoteproc drivers are built as modules and get loaded after the pd-mapper was attempted?

@AdamWill
Copy link
Author

Hmm...yeah, I guess that's an issue, since this has Restart=always, so in that scenario it would fail until the modules loaded and then eventually succeed.

Not sure if there's a nice way to handle this. @keszybz is there an elegant service config that can be used in this case?

@AdamWill
Copy link
Author

still, that does make the problem worse, because on a system which really doesn't have any remoteproc support, the service will constantly try and startup, forever, unless the admin manually disables it. It won't just fail once on boot and give up.

@lumag
Copy link

lumag commented Aug 30, 2024

@AdamWill I think the proper way is to rewrite pd-mapper to support delaying daemon init. If it can not find remoteprocs, it should start, but use libudev to wait for remoteprocs to appear.

@lumag
Copy link

lumag commented Aug 30, 2024

@AdamWill hoefully this becomes less important now. Starting from 6.11 there is an in-kernel implementation of PD mapper, so userspace daemon can be disabled completely.

@keszybz
Copy link

keszybz commented Aug 31, 2024

@AdamWill what if the remoteproc drivers are built as modules and get loaded after the pd-mapper was attempted?

Modules are normally loaded early. This service doesn't specify anything special, so it'd be started after basic.target, and modules would be loaded earlier than that.

@lumag
Copy link

lumag commented Sep 2, 2024

I couldn't find it in the documentation, why would Condition keep on attempting to start it. Maybe a Condition + dropping the RestartAways is a way to go?

@AdamWill
Copy link
Author

AdamWill commented Sep 2, 2024

It's Restart = always that will cause it to restart on any failure, yes. I don't have any opinion on whether that's appropriate for this tool or not.

@lumag
Copy link

lumag commented Sep 2, 2024

I think Restart = always can be dropped. @konradybcio @andersson WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants